[TRY] Core data: extend stable key to use per_page and page query parts #56354
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
… which should trigger new API calls, can be added to the state: per_page, page.
ramonjd
added
[Package] Core data
|
Size Change: +22 B (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
ramonjd
commented
Nov 21, 2023
| @@ -4,14 +4,14 @@ | |||
| import { getQueryParts } from '../get-query-parts'; | |||
|
|
|||
| describe( 'getQueryParts', () => { | |||
| it( 'parses out pagination data', () => { | |||
| it( 'parses out pagination data and adds to returned object', () => { | |||
There was a problem hiding this comment.
Just updating these tests due to OCD.
Given they they're checking for empty stableKeys here, I'm not very sure about this approach.
youknowriad
reviewed
Nov 21, 2023
|
I recently encountered this issue in a different use case. There's a lot of complexity there, but I think what we do is handle the What this means is if we have a query with From the issue description, it's a valid issue, though if we use the selector with a subscription to the store, the updated value is used later. So I'm not sure yet how we should proceed, but if we want to add |
|
What happens when you have both a page value other than 1 and a per page. In this case, using the stale is clearly a bug because on the second page, items won't start at the same position |
|
TL;DR This PR disrupts existing pagination caching. I'll close this and move the discussion to the issue.
Thanks for the helpful context. Yeah, there's a lot going on 😄 As @ntsekouras points out, I've tried to illustrate this over in a code sandbox: https://codesandbox.io/p/sandbox/cool-sunset-gyfxgy Core data maintains this state in the queries reducer under an empty key 
I don't think this PR is the right way forward as it kinda breaks all this - it's adding Example queries on this PR: // Fetches the first 3 records from API.
await wp.data.select( 'core' ).getEntityRecords( 'postType', 'post', { per_page: 3 } );
// Fetches the first 3 records from state (cached).
await wp.data.select( 'core' ).getEntityRecords( 'postType', 'post', { per_page: 3 } )
// Fetches the first 2 records from API, but returns null << we've already got these so null is wrong
await wp.data.select( 'core' ).getEntityRecords( 'postType', 'post', { per_page: 2 } )
On trunk, I'm still seeing an API call even when we've cached results, is that right? // Fetches the first 5 records from API.
await wp.data.select( 'core' ).getEntityRecords( 'postType', 'post', { per_page: 5 } );
// Fetches the first 5 records from state (cached).
await wp.data.select( 'core' ).getEntityRecords( 'postType', 'post', { per_page: 5 } )
// Fetches the first 2 records from API, AND fetches the first 2 records from API << we've already got these and
// the cache hasn't been invalidated, why make an API call?
await wp.data.select( 'core' ).getEntityRecords( 'postType', 'post', { per_page: 2 } )So yeah, maybe something is going on with the way we're checking the cache for pagination. 🤔 I'd expect something along the lines of the following:
|
|
I'll keep thinking about this 🤔 |
|
I don't have anything else to add, but great discussion here folks, and thanks for digging into the issue @ramonjd! While this PR wasn't the right one, it's helped hone in on a desired path forwards that can re-use cached items 👍 |
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.
Maybe resolves #56355
Follow up to #54046
❗ I'm not sure this is the right way forward. There's a test that explicitly checks that stableKey does not contain any pagination data. I'm not sure why.
What?
This is a test to check whether extending the stable key with params, which should trigger new API calls, can be added to the state:
per_page.Why?
How?
Adding an entry to the
queriesreducer every time theper_pagequery part changesTesting Instructions
Does this break anything?
Testing Instructions for Keyboard
Screenshots or screencast