Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Get the page count as derived data #55316

Merged
merged 1 commit into from
Oct 12, 2023
Merged

Get the page count as derived data #55316

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 0 additions & 6 deletions packages/core-data/src/queried-data/selectors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,9 +124,3 @@ export function getQueriedTotalItems( state, query = {} ) {

return state.queries?.[ context ]?.[ stableKey ]?.meta?.totalItems ?? null;
}

export function getQueriedTotalPages( state, query = {} ) {
const { stableKey, context } = getQueryParts( query );

return state.queries?.[ context ]?.[ stableKey ]?.meta?.totalPages ?? null;
}
3 changes: 0 additions & 3 deletions packages/core-data/src/resolvers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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' )
),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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'] );

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, let's keep it that way for now then, we can always change anytime :)

totalItems: parseInt(
response.headers.get( 'X-WP-Total' )
),
Expand Down
11 changes: 5 additions & 6 deletions packages/core-data/src/selectors.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,7 @@ import deprecated from '@wordpress/deprecated';
* Internal dependencies
*/
import { STORE_NAME } from './name';
import {
getQueriedItems,
getQueriedTotalItems,
getQueriedTotalPages,
} from './queried-data';
import { getQueriedItems, getQueriedTotalItems } from './queried-data';
import { DEFAULT_ENTITY_KEY } from './entities';
import {
getNormalizedCommaSeparable,
Expand Down Expand Up @@ -578,7 +574,10 @@ export const getEntityRecordsTotalPages = (
if ( ! queriedState ) {
return null;
}
return getQueriedTotalPages( queriedState, query );
if ( query.per_page === -1 ) return 1;
const totalItems = getQueriedTotalItems( queriedState, query );
if ( ! totalItems ) return totalItems;
return Math.ceil( totalItems / query.per_page );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

cc @ntsekouras, @youknowriad

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, @ntsekouras!

};

type DirtyEntityRecord = {
Expand Down
Loading