Add support for page=1 and perPage=-1 to getMergedItemIds #22707
Conversation
|
Size Change: +37 B (0%) Total Size: 1.12 MB
ℹ️ View Unchanged
|
adamziel
force-pushed
the
fix/queries-reducer-duplicating-ids
branch
from
f62448c to
df6e43b
Compare
adamziel
changed the title
There was a problem hiding this comment.
It gets a bit strange to consider, since technically -1 is not a valid value for the per_page parameter. The support is added externally via apiFetch fetchAllMiddleware middleware. I guess it's fair to consider this support consistently across packages, despite not being supported by the REST endpoints themselves.
| @@ -32,6 +32,7 @@ import getQueryParts from './get-query-parts'; | |||
| * @return {number[]} Merged array of item IDs. | |||
| */ | |||
| export function getMergedItemIds( itemIds, nextItemIds, page, perPage ) { | |||
| const receivedAllIds = page === 1 && perPage === -1; | |||
There was a problem hiding this comment.
Isn't it the case that the entire function can be short-circuited if this is true?
| const receivedAllIds = page === 1 && perPage === -1; | |
| const receivedAllIds = page === 1 && perPage === -1; | |
| if ( receivedAllIds ) { | |
| return nextItemIds; | |
| } |
There was a problem hiding this comment.
It is! In fact that's also how my initial attempt looked like. It's just the result is would no longer be "sparse-like" as it normally is - it works for me if it works for you though :-) I just updated the PR accordingly.
There was a problem hiding this comment.
Actually I'm wrong, the result will be exactly the same, maybe except for the fact that now it returns the original object instead of a new one.
There was a problem hiding this comment.
I just updated it to return a copy instead of the original list to maintain the consistency with the other code branch.
There was a problem hiding this comment.
Personally I don't think it matters too much if it returns a copy, since the function doesn't really assert one way or the other, and in most cases throughout the code, we encourage to avoid object mutation and make no guarantees about behaviors (or misbehaviors) when mutation is performed.
| @@ -45,7 +49,6 @@ export function getMergedItemIds( itemIds, nextItemIds, page, perPage ) { | |||
| const mergedItemIds = new Array( size ); | |||
|
|
|||
| for ( let i = 0; i < size; i++ ) { | |||
| // Preserve existing item ID except for subset of range of next items. | |||
There was a problem hiding this comment.
If the comment still applies, can it be left intact?
| @@ -32,6 +32,7 @@ import getQueryParts from './get-query-parts'; | |||
| * @return {number[]} Merged array of item IDs. | |||
| */ | |||
| export function getMergedItemIds( itemIds, nextItemIds, page, perPage ) { | |||
| const receivedAllIds = page === 1 && perPage === -1; | |||
There was a problem hiding this comment.
Personally I don't think it matters too much if it returns a copy, since the function doesn't really assert one way or the other, and in most cases throughout the code, we encourage to avoid object mutation and make no guarantees about behaviors (or misbehaviors) when mutation is performed.
|
@aduth I just addressed your notes 👍 |
Description
getMergedItemIds()does not handle a scenario whereperPage = -1(nextItemIds are all ids from all pages). This results in the following issue:A practical consequence of this is that dispatching
receiveEntityRecordswith data similar to these:Updates the state in such a way that
entities.data.menuItem.queriedData.queries["menus=2"]contains duplicate entries like[1, 2, 3, 2, 3]. This in turn causes aselect()call to return duplicate results.How has this been tested?
Confirm all unit and e2e tests pass
Types of changes
Bug fix (non-breaking change which fixes an issue)
Checklist: