Data: Improve hasResolvingSelectors redux metadata selector
#50865
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tyxla
added
[Type] Enhancement
|
Size Change: -4 B (0%) Total Size: 1.4 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 7cc91b6. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5055444848
|
mcsf
approved these changes
May 23, 2023
Comment on lines
145
to
147
| * This uses the internal `_map` property of `EquivalentKeyMap` for | ||
| * optimization purposes, since the `EquivalentKeyMap` implementation | ||
| * does not support a `.some()` implementation. |
There was a problem hiding this comment.
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?).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Hmm, I actually think I misunderstood you. Let me correct this.
There was a problem hiding this comment.
Updated. Thanks for the feedback!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
This PR adds a couple of improvements to the
hasResolvingSelectorsselector to address the feedback provided in #50222 (comment).Why?
To address some feedback and use the opportunity to improve and better document the selector.
How?
We're essentially doing a couple of improvements:
Object.values()already returns an array and cloning is unnecessary since there's no way to mutate it.EquivalentKeyMapproperty usage.Testing Instructions
Testing Instructions for Keyboard
None
Screenshots or screencast
None