Block Bindings: Keep bindings editor calls in their own logic useSelect
#65604
Conversation
SantosGuillamot
added
[Type] Code Quality
| * whenever there are updates in block context. | ||
| * `source.getFieldsList` may also call a selector via `registry.select`. | ||
| */ | ||
| const fieldsList = useSelect( () => { |
There was a problem hiding this comment.
As suggested here, I moved by @jsnajdr here, I return the fieldsList as the top-level object in the useSelect result.
However, this triggers the useSelect warning. After triaging it a bit, I believe it is caused because fieldsList is an object of objects, and when we check isShallowEqual here, it returns false. I guess that is expected? Should we keep the variable outside or should this be fixed in another way?
There was a problem hiding this comment.
Oh, that's because the values in fieldsList, the results of getFieldsList calls, are also not memoized 🙁 Every time getFieldsList is called, it returns a new value. I wonder if anything can be done about that?
Memoization and immutability is sometimes very hard to achieve, it's a weakness of Redux-like state management.
There was a problem hiding this comment.
I'm still trying to understand better how memoization works 😄 Let me ask some questions to see how I can proceed.
I was testing different scenarios to understand it better. Removing the getFieldsList calls to avoid interferences, I added a useSelect returning some objects.
const fieldsList = useSelect( () => {
// This triggers the warning.
return {
test: {
subprop: 'subprop',
},
};
// This DOES NOT trigger the warning.
return { test: 'prop' };
}, [] );Is this expected? Does this mean we have to memoized the value we are returning independently of what getFieldsList does?
There was a problem hiding this comment.
I was triaging this a bit more and in my testing if the useSelect returns an object of objects, the warning is always there.
I'm not saying this is the right solution, but for testing, I checked that running here fastDeepEqual instead of isShallowEqual doesn't trigger the warning. But I assume we are using isShallowEqual for a good reason?
@jsnajdr Any idea how this could be solved? I tried creating a ref with useRef or memorizing the fieldsList, but it doesn't seem to work.
There was a problem hiding this comment.
The principle of function memoization is that when a function has the same inputs, it should return the same output. In a sense that the old value is === to the other. This function is not memoized:
function getFieldsList() {
return { test: 'prop' };
}Although it always returns the same thing, it actually doesn't return the same thing: it's a different object on each call. If such a return value is passed as an useEffect dependency, or as a prop, or as context value, it tells React that something has changed (not === to the previous) value while in fact nothing has changed. That's bad, because it triggers a lot of extra work and can also lead to bugs.
That's why we need to invest a lot of care into memoizing return values and why sometimes library code issues warnings when it detects problems. JavaScript unfortunately doesn't do it very well natively.
The return value of useSelect is a "container of results". It's not treated as a "result" itself. That's why it can be a different object on each call. Because it's not supposed to be used as "result" itself, i.e., passed as a prop etc. Only it's members are "results".
Returning { test: 'prop' } is OK because the real result is the 'prop' string and it's always === to any other 'prop' string. It's different with objects and arrays. In { test: { subprop: 'subprop' } } the result is the { subprop: 'subprop' } object. It's a different object on each call and that's why useSelect complains.
Ideally the getFieldsList functions should return constant objects, or, if the result is dynamic and depends on something, use some cache. There are many utils that help with that, like memize.
There was a problem hiding this comment.
Thanks a lot for the detailed explanation, it is really useful
I don't think we can return constant objects from getFieldsList because they might depend on the store. For example, in case of post meta, we are calling getEditedEntityRecord. And ideally, it should reflect the changes made in the client. Other sources could have different use cases in the future.
I've tried to cache getFieldsList but I encountered some issues where the component isn't rerendered when the store changes, so I need to investigate it further.
There was a problem hiding this comment.
I don't think we can return constant objects from
getFieldsListbecause they might depend on the store.
In that case you should create a new constant object when the relevant store information changes. Memoization APIs usually have array of dependencies as arguments:
createSelector(
( state ) => { // calculate the result
return [ getA( state ), getB( state ) ];
},
( state ) => [ getA( state ), getB( state ) ], // state the array of dependencies
)or
useMemo( () => {
return [ a, b ];
}, [ a, b ] );the point is that the first argument, the main function, is called only when dependencies change. Otherwise the cached value is returned.
In your case getEditedEntityRecord would be in dependencies in one form or another.
I've tried to cache
getFieldsListbut I encountered some issues where the component isn't rerendered
What code are you testing with? In the Gutenberg repo itself I don't see any code that would actually create bindings, there is only the framework to provide this capability.
There was a problem hiding this comment.
Okay, let me try to share more context on this specific use case:
- We are calling
getFieldsListat a "framework" level here. We iterate through the registered sources and get a list of the ones that have defined fields. - Each source get the
selectand use it however they want, but they can't use hooks becausegetFieldsListis called inside conditionals and hooks. For example, post meta source callsgetEditedEntityRecordhere. - This means that when we call
getFieldsList, we don't know the dependencies.
For those reasons I'm finding it hard to know how to memoized this. Maybe it is an issue with how the framework is built. I don't know.
The only way I am able to avoid the warning while keeping the component reactive is by using useRef and checking against ref.current with fastDeepEqual. But that looks like a workaround, not a solution to the problem.
There was a problem hiding this comment.
To do things really 100% correctly, the getPostMetaFields function should be memoized. It depends on the meta fields registered for the post type and also on the meta field of a specific post. It can change only when one of these dependencies change. The code should look like:
const getPostMetaFields = createSelector(
( registry, context ) => {
// calculate the fields
},
( registry, context ) => [ // calculate the dependencies
registry.select.getEditedEntityRecord( context.postType, context.postId ).meta,
registry.select.getRegisteredPostMeta( context.postType ),
]
);I'm not sure if the createSelector implementation in @wordpress/data can really be used here. There are subtle details: the cache key for memoization includes not only the dependencies, but also the context argument, but if we call the getFieldsList function like:
getFieldsList( registry, { postType, postId } )then the context is always a different object, it's only its fields that are stable and should really be keys.
This is a very complex problem, memoization is very hard to do right.
There was a problem hiding this comment.
Thanks again for the detailed explanation 🙂
I've been testing createSelector in this commit: e9e00d4. It seems to work fine. It reacts to the changes in the store, it doesn't trigger the warning of too many rerenders, and it solves some issues we were having before.
My main concern is that these APIs are supposed to be used by extenders (hopefully getFieldsList won't be part of WordPress 6.7), and asking them to use createSelector feels a bit weird to me. Is this something we could abstract somehow? Any feedback in this regard?
Apart from that, as you shared, I had to create a context constant outside of the useSelect because it was interpreting it was a different object each time. Not sure if there is a better way to handle that.
|
Size Change: +954 B (+0.05%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
SantosGuillamot
added
[Type] Bug
SantosGuillamot
force-pushed
the
try/move-bindings-logic-outside-use-select
branch
from
9fef8fb to
d627aa5
Compare
SantosGuillamot
force-pushed
the
try/move-bindings-logic-outside-use-select
branch
from
d627aa5 to
8b494c6
Compare
This looks concerning for the type metric based on my learning on: |
| [ props.attributes.metadata?.bindings, name ] | ||
| ); | ||
| ); | ||
| const _updatedContext = {}; |
There was a problem hiding this comment.
Are changes in this file still needed after #67523? The PR might require a refresh anyway.
There was a problem hiding this comment.
You're right. I think they are not needed anymore. I've started exploring how it could look like here. I didn't want to add more noise here, but I can do a proper rebase once it is clear the path forward.
What?
Follow-up to these comments where we faced some issues with
useSelectwarning:canUserEditValue#65599 (comment)In that part of the code, we are creating an object outside the
useSelectto avoid the warning:However, that seems like a workaround and I'm opening this to exploring the different alternatives.
Why?
That seems like a workaround and I'm opening this to exploring the different alternatives.
How?
getFieldsListto its ownuseSelectas suggested here. However, that doesn't solve the issue with the warning. After triaging it a bit more I believe the warning is caused because fieldsList is an object of objects. And when we callisShallowEqualhere, it returnsfalsebecause it doesn't check nested objects.useMemo, which feels better.Testing Instructions