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?

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.

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?

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.

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.

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.

I don't think we can return constant objects from getFieldsList because 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 getFieldsList but 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.

Okay, let me try to share more context on this specific use case:

• We are calling getFieldsList at 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 select and use it however they want, but they can't use hooks because getFieldsList is called inside conditionals and hooks. For example, post meta source calls getEditedEntityRecord here.
• 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.

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.

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.

Are changes in this file still needed after #67523? The PR might require a refresh anyway.

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.