Core Data: Fix early return check for the record field-level resolutions #71541
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Mamaduka
added
[Type] Bug
|
Size Change: +73 B (0%) Total Size: 1.94 MB
ℹ️ View Unchanged
|
Mamaduka
commented
Sep 8, 2025
Comment on lines
115
to
126
| it( 'does not make a request if the record is already in store', async () => { | ||
| const select = { | ||
| hasEntityRecord: jest.fn( () => true ), | ||
| }; | ||
|
|
||
| await getEntityRecord( 'root', 'postType', 'post', { | ||
| context: 'edit', | ||
| _fields: 'id,title,slug', | ||
| } )( { dispatch, select, registry, resolveSelect } ); | ||
|
|
||
| // No request should have been made. | ||
| expect( triggerFetch ).not.toHaveBeenCalled(); |
Member
Author
There was a problem hiding this comment.
This looks like a better candidate for integration tests from #71474.
The current test gives us a false sense of safety; even if the hasEntityRecord behavior changes, the test will still pass.
Mamaduka
requested review from
ajitbohra,
fabiankaegy,
juanmaguitar,
ndiego and
nerrad
as code owners
|
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. |
|
Flaky tests detected in 611c852. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/17548578611
|
Mamaduka
force-pushed
the
fix/core-data-get-entity-record-field-resolution
branch
from
611c852 to
1294c4a
Compare
This was referenced Sep 8, 2025
priethor
reviewed
Sep 10, 2025
Contributor
There was a problem hiding this comment.
I was looking forward to this 🚀
I've taken some time to properly understand it and test it, and it looks good to me. Probably the biggest advice is to document a bit more when to use hasEntityRecord vs hasEntityRecords and how their lookup logic differs, because with the field-level resolutions, it is not just "the same but for collections" as the name suggests.
| return !! queriedState.itemIsComplete[ context ]?.[ key ]; | ||
| } | ||
|
|
||
| const item = queriedState.items[ context ]?.[ key ]; |
Contributor
There was a problem hiding this comment.
Thinking out loud: if we are checking for an item with specific fields and they are all cached, is it relevant that the context is not the same if we have the data?
Member
Author
There was a problem hiding this comment.
The data is stored by context, so looking it up by context is important. When there's no data for the same context, we'll have to make a request.
Member
Author
|
Thanks, @priethor! I'll try to document the differences for these selectors. I think Cc @aduth, I would appreciate your feedback here as well, if you've time. |
Member
Does this mean it's still an issue if someone were to use those selectors with a query including |
aduth
approved these changes
Sep 10, 2025
Member
aduth
left a comment
aduth
left a comment
There was a problem hiding this comment.
This makes sense to me 👍
I'd be curious if there's opportunities for more code reuse between query field resolution of the "records" vs. "record" flows, but perhaps it's appropriate that this be simpler in how it's evaluating existence of fields within a single item (vs. building up items matching in an array).
Comment on lines
+464
to
+474
| for ( let i = 0; i < fields.length; i++ ) { | ||
| const path = fields[ i ].split( '.' ); | ||
| let value = item; | ||
| for ( let p = 0; p < path.length; p++ ) { | ||
| const part = path[ p ]; | ||
| if ( ! value || ! Object.hasOwn( value, part ) ) { | ||
| return false; | ||
| } | ||
| value = value[ part ]; | ||
| } | ||
| } |
Member
There was a problem hiding this comment.
As a micro-optimization, I was curious if for ... of would be better than iterating index and assigning a variable, but alas the simple for loop continues to prevail (as of Node v22.16.0)
for loop x 12,295,933 ops/sec ±0.46% (99 runs sampled)
for...of x 12,141,949 ops/sec ±0.59% (97 runs sampled)
Benchmarking code
import Benchmark from "benchmark";
const fields = ["a", "b.c", "d"];
const item = { a: 1, b: { c: 2 }, e: 3 };
const suite = new Benchmark.Suite();
suite.add("before", () => {
for (let i = 0; i < fields.length; i++) {
const path = fields[i].split(".");
let value = item;
for (let p = 0; p < path.length; p++) {
const part = path[p];
if (!value || !Object.hasOwn(value, part)) {
return false;
}
value = value[part];
}
}
});
suite.add("after", () => {
for (const field of fields) {
const path = field.split(".");
let value = item;
for (const part of path) {
if (!value || !Object.hasOwn(value, part)) {
return false;
}
value = value[part];
}
}
});
suite.on("cycle", (event) => {
console.log(String(event.target));
});
suite.run({ async: true });
Member
Author
There was a problem hiding this comment.
Thanks for checking. That's also my experience that simple for loops perform better for now.
Member
Author
I've not seen any related issues with The |
priethor
approved these changes
Sep 12, 2025
Contributor
priethor
left a comment
priethor
left a comment
There was a problem hiding this comment.
As we talked over Slack, my note about the docs should not be a blocker
Mamaduka
deleted the
fix/core-data-get-entity-record-field-resolution
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Part of #26629.
PR fixes the early return check for the record field-level resolutions, inside the
getEntityRecord. This restores optimization logic and avoids additional HTTP requests when entity record data already exists in the store.Why?
It looks like the
hasEntityRecordsselector has regressed sometime ago, probably when we had to addincludeto thestableCacheKeyto avoid incorrect results.How?
PR introduces a new
hasEntityRecordselector, which is now used to perform checks.Testing Instructions
Test Snippets.
Testing Instructions for Keyboard
Same.