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

Data: Improve hasResolvingSelectors redux metadata selector #50865

Merged
merged 4 commits into from
May 23, 2023
Merged

Data: Improve hasResolvingSelectors redux metadata selector #50865

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
15b964f
Remove unnecessary array cloning
tyxla May 23, 2023
7cc91b6
Document internal property usage
tyxla May 23, 2023
795937e
Use Array.from() instead of spread
tyxla May 23, 2023
0a2b679
Correct docs
tyxla May 23, 2023
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
9 changes: 8 additions & 1 deletion packages/data/src/redux-store/metadata/selectors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,14 @@ export function getCachedResolvers( state ) {
* @return {boolean} True if one or more selectors are resolving, false otherwise.
*/
export function hasResolvingSelectors( state ) {
return [ ...Object.values( state ) ].some( ( selectorState ) =>
return Object.values( state ).some( ( selectorState ) =>
/**
* This uses the internal `_map` property of `EquivalentKeyMap` for
* optimization purposes, since the `EquivalentKeyMap` implementation
* does not support a `.some()` implementation.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* This uses the internal `_map` property of `EquivalentKeyMap` for
* optimization purposes, since the `EquivalentKeyMap` implementation
* does not support a `.some()` implementation.
* This uses the internal `_map` property of `EquivalentKeyMap` for
* optimization purposes, since the `EquivalentKeyMap` implementation
* does not support a `.values()` implementation.

Unless I misunderstood, I believe you have a typo here. By analogy with the standard Map, we expect the prototype to contain values() but not some() (besides, what would this some() iterate over? Just the values? Keys? Keys and values?).

Copy link
Member Author

@tyxla tyxla May 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Oh well, .some() is what we intend to use here and it is what we actually use from Array.prototype once we convert the map to an array. We could rephrase this to use .values() or .entries() but I think since .some() is what we are practicaly using, it makes most sense to mention that in the documentation. We could mention .values() but it wouldn't immediately be clear as to where we'd need .values() since we never call Map.prototype.values(). Does this make sense?

I think you're right, will update. We do need only the values anyway.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, I actually think I misunderstood you. Let me correct this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated. Thanks for the feedback!

*
* @see https://github.com/aduth/equivalent-key-map
*/
[ ...selectorState._map.values() ].some(
( resolution ) => resolution[ 1 ]?.status === 'resolving'
)
Expand Down