Core Data: Implement _fields data reuse for entities
#19498
Conversation
aduth
added
[Type] Performance
aduth
requested review from
ajitbohra,
chrisvanpatten,
mkaz,
nerrad and
youknowriad
as code owners
|
Cool PR ❤️ I haven't had the chance to look at the PR deeply but one thought here is: if we can have a mapping "context => fields" in the data package, we could also potentially trigger the "view" context request instead of "edit" if the fields requested are available there. This would solve issues we have where we can't use the data module in some components/blocks due to failures for the contributor role. |
|
@youknowriad That's a good thought. I'm not sure it's something to be done here, though I've been thinking of a few things as well, where each could potentially have some impact on what an implementation should look like. For example:
|
|
I still think this is very valuable work, though given the current conflicts, it may need a bigger refresh than would be reasonable for a rebase. Similarly, there are some considerations mentioned in the prior two comments which might alter what's considered as an ideal implementation. Going to close this for now. The issue #15114 remains open for tracking the task. |
|
I've restored this branch, I'd like to see how much effort it would take to refresh it. |
youknowriad
force-pushed
the
update/core-data-fields-query
branch
from
40ec2f2 to
ce01c26
Compare
|
Size Change: -38 B (0%) Total Size: 1.16 MB
ℹ️ View Unchanged
|
youknowriad
added
Needs Dev Note
| */ | ||
| function getNormalizedCommaSeparable( value ) { | ||
| if ( typeof value === 'string' ) { | ||
| return value.split( ',' ); |
There was a problem hiding this comment.
Minor question: should we .map( trim ) in case the input looks like 'this, list, with, spaces'?
There was a problem hiding this comment.
I think it depends on whether the REST API supports spaces there or not. (_fields and include query args)
packages/core-data/src/selectors.js
Outdated
| // Queried data state is prepopulated for all known entities. If this is not | ||
| // assigned for the given parameters, then it is known to not exist. Thus, a | ||
| // return value of `undefined` is used instead of `null` (where `null` is | ||
| // otherwise used to represent an unknown state). |
There was a problem hiding this comment.
This sounds like implementation details (in particular, the way that state.entities.data is structured) bleeding into the interface of getEntityRecord, no?
I worry that this reversal of null and undefined is so unconventional that it would create confusion. Have you considered false for "known not to exist"? Or (re-)reversing null and undefined?
There was a problem hiding this comment.
Indeed it supports undefined is better for data that is not fetched yet and null for things we know don't exist. Why can't we switch it?
There was a problem hiding this comment.
I'm not sure I understood. :) Are you suggesting we change to what I said (undef -> unknown, null -> known nonexistence)? That would be good with me.
There was a problem hiding this comment.
Yes, that's my suggestion. At least it seems more logical to me but I don't know why @aduth went the other direction initially.
There was a problem hiding this comment.
Ok, I spent some time looking into this and I think we were misunderstanding the intent.
Returning undefined here doesn't mean the request record is known to not exist but it instead means the Entity doesn't exist (for example if I request a CPT "book" with id 1, it will return undefined if the entity doesn't exist but if the entity exists and the book is not found or not fetched yet, it will still return null)
I also found that we already return null in some of these selectors when requests are in progress or incomplete. So my suggestion would be to avoid the distinction between undefined and null here and always return null.
There was a problem hiding this comment.
Hmm, I see then. In that case it makes to converge on null. 👍 We can always rely on other selectors to inquire on the status of data satisfaction and such.
There was a problem hiding this comment.
I could have sworn that I'd been of the like opinion here that undefined is a good representation of "not yet known" and null of "known to be nothing". I'm not really sure why I'd reversed it here. I'd be agreeable to changing it though.
In general, I'd still agree it's a bit of an abuse of these values to try to signal the tri-state "unknown", "known and nothing", "known and something" via undefined, null, and some truthy value. I'm not sure the language gives us a great alternative here. I think there'd been cases where using some other value like false could be an issue (imagine something like wasSaveSuccessful, where false should mean "the save was not successful" and not "the save is still in progress and the status is not yet known"). Pretty sure there are real cases of this in the current code.
| itemIsComplete: { | ||
| 1: true, | ||
| 2: true, | ||
| }, | ||
| queries: [ 1, 2 ], |
There was a problem hiding this comment.
Not in the diff, but when would queries be an array?
Co-authored-by: Miguel Fonseca <[email protected]>
tellthemachines
removed
the
Needs Dev Note
Closes #15114
Related: #18988, #13618, #18586
This pull request seeks to implement
_fieldsfiltering and data reuse for core data entities. With these changes, thegetEntityRecordandgetEntityRecordsselectors will behave in such a way that_fieldsqueries will only trigger a request to the REST API if there was not already a previous request for either (a) the complete entity record or (b) another_fieldsfiltered request which satisfies (is a superset of) the currently-requested fields.These changes would enable a number of potential optimizations, where currently we do not implement
_fieldsquerying, even when only using a limited subset of the available data. This is due to a previous lack of support of this functionality. One major improvement this may enable is to omit the post'scontent.renderedvalue from the bootstrapped post data, which is unused, potentially substantial, and requires server-side processing (the_contentfiltering) to generate. See also #18988.Testing Instructions:
There should be no expected impact on the runtime of the editor application, since the included changes implement support for
_fieldsfiltering, but it is not yet leveraged anywhere.Verify that there is no change in the expected behavior of common entities use (e.g. post loading, edits, etc).
Ensure unit tests pass:
Testing the added support is most effective using your browser's DevTools console and, optionally, the Redux DevTools browser extension (to verify expected state shape, resolution behaviors).
Example:
In this example, the
nullreturn values indicate where data is not yet available and a network request would have been issued. The subsequent identical selector call represents the data as it is expected to have been returned by the REST API. You can see in the later examples that, while there was no original network request for the aggregate of all of the fields, the available data is such that it can be returned immediately, without needings its own network call.