Data: Include more details when shallow equality fails in 'useSelect' #67713
+20
−4
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
Mamaduka
added
[Type] Enhancement
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Size Change: 0 B Total Size: 1.83 MB ℹ️ View Unchanged
|
jsnajdr
reviewed
Dec 9, 2024
| keys = Object.keys( a ).filter( ( k ) => a[ k ] !== b[ k ] ); | ||
| } else if ( Array.isArray( a ) && Array.isArray( b ) ) { | ||
| keys = [ ...a.keys() ].filter( ( i ) => a[ i ] !== b[ i ] ); | ||
| } |
There was a problem hiding this comment.
Object.keys should work equally well also for arrays. The only difference is that .keys() returns numbers, while Object.keys are strings.
The a.constructor check will crash when a is null or undefined, we should guard for that.
There was a problem hiding this comment.
Object.keys should work equally well also for arrays. The only difference is that .keys() returns numbers, while Object.keys are strings.
Yeah. I should have guessed that 😅
The a.constructor check will crash when a is null or undefined, we should guard for that.
I've borrowed the check for isShallowEqual, so it should be safe in this case.
Based on your suggestion, I think we can simplify logic to the example below. What do you think?
const keys =
typeof a === 'object' && typeof b === 'object'
? Object.keys( a ).filter( ( k ) => a[ k ] !== b[ k ] )
: [];There was a problem hiding this comment.
I've borrowed the check for
isShallowEqual, so it should be safe in this case.
But isShallowEqual does an if ( a && b ) check before it starts looking at a.constructor.
typeof a === 'object' && typeof b === 'object'
typeof null is also 'object' 🙀 It will all work only if you exit early when a or b is falsy.
There was a problem hiding this comment.
I should stop ignoring the "everything is an object" JS motto 😄
Pushed update in 39bc8c1.
jsnajdr
reviewed
Dec 9, 2024
| 'This can lead to unnecessary re-renders and performance issues if not fixed.\n\n' + | ||
| 'Non-equal value keys: %s\n\n', | ||
| keys.join( ', ' ) | ||
| ); |
There was a problem hiding this comment.
The warning will be confusing when the b object has some field that the a object doesn't have. Then it will report an empty list of non-equal keys.
It can happen in practice when we write code like this:
useSelect( select => {
if ( someCondition ) {
return { loading: false };
}
return { loading: false, data: select( store ).getData() };
} );To fix this, we could construct a set of all fields on both objects and then filter it:
const allKeys = Array.from( new Set( [ ...Object.keys( a ), ...Object.keys( b ) ] ) );
const keys = allKeys.filter( ( k ) => a[ k ] !== b[ k ] );But this will fail again for objects like {} and { a: undefined } which are not shallow equal but keys will be [].
There was a problem hiding this comment.
The warning will be confusing when the b object has some field that the a object doesn't have. Then it will report an empty list of non-equal keys.
It means that the b run happened with different state or parameters (mostly params, based on your example). That shouldn't be the case for this warning, as it only makes sense with the same state/params.
There was a problem hiding this comment.
Oh that's true, I've been overthinking it 🙂 Running the mapSelect callback twice with the same state shouldn't produce objects like this unless you're doing it on purpose.
|
Flaky tests detected in 39bc8c1. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/12232091964
|
jsnajdr
approved these changes
Dec 9, 2024
|
@jsnajdr, as always, thank you for the thoughtful review. |
|
This is really useful, thank you @Mamaduka 🙌 Also, thank you both for the discussion on handling language subtleties - it is always useful and a pleasure to read! |
|
I'm considering exposing this warning to third-party consumers when the cc @gziolo |
Discussed also on WordPress Slack here. It sounds like a great idea to me. For me, |
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 is a follow-up to #53666.
Closes #63608.
PR updates the warning that's logged when
useSelectreturns different values when run with the same state and parameters. The warning now also includes the keys that fail the check.Why?
A quality of life improvement when debugging
useSelectissues.Testing Instructions
Test plugin
Testing Instructions for Keyboard
Same.
Screenshots or screencast