Closed
Conversation
ockham
added
[Type] Bug
|
Size Change: +10 B (0%) Total Size: 1.96 MB
ℹ️ View Unchanged
|
gziolo
reviewed
Oct 6, 2025
| setModalState( null ); | ||
| }; | ||
|
|
||
| const registeredSources = getBlockBindingsSources(); |
Member
There was a problem hiding this comment.
Is the selector used internally in getBlockBindingsSources private and can't be accessed through useSelect?
Contributor
Author
There was a problem hiding this comment.
Seems that way:
gutenberg/packages/blocks/src/api/registration.js
Lines 1083 to 1085 in c5e8bd0
unlock it here, though (?)
(Would you prefer using the selector?)
Member
There was a problem hiding this comment.
I assume we would prefer any changes in binding sources would trigger re-render, so this could be handled inside the useSelect that consumes it.
Member
There was a problem hiding this comment.
In other words, I think the error comes from the fact that getAllBlockBindingsSources returns different results on subsequent executions and that's why _sources makes React think it's the same object even when the data changes.
Contributor
Author
There was a problem hiding this comment.
That should stop though if I move the call outside of the useSelect, and add the resulting registeredSources as a param to useSelect, no? (Unfortunately, it didn't.)
Member
Contributor
Author
There was a problem hiding this comment.
BTW I tried the following against trunk. The issue persists 😕
diff --git a/packages/block-editor/src/hooks/block-bindings.js b/packages/block-editor/src/hooks/block-bindings.js
index 87ab4e3632..39206bef7a 100644
--- a/packages/block-editor/src/hooks/block-bindings.js
+++ b/packages/block-editor/src/hooks/block-bindings.js
@@ -7,7 +7,7 @@ import fastDeepEqual from 'fast-deep-equal/es6';
* WordPress dependencies
*/
import { __ } from '@wordpress/i18n';
-import { getBlockBindingsSources, getBlockType } from '@wordpress/blocks';
+import { getBlockType, store as blocksStore } from '@wordpress/blocks';
import {
__experimentalItemGroup as ItemGroup,
__experimentalItem as Item,
@@ -302,9 +302,9 @@ export const BlockBindingsPanel = ( { name: blockName, metadata } ) => {
// Use useSelect to ensure sources are updated whenever there are updates in block context
// or when underlying data changes.
// Still needs a fix regarding _sources scope.
- const _sources = {};
const { sources, canUpdateBlockBindings, bindableAttributes } = useSelect(
( select ) => {
+ const _sources = {};
const { __experimentalBlockBindingsSupportedAttributes } =
select( blockEditorStore ).getSettings();
const _bindableAttributes =
@@ -313,7 +313,11 @@ export const BlockBindingsPanel = ( { name: blockName, metadata } ) => {
return EMPTY_OBJECT;
}
- const registeredSources = getBlockBindingsSources();
+ const { getAllBlockBindingsSources } = unlock(
+ select( blocksStore )
+ );
+ const registeredSources = getAllBlockBindingsSources();
+
Object.entries( registeredSources ).forEach(
( [
sourceName,
Member
There was a problem hiding this comment.
In that case, this is rather a more complex issue related to how the logic works. What I see is that sources is used later in two case:
Object.keys( sources ).lengthcall to check whether the panel should be in read-only mode. That depends on the filterting, but it could get converted into abooleanso maybe not all processing is necessary to decide about this mode.sourcesis also used in the loop when renderingItemGroupin the Attributes panel, so the question is whether all the heavy filtering can be moved there instead?
Contributor
Author
There was a problem hiding this comment.
I think the main problem is that for each individual source, we need to run source.editorUI() (or source.getFieldsList(), respectively), and pass context and select to that function.
I have an idea how to untangle this, though. (In broad strokes, loop over sources, and move the useSelect inside the loop.)
Member
There was a problem hiding this comment.
Hmm, the problem is that it's a different store (blocks), and unlock isn't available across package boundaries AFAICS
Sorry for responding so late, but: unlock should work across package boundaries without any problems. It's typical that an object is lock-ed in package A and unlocked in package B.
The crucial condition is that both places use the same instance of the @wordpress/private-apis package, i.e., the same wp.privateApis global and that the package is not bundled. Locking is implemented as a special field in the locked object whose key is a symbol. The following is not the exact implementation, but it's close:
const __private = Symbol( 'Private API ID' );
function lock( obj, privateData ) {
obj[ __private ] = privateData;
return obj;
}
function unlock( obj ) {
return obj[ __private ];
}The Symbol( 'Private API ID' ) returns a different thing on each call, so it's sensitive to duplication.
|
Flaky tests detected in d85201f. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/18283698099
|
Contributor
Author
|
I spent some time on this today, trying to colocate values computed inside of But even at this most granular level (which would have introduced some other problems -- specifically with determining if a bound attribute should be rendered as read-only because it has no source items that are compatible with the attribute type), the "The This means that the problem is the invocation of those This finding seem to be consistent with this earlier finding (on a different PR that attempted to solve the same problem a year ago). In other words, the solution is to memoize the |
This was referenced Oct 7, 2025
Contributor
Author
|
I thought about this some more. One problem is that the burden of memoization is ultimately on the source -- this was discussed by Jarda and Mario on the other PR. While we can take care of that for the sources that we ship with GB/Core, it's a drawback for extenders who want to include their own sources: They would have to take care of making sure that This would require the extender to use fairly low-level memoization techniques (and probably resort to existing libraries such as This led me to another thought: What if we make IMO, this could be an opportunity for the new API to get rid of a problem that has always plagued |
Member
How would the example look like for the dropdown type from the extender perspective? Well, I'm curious how this would work for Post Meta or Post Data, too. For modal, you have to create a React component anyway, so it might be even better to keep it similar. |
Contributor
Author
Yeah, I found that similarity somewhat appealing, too! In my head, we'd provide a The dropdown itself might not offer a lot of extensibility, but it doesn't right now, either. Do you think it's missing that? We could add additional props later (e.g. function MyBindingsDropdown( { attribute, bindings, blockName } ) {
const handleSelect = ( selectedItem ) => {
/* ... */
}
return <BindingsDropdown
attribute={ attribute }
bindings={ bindings }
blockName={ blockName }
onSelect={ handleSelect }
/>
}Essentially leveraging React's composability principles, I guess. |
Member
|
I'm trying to understand how you envision handling gutenberg/packages/editor/src/bindings/post-meta.js Lines 148 to 165 in 036579b Currently |
Contributor
|
Maybe a "show" or "hide" property. That way you can use it to filter via attribute type (ie: custom fields depending on block selected) |
Contributor
Author
Oh yeah, you're right, I missed that in my earlier comments. I'm currently thinking that the |
ockham
force-pushed
the
fix/block-bindings-use-select
branch
from
d85201f to
ad5e866
Compare
Contributor
Author
|
I've pushed a rough WIP. It demonstrates the principle using the
There are still plenty of rough edges.
I might've missed some things; there are some inline comments to cover them. |
|
Size Change: +135 B (+0.01%) Total Size: 2.04 MB
ℹ️ View Unchanged
|
ockham
force-pushed
the
fix/block-bindings-use-select
branch
from
ad5e866 to
622691f
Compare
Contributor
Author
Pushed a commit which I thought should remove the warning, but I'm still getting it 🤔 |
Contributor
Author
|
ccd64e4 fixed the warning 🎉 |
2 tasks
Contributor
Author
Contributor
Author
|
Closing in favor of #72367. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What?
WIP. See https://github.com/WordPress/gutenberg/pull/71542/files#r2378222245 and #65604.
Why?
We were previously defining an object prior to
useSelectthat was then modified and returned fromuseSelect. Due to referential equality, this could lead to problems where return values are assumed to be identical when they're actually different.How?
Largely by following best practices, see e.g. https://developer.wordpress.org/news/2024/03/how-to-work-effectively-with-the-useselect-hook/#be-careful-about-transforming-data-inside-the-callback.
Testing Instructions
TBD
Screenshots or screencast