Core data: properly forward entity resolvers #61013
Conversation
ellatrix
added
[Type] Bug
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
ellatrix
added
[Type] Performance
|
Size Change: -398 B (0%) Total Size: 1.74 MB
ℹ️ View Unchanged
|
|
@Mamaduka I don't think that will work, because |
Oh, that's a good point, so this could be seen as a breaking change, maybe there's a way to shim that somehow. |
|
I just noticed that the Inserter > Media > Images tab hangs in a loading state on this branch due to an error in the I believe this was the reason for the e2e test failure in the first run - https://github.com/WordPress/gutenberg/actions/runs/8806701315/attempts/1?pr=61013. |
|
@Mamaduka Yes that's because |
|
Okay, so both meta-selectors and Yeah, this seems like a breaking change, and I'm also wondering if we can shim the previous behavior. |
packages/core-data/src/index.js
Outdated
| result[ getMethodName( kind, name ) ] = createRegistrySelector( | ||
| ( select ) => ( state, key, query ) => | ||
| select( STORE_NAME ).getEntityRecord( kind, name, key, query ) | ||
| ); |
There was a problem hiding this comment.
Here createRegistrySelector is not used to select from another store, but to select from the same store, we just want to call the selectors that are already bound to the store, with all the resolver-calling wrappers, instead of the unbound raw functions.
That's fine, but I think we should eventually come up with some new API for this.
There was a problem hiding this comment.
What do you have in mind? I was thinking of creating something to pass aliases for selector/resolvers when creating the store, and handle them in the store
There was a problem hiding this comment.
I don't have anything specific in mind yet. But the idea behind createRegistrySelector is that you can select across multiple stores and create one return value. But here we're not doing any cross-store work at all, we're still perfectly encapsulated inside one store. All I want to say is that using createRegistrySelector this way feels odd 🙂
There was a problem hiding this comment.
Yep, we do the same thing to trigger resolvers from other normal selectors in the same store. A better solution is needed there too.
|
Some loud thinking about the breaking change: before this PR, the resolver for After this PR, What if we did this: resolvers[ getMethodName( kind, name ) ] = {
fulfill: ( key, query ) => resolvers.getEntityRecord( kind, name, key, query ),
forward: ( args ) => [ 'getEntityRecord', [ kind, name, args.key, args.query ] ],
}The function of metadataSelectors.hasStartedResolution( metadata, selectorName, args )would check if there is a "forwarder" in the map for For example, What do you think? Is it too overengineered? But it works, doesn't it? |
|
@jsnajdr with the change you suggest, there's also no need to use registry selectors if I'm not wrong. |
|
@jsnajdr That's closer to what I meant with creating aliasing. This would also eliminate the duplication in this code. By knowing how to forward, we also know how to fulfill. resolvers[ getMethodName( kind, name ) ] = {
fulfill: ( key, query ) => resolvers.getEntityRecord( kind, name, key, query ),
forward: ( args ) => [ 'getEntityRecord', [ kind, name, args.key, args.query ] ],
} |
Yes, the existing code would be kept without changes, there would still be the Alternatively, we could have a resolvers[ getMethodName( kind, name ) ] = forwardResolver( ( key, query ) => [ 'getEntityRecord', [ kind, name, key, query ] ] );It's not a different solution, just a syntactic sugar over the one from my previous comment. The function forwardResolver( forward ) {
return {
fulfill: ( ...args ) => {
const [ selectorName, selectorArgs ] = forward( ...args );
return resolvers[ selectorName ]( ...selectorArgs );
},
forward,
};
} |
Exactly! The |
|
Note: We already have the |
Interesting! And it seems it has none of the problems because it uses |
Saw that, but that only works if args are the same, no? |
|
Ah, let me try that approach |
|
We could update it to provide extra static args or just use a similar technique to define the resolvers of the aliases without necessarily using the function. |
|
Yeah, trying it |
|
Nice, much simpler and works |
|
Cool :) |
|
Nice! Works like a charm. Here's a simple plugin I've used for manual testing the resolution states. Works better with emulated network. Code
const {createElement: el, useState} = wp.element;
const {useSelect} = wp.data;
const registerPlugin = wp.plugins.registerPlugin;
const PluginDocumentSettingPanel = wp.editPost.PluginDocumentSettingPanel;
const {SelectControl} = wp.components;
function TestDataShortcutResolution() {
const [currentImageId,setCurrentImageId] = useState(0);
const images = useSelect((select)=>select('core').getMediaItems({ _fields: 'id,title' }), []);
const { image, status } = useSelect( ( select ) => {
if ( currentImageId === 0 ) {
return { image: undefined, status: 'idle' };
}
const { getMedia, getResolutionState } = select( 'core' );
console.log( currentImageId );
return {
image: getMedia( currentImageId ),
...( getResolutionState( 'getMedia', [ currentImageId ] ) ?? {} ),
}
}, [ currentImageId ] );
const options = (images ?? []).map(image=>({
value: image.id,
label: image.title.rendered
}))
if ( options.length === 0 ) {
return null;
}
options.push( { value: 0, label: 'No Selection' } );
return el(PluginDocumentSettingPanel, {
className: 'test-data-shortcut-resolution',
title: 'Data Shortcut Resolution',
name: 'test-data-shortcut-resolution'
}, el(SelectControl, {
label: 'Select Image',
options,
value: currentImageId,
help: `Resolution status: ${ status }`,
onChange: (id)=>setCurrentImageId(parseInt(id)),
__next40pxDefaultSize: true,
__nextHasNoMarginBottom: true,
})
);
}
registerPlugin('test-insertion-point', {
render: TestDataShortcutResolution,
}); |
|
Added a comment to explain why it works. Would appreciate a new ✅ with this new approach 🙂 |
|
✅ |
|
Ok, there's still a slight back compat concern: for example See 7e81589. |
|
I guess we could map |
|
So I was playing with this. The problem is that the first action will not work as you can either only invalidate an entire resolution by selector, or with an exact args match? entityActions.invalidateResolutionForStoreSelector =
( selectorName ) =>
async ( { dispatch } ) => {
let isPlural = false;
const config = entitiesConfig.find(
( entity ) =>
getMethodName( entity.kind, entity.name ) === selectorName
);
if ( ! config ) {
const pluralConfig = entitiesConfig.find(
( entity ) =>
getMethodName( entity.kind, entity.plural ) === selectorName
);
if ( pluralConfig ) {
isPlural = true;
}
}
dispatch( {
type: 'INVALIDATE_RESOLUTION_FOR_STORE_SELECTOR',
selectorName: isPlural ? 'getEntityRecords' : 'getEntityRecord',
args: [ config.kind, config.name ],
} );
};
entityActions.invalidateResolution =
( selectorName, args ) =>
async ( { dispatch } ) => {
let isPlural = false;
const config = entitiesConfig.find(
( entity ) =>
getMethodName( entity.kind, entity.name ) === selectorName
);
if ( ! config ) {
const pluralConfig = entitiesConfig.find(
( entity ) =>
getMethodName( entity.kind, entity.plural ) === selectorName
);
if ( pluralConfig ) {
isPlural = true;
}
}
dispatch( {
type: 'INVALIDATE_RESOLUTION',
selectorName: isPlural ? 'getEntityRecords' : 'getEntityRecord',
args: [ config.kind, config.name, ...args ],
} );
}; |
|
It should be possible to define the In fact, we should probably do the same for the existing forwardResolver helper. |
|
I've tried that above, and mentioned there: I don't think it's possible to invalidate with partial arguments? |
|
@ellatrix I'm not sure I understand the code above, what is |
|
What I'm saying is that you should do: In other words when invalidating |
It's not currently possible, but should be implementable by extending the invalidateResolution( 'getEntityRecord', [ 'postType', 'post', match.anything() ] );or even invalidateResolution( 'getEntityRecord', [ 'postType', 'post', match( value => value.startsWith( 'prefix:' ) ) ] );Very similar to Jest matchers 🙂 However, seeing the troubles with With the Now let's call Property implemented forwarding would create a real alias, where all operations on |
What?
Currently the dynamically entity resolver "shortcuts" are not really properly forwarded: is the
corepackage these will be seen as separate resolvers altogether with and will be cached separately.Why?
For example:
getSite()andgetEntityRecord( 'root', 'site' )should be equivalent, but they will each trigger a separate REST request.How?
We should avoid creating new resolvers for these shortcuts and instead rely on
createRegistrySelector, so that the underlying resolver is called instead of a wrapper resolver.Edit: it seems like this breaks
resolveSelect: when trying to resolve a normal selector, it will just return the initial value without awaiting the result. Not sure how we can fix that. Maybe declare aliases and let thedatapackage handle aliasing instead?Testing Instructions
Load a page in the site editor, and you'll see two GET requests to the
/v2/settingsendpoint in the network tab. With this PR, there should only be one.Testing Instructions for Keyboard
Screenshots or screencast