Improve Slot/Fill performance #44642
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
|
Size Change: +2.28 kB (0%) Total Size: 1.27 MB
ℹ️ View Unchanged
|
|
So this indeed solves the issue but there's a few test failures that I need to address as well (tomorrow) |
kevin940726
reviewed
Oct 4, 2022
| * External dependencies | ||
| */ | ||
| import { ref as valRef } from 'valtio'; | ||
| import { proxyMap } from 'valtio/utils'; |
There was a problem hiding this comment.
Any reason why we want to introduce yet another package here to solve this issue? Is it too tedious to solve in vanilla React?
There was a problem hiding this comment.
Yes, I found it a bit difficult (also a lot of code) to recreate the same behavior with only React. Basically we'd want multiple event emitters, we want to subscribe to changes for each slot separately and each fills for a slot.
There was a problem hiding this comment.
Sorry, I accidentally marked this as resolved.
youknowriad
commented
Oct 4, 2022
youknowriad
commented
Oct 4, 2022
Mamaduka
reviewed
Oct 4, 2022
packages/components/src/slot-fill/bubbles-virtually/slot-fill-provider.js
Show resolved
Hide resolved
mtias
approved these changes
Oct 4, 2022
|
@youknowriad I think there might be a small regression. Sometimes when a block is selected, empty slots are rendering on the toolbar: It's a little inconsistent to reproduce. These steps seem to work:
|
|
Was going to report this one |
|
Nice work here! |
|
@youknowriad, do we publish dev notes for experimental APIs? I believe this is a breaking change for third-party code using Directory search: https://wpdirectory.net/search/01GPDJDZ7CA9W122V0ENRCA0T0 The problem was initially reported by the CoBlocks team. Example code that will break: function ExampleSlot( { children } ) {
const slot = useSlot( slotName );
const hasFills = Boolean( slot.fills && slot.fills.length );
if ( ! hasFills ) {
return children;
}
// return the actual slot;
return null;
} |
|
Yeah, at the time of this PR, |
14 tasks
This was referenced Jan 25, 2023
Merged
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 #42722 introduced a regression in performance for block selection and inserter opening, after some debugging, I found that the main issue is that the popover components are being re-rendered synchronously even if they're actually inside "Async mode".
The reason is because they're not being re-rendered due to a "store change" (which is impacted by async mode) but because a new fill is being added or removed on each block selection which calls the
SlotFillContextto update its value causing a re-render for any component callinguseSlot, including all popovers.How?
This PR solves this by making
SlotFillContextvalue stable across addition and removals of slot/fills and by using "valtio" library to be able to subscribe individually to a given slot name or fills for a given slot name. It also does so by introducing a new hook called useSlotFills which only gives you the "fills" of a given slot.Testing Instructions
1- Mainly check the results of the performance jobs.