Get the page count as derived data #55316
Merged
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
ntsekouras
added
the
[Type] Bug
ntsekouras
changed the title
youknowriad
reviewed
Oct 12, 2023
| @@ -233,9 +233,6 @@ export const getEntityRecords = | |||
| const response = await apiFetch( { path, parse: false } ); | |||
| records = Object.values( await response.json() ); | |||
| meta = { | |||
| totalPages: parseInt( | |||
| response.headers.get( 'X-WP-TotalPages' ) | |||
| ), | |||
There was a problem hiding this comment.
I wonder if there's a way to store this in the reducer with the "perPage" as a sub key in order to retrieve the right value in the selector instead of computing it.
I wonder which is best
There was a problem hiding this comment.
I'm not super confident that I could make that change.. I checked about the sub keys, but haven't touched that part of the code at all before.
There was a problem hiding this comment.
I can make the change myself, I'm just wondering which is better. Should we rely on the server side value or the computed one.
There was a problem hiding this comment.
It's a simple calculation and it seems core does that too:
$max_pages = ceil( $total_posts / (int) $posts_query->query_vars['posts_per_page'] );
There was a problem hiding this comment.
Ok, let's keep it that way for now then, we can always change anytime :)
|
Size Change: +31 B (0%) Total Size: 1.65 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in f496132. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6498431702
|
Mamaduka
reviewed
Mar 19, 2024
| if ( query.per_page === -1 ) return 1; | ||
| const totalItems = getQueriedTotalItems( queriedState, query ); | ||
| if ( ! totalItems ) return totalItems; | ||
| return Math.ceil( totalItems / query.per_page ); |
There was a problem hiding this comment.
This can result in NaN; the query.per_page can be undefined when the request relies on the default per_page by REST API.
We can try using default 10 as a fallback, but that would be incorrect for some entities that don't support pagination or if the project changes default per_page for endpoints.
I was debugging the useEntityRecords hook triggering unstable references warning (NaN !== NaN), which led me here.
There was a problem hiding this comment.
Good catch! I guess if the per_page is not provided, we can fallback to the header of the response, since it should update properly with other changes of the query. The problem was with the perPage argument not being part of the stable key. I'll open a PR in a bit.
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?
Recently there was an update to core-data package to include the total number of records and pages per query.
There is a bug though in getting the total pages of a query, because internally in state we keep the queries with a stable key, which doesn't include the
perPageargument. This makes sense in practice, but the stored value for total pages has the value of the last request.I kept the same APIs, but I'm calculating the total pages based on the total number of records.
Testing Instructions