Resolve per-entity resolvers after receiving a list of records #26575
Conversation
adamziel
requested review from
youknowriad,
mcsf,
noisysocks,
draganescu and
gziolo
adamziel
added
[Package] Core data
|
Size Change: +511 B (0%) Total Size: 1.21 MB
ℹ️ View Unchanged
|
packages/core-data/src/resolvers.js
Outdated
| @@ -173,6 +173,13 @@ export function* getEntityRecords( kind, name, query = {} ) { | |||
| } | |||
|
|
|||
| yield receiveEntityRecords( kind, name, records, query ); | |||
| for ( const record of records ) { | |||
| yield { | |||
| type: 'FINISH_RESOLUTION', | |||
There was a problem hiding this comment.
These are meant to be low-level actions triggered by the framework itself, not sure we should be doing this here. Can we instead bail early in getEntityRecord if the record is available?
There was a problem hiding this comment.
@youknowriad this was my first hunch too, but then I realized that short-circuiting when the record is available will make it impossible to invalidate that record. The resolver will simply keep returning the cached value.
There was a problem hiding this comment.
how is that wrong or any different from the solution here?
There was a problem hiding this comment.
The more I think about it, the more I believe that receiveEntityRecords should somehow mark the records as resolved conditional on invalidateCache == false? This proposed change is somehow indirect. It works because we don't have many places where records are received, but we'd have to repeat it each time we do receiveEntityRecords. Conceptually, receiving a record means it no longer needs to be resolved, right? Maybe the resolvers part of the equation could somehow listen to RECEIVE_ITEMS?
There was a problem hiding this comment.
If you shortcircuit like I suggested above, the resolver will get called once, the bail early happens and the resolution is marked as "true" as well because the resolver ends. And we'd do so without any new API or low-level action. What's wrong with that?
There was a problem hiding this comment.
Indeed, you're right. I think we can try it in that case; I don't like the usage of low-level things not in the way they were meant to be used initially but it seems like the only way for now.
There was a problem hiding this comment.
That said, I think the current fix is not 100% accurate, it should only happen if we fetched all the fields (no _fields in the query)
There was a problem hiding this comment.
Good point! Hm but then if we use a limited set of fields in both getEntityRecords() and getEntityRecord(), it would still make sense to return that partial record. Which means the resolution is not just an entity-level concept but also/instead a field-level concept. That smells like a large refactor. Let's take a note and for now stick to the easy option of checking if there's no _fields in the query.
There was a problem hiding this comment.
Fortunately _fields is only about a subset of fields according to https://developer.wordpress.org/rest-api/using-the-rest-api/global-parameters/#_fields . It would get even more interesting if it would be possible to request additional fields that are not present by default.
There was a problem hiding this comment.
I created an issue here #26629
|
@youknowriad I added the check you suggested as well as another action, unit and integration tests, and some comments. I explored using a middleware, but I don't think we support middleware of one store dispatching actions on another store. This is as good as it gets for now. |
|
Why do we need the "start" action? |
|
@youknowriad I think it could be safely removed. I added it purely for consistency so that we don’t have any „finish” action without a corresponding „start” action. Maybe it would prevent folks from noticing that behavior in devtools and „debugging”. |
adamziel
deleted the
update/resolve-specific-entities-from-collections
branch
Description
The problem: calling
saveEntityRecord()triggers a number of per-entity GET requests:The cause is described in #26325:
This PR proposes resolving all per-entity resolvers after receiving the list of records.
How has this been tested?
Types of changes
Bug fix (non-breaking change which fixes an issue)
Checklist: