Core Data: getEntityRecords return items even if some included IDs don't exist #34034
Conversation
This file contains 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
[Package] Core data
|
Size Change: +42 B (0%) Total Size: 1.03 MB
ℹ️ View Unchanged
|
|
The slight difference I noticed when this fix is applied: The first call to Should we consider this change as a regression? |
|
I think the logic here is that even if some of the items are already in state, it doesn't mean the query is complete, the items might be already in state because of another query. The reason is independent of the |
|
I'm not sure whether we should land this. It might have some big impacts like you raised here #34034 (comment) We could try to make a special case for "include" but we'd need to find a way to know for sure that the request finished I think. |
|
Thanks for the feedback, Riad. I was expecting that fixing the issue won't be this easy. Nevertheless, I will continue my search for the proper fix :)
How can I reproduce this state for testing? Would |
I'm not sure this would do it because the "itemIds" would only be updated once all the requests completes. Looking more at the code, I saw this https://github.com/WordPress/gutenberg/pull/34034/files#diff-6fae435c7327c5edb1a67718054b7ee8513d4691a19b416e7d5f628aefc4459bR41-R48 It seems that removing that part of the code might solve the issue, I wonder why we have this shortcut. (other unintended consequences are possible though) |
|
In other words, why we're considering the "request" as the source of truth for the ids instead of the "response"? (for the includes case) |
|
Now that you mentioned it, I remember adding I think it makes sense to have a single source of truth for |
|
@youknowriad updated |
|
The code looks good here but how to make sure we didn't break anything :P |
youknowriad
approved these changes
Aug 12, 2021
|
I guess merging is also one way to find out 😅 |
|
It took few weeks, but now I know that PR caused a regression. Thanks to #34506. Currently, the issue only affects entity selection with a query parameter. The problem
gutenberg/packages/core-data/src/resolvers.js Lines 113 to 122 in 44d1d32 Failing unit test exampleit( 'should return null if requesting by include but no item IDs', () => {
const state = {
items: {
default: {
1: { id: 1 },
2: { id: 2 },
},
},
itemIsComplete: {
default: {
1: true,
2: true,
},
},
queries: {
default: {
'': [ 1, 2 ],
},
},
};
const result = getQueriedItems( state, { include: [ 3 ] } );
expect( result ).toEqual( null );
} );@youknowriad should I revert this change while I'm looking for a better solution? The only thing I've for now is to add the following check after the loop, but this might affect some other cases. // If we have item IDs but no items, this means that objects haven't been queried.
if ( itemIds && ! items.length ) {
return null;
} |
|
@Mamaduka Based on the unit test above can we make the following assumption: You can revert if you think it's going to take time to get the right fix in. |
|
@youknowriad we're already doing a similar check (assuming you meant gutenberg/packages/core-data/src/queried-data/selectors.js Lines 41 to 47 in faf3e98 In the following scenario, the // The inital request triggered by setting featured image or some other parts of editor.
wp.data.select('core').getMedia( 1, { context: 'view' } );
// Second request by replacing featurd image
// This will never trigger REST API request. The `context` and `stableKey` are the same for both queries.
wp.data.select('core').getMedia( 2, { context: 'view' } ); |
|
It seems weird to me that |
|
I'm no expert in Redux caching, but this kind of makes sense if we only want to look up items by ID to use the general query key. The key is the same ( I think I will revert this "fix" while searching for better alternatives. The impact of this regression is more significant than the original issue I tried to solve here. |
|
For me |
|
Considering that we want the "response" to be the source of truth in case of includes. So making it part of the query key makes sense. Thanks for your feedback, Riad! I'm going to create PR for this update. |
7 tasks
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.
Description
Makes response single source of truth for
itemIds.I couldn't find context for the original logic of aborting the loop and returningnull.The original code was introduced in #19498.Fixes #28429.
How has this been tested?
Make sure unit tests are passing for the core data package:
Run the following code in the browser console:
Replace
includethe existing Tag IDs and one non-existing.Types of changes
Bugfix
Checklist:
*.native.jsfiles for terms that need renaming or removal).