Fix entity cache misses for single posts due to string as recordKey #52338
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
getdave
added
[Type] Bug
getdave
commented
Jul 5, 2023
Comment on lines
1
to
4
| const POST_TYPES_THAT_USE_STRING_BASED_IDS = [ | ||
| 'wp_template', | ||
| 'wp_template_part', | ||
| ]; |
There was a problem hiding this comment.
Perhaps this information could come from entities config? It feels like we should have one single source of truth for this information rather than have it stuff away in a const within a utility.
There was a problem hiding this comment.
This is interesting. Could use use Number.isNaN(Number(postId)) on any ID and if it's a number we convert if not we dont? So that we don't need to have this array of types that use string ids?
There was a problem hiding this comment.
I updated this to use the utility from #52120.
getdave
commented
Jul 5, 2023
Comment on lines
+27
to
+28
| // See https://github.com/WordPress/gutenberg/pull/52120. | ||
| const postId = Number( params?.postId ); |
There was a problem hiding this comment.
As this is definitely Navigation posts we know it's a number.
getdave
commented
Jul 5, 2023
| } = useNavigator(); | ||
| const { params } = useNavigator(); | ||
|
|
||
| const postId = Number( params?.postId ); |
There was a problem hiding this comment.
As this is Page entities we know it's a number.
|
Size Change: +2.34 kB (0%) Total Size: 1.42 MB
ℹ️ View Unchanged
|
|
@draganescu As you are not in favour of the solution in #52120 it would be good to land this PR to avoid all current instances of unwanted network requests in Gutenberg. In the future we should follow up here and make some way for the router to register the types of the params so they are always consistent. |
getdave
commented
Jul 6, 2023
| const { setCanvasMode } = unlock( useDispatch( editSiteStore ) ); | ||
|
|
||
| const { params } = useNavigator(); | ||
| const { postType } = params; | ||
| const postId = normalizePostIdForPostType( params?.postId, postType ); |
There was a problem hiding this comment.
Patterns can be wp_block or wp_template_part which use different types for recordKey so we need to selectively normalize.
|
I think this is good. I only have one question about having to maintain that array of entities that have string ids. We could come up with a way to always convert to number if the id looks like a number even if it comes as a string. Should we? |
|
We could mirror what I'm doing in #52120 and have a util which checks for numericIDs and if so coerces to |
|
Flaky tests detected in 7fe8755. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5486353555
|
|
I still see a request to |
No. It appears to be because calling |
|
Should we fix that in a different PR? |
|
I have a suspicion it's ending up calling |
scruffian
approved these changes
Jul 10, 2023
|
Before we merge this let's consider whether we can land #52120 (comment). |
|
I think we should land both. |
westonruter
added a commit
that referenced
this pull request
Jul 12, 2023
…dd/defer-script-loading-strategy * 'trunk' of https://github.com/WordPress/gutenberg: Update Changelog for 16.2.0 Adding support for defined IDs in `TextControl` component (#52028) Bump plugin version to 16.2.0 Revert "Bump plugin version to 16.2.0" Bump plugin version to 16.2.0 Add maxLength to LinkControl search items (#52523) [RNMobile] Update Editor block inserter button styles and default text input placeholder/selection styles (#52269) Site Editor: Reset device preview type when exiting the editing mode (#52566) Trim footnote anchors from excerpts (#52518) Add back old Navigation and File blocks JavaScript implementation when Gutenberg is not installed (#52553) Block Editor: Ensure synced patterns are accounted for in 'getAllowedBlocks' (#52546) Fix md5 class messed up with new block key (#52557) Fix entity cache misses for single posts due to string as recordKey (#52338) Make "My patterns" permanently visible (#52531) Hide site hub when resizing frame upwards to avoid overlap (#52180) Fix "Manage all patterns" link appearance (#52532) Update navigation menu title size & weight in detail panels (#52477) Site Editor Patterns: Ensure sidebar does not shrink when long pattern titles are used (#52547) Site Editor: Restore quick inserter 'Browse all' button (#52529) Patterns: update the title of Pattern block in the block inspector card (#52010)
This was referenced Aug 1, 2023
getdave
added a commit
that referenced
this pull request
Aug 8, 2023
scruffian
pushed a commit
that referenced
this pull request
Aug 8, 2023
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?
Fixes instances where entity data for a single entity is re-requested from network even though it already exists in cache.
Related to #52120.
Why?
Because if the data is already in state then we shouldn't fetch from network unless the query changes.
This fixes additional requests for:
wp_blockpost type).It also preserves existing functionality for entities which do use strings for the
recordKeywhich istemplate_partandtemplate.How?
When
recordKeyargument ofgetEntityRecordis intended to be a numeric post ID (e.g.123) ensure it as passed as a number and not a string.Why? Because passing as a string will cause the resolve cache to "miss" (see fix in #52120) and thus retrigger the network request for the entity.
The root fix for this is #52120 but it helps to encourage use of the correct pattern in Core.
Testing Instructions
wp.data.select('core').getEntityRecords('postType', 'wp_navigation')- alter query as needed per post type.Note: that whilst #52120 is not merged, you can try this on
trunkto see that requests are dispatched without this PR.Testing Instructions for Keyboard
Screenshots or screencast