Fix editing performance in Widgets Customizer #30654
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
kevin940726
added
[Type] Bug
|
Size Change: +39.9 kB (+3%) Total Size: 1.46 MB
ℹ️ View Unchanged
|
kevin940726
commented
Apr 13, 2021
Comment on lines
271
to
304
| canUndo() { | ||
| return this.historyIndex > 0; | ||
| } | ||
| canRedo() { | ||
| return this.historyIndex < this.history.length - 1; | ||
| } | ||
| _seek( historyIndex ) { | ||
| const currentWidgets = this.getWidgets(); | ||
|
|
||
| this.historyIndex = historyIndex; | ||
|
|
||
| const widgets = this.history[ this.historyIndex ]; | ||
|
|
||
| this._internalUpdateWidgets( widgets ); | ||
|
|
||
| this._emit( currentWidgets, this.getWidgets() ); | ||
| } | ||
| undo() { | ||
| if ( ! this.canUndo() ) { | ||
| return; | ||
| } | ||
|
|
||
| this._seek( this.historyIndex - 1 ); | ||
| } | ||
| redo() { | ||
| if ( ! this.canRedo() ) { | ||
| return; | ||
| } | ||
|
|
||
| this._seek( this.historyIndex + 1 ); |
There was a problem hiding this comment.
These are currently unused, but paving the way for the undo/redo feature in the future.
noisysocks
reviewed
Apr 14, 2021
packages/customize-widgets/src/components/sidebar-block-editor/sidebar-editor-provider.js
Outdated
Show resolved
Hide resolved
| ignoreIncoming.current = true; | ||
| ( nextBlocks ) => { | ||
| setBlocks( ( prevBlocks ) => { | ||
| if ( isShallowEqual( prevBlocks, nextBlocks ) ) { |
There was a problem hiding this comment.
Is this ever true? I don't understand why useBlockSync would call onChange if nothing has changed.
There was a problem hiding this comment.
It happened once in my testing, not sure why though 🤔.
There was a problem hiding this comment.
IIRC, useBlockSync works based on the reference to the blocks array; I don't know if it goes through the array of blocks. It theoretically works because the block-editor store is supposed to make sure that references to a slice of blocks don't change unless the blocks actually did change. (To prevent re-renders and stuff.)
It might be worth changing that line to use isShawllowEqual as well. I think it may have been avoided to prevent looping over the entire array of blocks every time the subscription callback fires
There was a problem hiding this comment.
It might be worth changing that line to use
isShawllowEqualas well. I think it may have been avoided to prevent looping over the entire array of blocks every time the subscription callback fires
Yeah I think that would make sense! I believe it also means that I don't have to do the weird if (nextBlocks === newBlocks) newBlocks = [...nextBlocks] thing below.
Want to make an issue/PR?
| const prevWidget = sidebar.getWidget( widgetId ); | ||
|
|
||
| // Bail out updates by returning the previous widgets. | ||
| // Deep equality is necessary until the block editor's internals changes. |
There was a problem hiding this comment.
What do you mean by "until the block editor's internals change"? Is this ever going to happen?
There was a problem hiding this comment.
It seems like we currently use isEqual under the hood in our reducer for every edit in the block editor package. Until we optimize it there, it's best to follow the same logic here and do a isEqual check for every edit as well. I have no idea if we ever have time to optimize it though 😅.
packages/customize-widgets/src/components/sidebar-block-editor/use-sidebar-block-editor.js
Outdated
Show resolved
Hide resolved
Comment on lines
+66
to
+69
| this.canUndo = this.canUndo.bind( this ); | ||
| this.canRedo = this.canRedo.bind( this ); | ||
| this.undo = this.undo.bind( this ); | ||
| this.redo = this.redo.bind( this ); |
There was a problem hiding this comment.
Binding these shouldn't be necessary. It's only needed when we use a class member function as a callback.
There was a problem hiding this comment.
It's just easier if any components/hooks want to access these methods without having to memoize/bind them again. We don't need these now though.
const { canUndo, canRedo, undo, redo } = sidebar;
packages/customize-widgets/src/components/sidebar-block-editor/sidebar-adapter.js
Show resolved
Hide resolved
| _handleSettingChange( newWidgetIds, oldWidgetIds ) { | ||
| const addedWidgetIds = difference( newWidgetIds, oldWidgetIds ); | ||
| const removedWidgetIds = difference( oldWidgetIds, newWidgetIds ); | ||
| _getWidgetIds() { |
There was a problem hiding this comment.
It's slightly confusing that getWidgets() and _getWidgetIds() are named similarly but the former returns "adapted" data and the returns data from the underlying setting. I'd consider not having _getWidgetIds() and instead using this.setting.get() everywhere. It's more verbose but, in my opinion, less ambiguous.
There was a problem hiding this comment.
I tried that but I thought this.setting.get() isn't very clear what it would return.
packages/customize-widgets/src/components/sidebar-block-editor/sidebar-adapter.js
Outdated
Show resolved
Hide resolved
packages/customize-widgets/src/components/sidebar-block-editor/sidebar-adapter.js
Show resolved
Hide resolved
packages/customize-widgets/src/components/sidebar-block-editor/sidebar-adapter.js
Outdated
Show resolved
Hide resolved
noisysocks
approved these changes
Apr 15, 2021
kevin940726
force-pushed
the
fix/widgets-customizer-edit-performance
branch
from
2c0b10d
to
b8a326e
Compare
kevin940726
requested review from
adamsilverstein,
adamziel,
ajitbohra,
ajlende,
atimmer,
cameronvoell,
chrisvanpatten,
dmsnell,
draganescu,
ellatrix and
guarani
as code owners
kevin940726
requested review from
gziolo,
mkaz,
mkevins,
mmtr,
nerrad,
noahtallen,
oandregal,
ntwb,
spacedmonkey,
swissspidy and
TimothyBJacobs
as code owners
kevin940726
changed the base branch from
fix/create-new-block-on-enter
to
trunk
|
Shoot, I kept forgetting to change my base before rebasing and force pushing, sorry for the pings folks 😅. |
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.
Description
This fixes an important performance issue in the Widgets Customizer where editing any block will re-render the whole block editor. This branch is based on #30221 for easier review but should be re-target to trunk before merge.
The problem with the original approach is that every time we update any block we create a new instance of the
nextBlocksarray and set it back to the component's state viasetState. This feels intuitive but apparently it breaks the optimizationuseBlockSynchas. Turns out we need to make sure thenextBlocksinstance we pass to the<BlockEditorProvider>'svalueshould be referential equal to thenextBlockswe received from theonChangecallback. Or else all changes are marked as persistent and will re-render the whole block editor.The solution is simply to call
setStatewith thenextBlockswe receive from the callback whenever possible.I also essentially refactored the code in
SidebarAdaptoranduseSidebarBlockEditorto use the customize API as the source of truth of all states, and simplify the logic a little bit. I added a few methods regarding undo/redo in #30400. They're currently unused yet though, but they should work. We can add the UI in another PR.How has this been tested?
Screenshots
Kapture.2021-04-09.at.14.49.28.mp4
Types of changes
Bug fix
Checklist:
*.native.jsfiles for terms that need renaming or removal).