Data Store: invalidateResolution not working

[swissspidy]

Description

In my project I wanted to invalidate the resolution for the following getMedia call in the image block:

https://github.com/WordPress/gutenberg/blob/30258b51228b78f8973c32a86148d7c66e470b7d/packages/block-library/src/image/image.js#L141-L147

I tried things like invalidateResolution( 'getMedia', [ id, { context: 'view' } ] ) or invalidateResolutionForStoreSelector( 'getMedia' ) or even wp.data.dispatch( 'core' ).invalidateResolutionForStore() to no avail. No new HTTP request happens when I then call getMedia again.

There is always still something cached.

Step-by-step reproduction instructions

For this case it seems to work:

wp.data.select( 'core').getMedia( id ); // Triggers a request with context=edit
wp.data.dispatch( 'core' ).invalidateResolutionForStoreSelector( 'getMedia' );
wp.data.select( 'core').getMedia( id ) // Triggers a new request with context=edit

But this doesn't:

wp.data.select( 'core').getMedia( id, { context: 'view' } ); // Triggers a request with context=view
wp.data.dispatch( 'core' ).invalidateResolutionForStoreSelector( 'getMedia' );
wp.data.select( 'core').getMedia( id, { context: 'view' } ) // Nothing happens

Even after invalidateResolutionForStore(), which I thought would invalidate all selectors, none of my selectors trigger a new fetch. wp.data.select( 'core' ).getCachedResolvers() still shows all cached data, but I suppose that's expected (stale while revalidate)

Screenshots, screen recording, code snippet

No response

Environment info

• WordPress trunk (6.7)
• Gutenberg trunk (19.2 alpha or so)

Please confirm that you have searched existing issues in the repo.

• [X] Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

• [X] Yes

[youknowriad]

While I'm not sure about the exact reason that the invalidation calls are not working, I'm wondering if this code is meant to be included in Gutenberg or is for third-party code. If it's the former, a better approach to invalidation is a declarative one by adding a shouldInvalidate definition to the getEntityRecord resolver and taking into consideration the action that should invalidate it (for instance uploading a new image or something like that)

[swissspidy]

I was actually looking for such a shouldInvalidate flag and I think would come in handy either way. But a solution for third-party code would be great too :)

[Mamaduka]

The createResolversCacheMiddleware uses invalidateResolution under the hood, and the same bug will affect the shouldInvalidate method.

[youknowriad]

I think the issue in your code is that the resolver that you should invalidate is getEntityRecord, getMedia is just a shortcut. (confidence of my answer: 90% :P )

[Mamaduka]

I think the issue in your code is that the resolver that you should invalidate is getEntityRecord.

The same issue affects getEntityRecord.

// Replace `id` with valid media ID.
wp.data.select( 'core').getEntityRecord( 'root', 'media', id, { context: 'view' } );
wp.data.dispatch( 'core').invalidateResolution( 'getEntityRecord', [ 'root', 'media', id, { context: 'view' } ] );
wp.data.select( 'core').getEntityRecord( 'root', 'media', id, { context: 'view' } );

[youknowriad]

🤔 Did you try putting const query = { context: 'view' } and reusing the same object instance for that ?

[Mamaduka]

I did and got the same buggy results. After invalidation, the getResolutionState returns the correct state, but the HTTP request isn't made on the second call. This is same with and without using query const.

wp.data.select( 'core').getResolutionState(  'getMedia', [ id, { context: 'view' } ] );

[jsnajdr]

The questionable behavior is caused by this part of the getEntityRecord resolver:

https://github.com/WordPress/gutenberg/blob/trunk/packages/core-data/src/resolvers.js#L158-L172

If there is a query, and the store has data for the query, even if we don’t want to use them (we invalided the resolution in order to force re-fetch!), then the resolver will return early without issuing the REST request. That’s a bug. Without a query, ther REST request is issued as expected.

Let's figure out why this early return is there.

[swissspidy]

Nice find!

A git blame shows it was introduced in bfbcc92a207dd6813967bf1eff667f0a1e1bcc56 / #19498

[jsnajdr]

A git blame shows it was introduced in #19498

That's a feature that ensures that a request is not triggered for ?_fields=foo if you already have a response for ?_fields=foo,bar in the state. But it doesn't work well with resolver invalidation.

I suspect that it won't work with shouldInvalidate either. Selector is invalidated, and then the resolver validates is again without really re-fetching.

The "right" solution would be that: 1) the resolvels cache is smart about (i.e., aware of) the _fields query param, just like it is smart about selector parameters in general. 2) the selector is also aware of _fields, returning state for _fields=foo,bar if it already has it and the request is for _fields=foo.

[Mamaduka]

It looks like the getEntityRecord optimization for _fields has also regressed; it now triggers requests for fields already available in store. I think I introduced that regression when fixing a different bug, and it wasn't caught by tests (#34583).

Calling selector in this order should only make one request, but it makes three:

// Fetch all fields.
wp.data.select( 'core').getMedia( id );
// Fetch only couple.
wp.data.select( 'core').getMedia( id, { _fields: [ 'author', 'title' ] } );
// Fetch only one.
wp.data.select( 'core').getMedia( id, { _fields: [ 'author' ] } );

P.S. I think we need integration tests for selectors like getEntityRecord and getEntityRecords. The current unit tests are good at testing selector/resolver logic separately but have failed to spot similar regressions.

[Mamaduka]

A temp solution to unblock @swissspidy's "media experiments" work would be to fix the condition in the getEntityRecord resolver - query !== undefined && '_fields' in query.

A long-term solution:

• Add integration tests for getEntityRecord and getEntityRecords.
• Make resolver cache logic _fields aware.
• Fix regression and avoid making additional requests when the entity field is already available in store.

[jsnajdr]

I think we need integration tests for selectors like getEntityRecord and getEntityRecords.

Yes. They don't need to be integration tests like the ones in test/integration, just normal unit tests "done right". The test needs to create a registry with stores and test all actions, resolvers and selectors together, while they are doing a realistic job. The network requests can be mocked with nock or a similar package.