Block Editor: Improve getBlockInsertionPoint memoization
#47489
Merged
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] Performance
tyxla
requested review from
Mamaduka,
youknowriad,
jsnajdr,
talldan,
flootr,
jorgefilipecosta and
ntsekouras
|
Size Change: +35 B (0%) Total Size: 1.31 MB
ℹ️ View Unchanged
|
youknowriad
reviewed
Jan 27, 2023
| state.insertionPoint?.rootClientId, | ||
| state.insertionPoint?.index, | ||
| state.insertionPoint?.__unstableWithInserter, | ||
| state.insertionPoint?.operation, |
There was a problem hiding this comment.
I don't understand why we need to do this, these are all the keys of the state no?
There was a problem hiding this comment.
Because theoretically, insertionPoint is an object, and by listing those here, we'll prevent recalculation if state.insertionPoint is assigned a new object with the same subfields.
I don't have proof that this is causing an issue right now, the main motivation for this PR was selectionEnd causing the issue I described. So if you prefer omitting those, I'm fine with that. Then can re-add them if we find a practical need for that.
WDYT?
There was a problem hiding this comment.
Yes, let's do that. Restore the insertion point and keep the change for selectionEnd. Thanks Marin
There was a problem hiding this comment.
Done in ab5ea8a
Thanks for the thorough and in-depth reviews, @youknowriad ❤️
|
Flaky tests detected in ed18db3. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4023605083
|
youknowriad
approved these changes
Jan 27, 2023
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 improves the memoization of
getBlockInsertionPoint()by making the list of dependents more specific. This allows us to avoid unnecessary extra rerenders when the insertion point is the same but an update is triggered just because we return a new object with the same data.Why?
After landing #47448 I noticed that there are still unnecessary re-renders caused by
getBlockInsertionPoint(), in cases where we're moving inside the same block, for example.How?
We're listing the dependents specifically and that way we avoid unnecessary updates when new nested objects with the same data are being dispatched. This was particularly a problem for
state.selection.selectionEnd, which can be a different object but thestate.selection.selectionEnd.clientIdwill still be the same, and that's all the selector needs from that state subtree, so the update was unnecessary.Testing Instructions
Testing Instructions for Keyboard
None.
Screenshots or screencast
None.