getInsertionPoint: Avoid returning a different object on every call #53722
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] Bug
Mamaduka
requested review from
noisysocks,
draganescu,
talldan,
tellthemachines,
adamziel and
kevin940726
as code owners
|
Size Change: -199 B (0%) Total Size: 1.51 MB
ℹ️ View Unchanged
|
jsnajdr
reviewed
Aug 16, 2023
| @@ -35,6 +35,5 @@ export function isInserterOpened( state ) { | |||
| * @return {Object} The root client ID and index to insert at. | |||
| */ | |||
| export function __experimentalGetInsertionPoint( state ) { | |||
| const { rootClientId, insertionIndex } = state.blockInserterPanel; | |||
| return { rootClientId, insertionIndex }; | |||
There was a problem hiding this comment.
The reducer state can be false, so the selector now can return also a boolean in addition to the { rootClientId, insertionIndex } object. On one hand that's fine, because false is object-like and false.rootClientId evaluates to undefined. It doesn't crash. On the other hand, the selector's return type becomes messy and TypeScript would be very sad seeing this.
I propose to also patch the reducers so that state.blockInserterPanel is always an object with the same shape, { rootClientId, insertionIndex }, sometimes plus filterValue. The initial values of the fields are null. And actions that until now were setting the state to false would reset the fields back to nulls.
There was a problem hiding this comment.
Agree. This is a bit confusing way to handle the state. I was just trying to keep changes related to the issue here, but I am also happy to work on those improvements.
The same state is responsible for opening the block inserter - setIsInserterOpened( true ). How do you suggest handling a truthy value? The insertionPoint data isn't required for opening the inserter.
There was a problem hiding this comment.
For the __experimentalGetInsertionPoint selector, it doesn't matter at all whether the state value is true or false. The const { rootClientId } = state destructuring always yields the same undefined value.
But patching the reducers is not as easy as I originally thought, because there's also the isInserterOpened selectors that cares about the boolean value.
We could do this:
const EMPTY_INSERTION_POINT = {
rootClientId: undefined,
insertionIndex: undefined,
};
function __experimentalGetInsertionPoint( state ) {
if ( typeof state == 'boolean' ) {
return EMPTY_INSERTION_POINT;
}
return state;
}That gives the selector a consistent return type, the same as it had before this PR, and it should be easy to implement.
There was a problem hiding this comment.
Sounds good. I'll update the PR.
I think in the future, we could split the state in two - isInserterOpen and blockInsertionPoint.
jsnajdr
approved these changes
Aug 16, 2023
Thanks for addressing the feedback.
|
Nice optimization 👏 |
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?
Avoid returning a different object when the
__experimentalGetInsertionPointselector is called with the same state multiple times.There's one more definition of the same selector in the
edit-sitepackage. I'll handle it separately as it contains more logic.Why?
It prevents unnecessary rerenders. See #53666.
How?
Return
state.blockInserterPaneldirectly instead of destructuring and reconstructing the object.Testing Instructions
Selector warning
The 'useSelect' hook returns different values when called with the same state and parameters. This can lead to unnecessary rerenders.Selector behavior
Screenshots or screencast
CleanShot.2023-08-16.at.13.22.53.mp4