We should have done this a long time ago that said, it can be seen as a breaking change.

I know at some point @ellatrix wanted to use this loophole/bug to "auto-migrate" templates to the new "activate template" mode. I wonder if it's still the case or not.

I tried to limit the breaking change effects. If perPage is defined it will be respected when slicing the records.

I don’t have info about auto-migration plan, but using similar bugs as loopholes doesn’t seem reliable.

Why don't we check supportsPagination before slicing? Tbh, I've always thought per_page: -1 is wrong and we should use that property to define "unbound" requests. Even if you use per_page: -1 on pages that support pagination, it can be valuable to be able to set by how many pages it batches requests. per_page: -1 suggests it grabs all items in a single request, and that's simply not true.

I think this is the closest we can do before slicing, right before action is dispatched to the reducer.

Tbh, I've always thought per_page: -1 is wrong and we should use that property to define "unbound" requests.

I think this was just copied from WP_Query, but I agree that, depending on the data, it might take more than a couple of requests. We could try to improve this, but I don't think it's in the scope of this PR.

This is how I understand the problem, correct me if I'm wrong:

• the /wp/v2/icons endpoint returns an array of 80+ icons, with no pagination: ignores the per_page and page query args, there are no x-wp-total and x-wp-totalpages headers in the response.
• the getEntityRecords( 'root', 'icon' ) selector will, however, return only first 10 items, because getQueriedItems and getQueryParts assigns default values to page and per_page. Although the REST endpoint doesn't support pagination, it's implemented client-side, inside the getEntityRecords selector.
• this PR partially disables this "client-side pagination" for entities that don't have config.supportsPagination === true. It will work only when you specify page and per_page explicitly. If you don't specify them, getEntityRecords( 'root', 'icon' ) will return the full list by default.

it can be seen as a breaking change.

The breaking change is that the client-side pagination is no longer the default? I.e., that the getEntityRecords( 'root', 'icon' ) doesn't return just the first 10 items, but all 80+? Or is there also something else?

Implementation-wise, I don't like the fact that getQueriedItems always looks at page and per_page and respects them, and that we need getNonPaginatedQuery to create a "fake" paginated query, adding an artificial per_page=-1 query arg. I would prefer the following, if possible:

• getQueryParts doesn't add any default values for page and per_page. They are relevant only when supportsPagination is true.
• it's the getQueriedItems selector that reads the supportsPagination flag and does the pagination accordingly, not getNonPaginatedQuery.

The breaking change is that the client-side pagination is no longer the default? I.e., that the getEntityRecords( 'root', 'icon' ) doesn't return just the first 10 items, but all 80+? Or is there also something else?

That could be considered a breaking change, but consumers were relying on the existing bug. The whole data set was received but then discarded.

@jsnajdr, I've pushed changes based on your feedback, plus rebased the branch after the offset fixes.

Based on 849901f and to limit breaking changes, we might want to configure entities supportsPagination explicitly. Currently, entities don't support pagination by default.

Plan:

1. Update core entity configs and explicitly define supportsPagination.
2. If supportsPagination is undefined, selectors should work as before - slice the data on the client.

Update: This seems like the right direction. Pushed changes with the last commit.

Any idea why we have createSelector here when there is no dependents array to memoize by? The caching is implemented manually inside the selector.

This is all I can find #6395 (comment).

Interesting, it's @aduth's work 😄 @aduth do you happen to remember why you used createSelector without any dependency array 8 years ago, back in May 2018? 😉

I believe that all the memoization is done already by the two level cache, first level for state, second level for query.

Hm, I don't remember exactly. It does seem intentional and was documented in the function's DocBlock ("caches result by both per state (by reference) and per query (by deep equality)". It does practically have dependents on the whole state object. But in practice you're right that the inner caching logic is pretty effective on its own. I think the one key difference is that createSelector memoization is going to be a lot faster than EquivalentKeyMap lookup if the query object is a stable reference between calls.

I think the one key difference is that createSelector memoization is going to be a lot faster than EquivalentKeyMap lookup if the query object is a stable reference between calls.

I think most consumers pass inline query objects into useSelect, which makes it harder to ensure stable references when you're dealing with dynamic query values (search, order, include, etc.). So actual benefit of this would be hard to measure.

The options object doesn't need to be part of the cache key, is that right? It's always a constant for a given kind/name entity type combination.

That's correct.