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

Add support for page=1 and perPage=-1 to getMergedItemIds #22707

Merged
merged 8 commits into from
May 28, 2020
+18 −0
Merged

Add support for page=1 and perPage=-1 to getMergedItemIds #22707

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
8a205c9
Add support page=1 and perPage=-1 to getMergedItemIds
adamziel May 28, 2020
df6e43b
Add one more test case
adamziel May 28, 2020
d13ec11
short-circuit instead of looping through the data
adamziel May 28, 2020
7b6116f
Update test descriptions
adamziel May 28, 2020
470ec3b
Return a copy of nextItemIds instead of the original list
adamziel May 28, 2020
1223c74
Update reducer.js
adamziel May 28, 2020
27dc6d3
Update reducer.js
adamziel May 28, 2020
d533333
Update reducer.js
adamziel May 28, 2020
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
4 changes: 4 additions & 0 deletions packages/core-data/src/queried-data/reducer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,10 @@ 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;
Copy link
Member

@aduth aduth May 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Isn't it the case that the entire function can be short-circuited if this is true?

Suggested change
const receivedAllIds = page === 1 && perPage === -1;
const receivedAllIds = page === 1 && perPage === -1;
if ( receivedAllIds ) {
return nextItemIds;
}

Copy link
Contributor Author

@adamziel adamziel May 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@adamziel adamziel May 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I just updated it to return a copy instead of the original list to maintain the consistency with the other code branch.

Copy link
Member

Choose a reason for hiding this comment

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

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.

if ( receivedAllIds ) {
return nextItemIds;
}
const nextItemIdsStartIndex = ( page - 1 ) * perPage;

// If later page has already been received, default to the larger known
Expand Down
14 changes: 14 additions & 0 deletions packages/core-data/src/queried-data/test/reducer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,20 @@ describe( 'getMergedItemIds', () => {

expect( result ).toEqual( [ 1, 2, 3, 4, 5, 6, 7 ] );
} );

it( 'should return a copy of nextItemIds if it represents all ids (single id removed) (page=1 and perPage=-1)', () => {
const original = deepFreeze( [ 1, 2, 3 ] );
const result = getMergedItemIds( original, [ 1, 3 ], 1, -1 );

expect( result ).toEqual( [ 1, 3 ] );
} );

it( 'should return a copy of nextItemIds if it represents all ids (single id removed and another one added) (page=1 and perPage=-1)', () => {
const original = deepFreeze( [ 1, 2, 3 ] );
const result = getMergedItemIds( original, [ 1, 3, 4 ], 1, -1 );

expect( result ).toEqual( [ 1, 3, 4 ] );
} );
} );

describe( 'reducer', () => {
Expand Down