Block edit: avoid memoized block context in favour of useSelect #29333
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
ellatrix
force-pushed
the
try/avoid-memoized-block-context
branch
from
b28f4a9 to
e20310d
Compare
|
Size Change: -15 B (0%) Total Size: 1.4 MB
ℹ️ View Unchanged
|
ellatrix
force-pushed
the
try/avoid-memoized-block-context
branch
from
e20310d to
5c5e387
Compare
ellatrix
force-pushed
the
try/avoid-memoized-block-context
branch
from
5c5e387 to
3ddbaef
Compare
6 tasks
|
I run into this PR when investigating failing tests in #28456. I have some concerns regarding the changes proposed in this PR. It feels like the
|
|
Cc @youknowriad I'll investigate the performance problem. Generally we should avoid passing data down that can be derived from something else like the client ID. Where do we draw the line? You could pass lots of data like the block type, other selection info... through memoized context. Our data API should be a superior way to retrieve and memoize data only in places where it's needed. |
|
Worth noting that #29309 deprecates the hook in favour of a new one, which restores performance to previous levels. The PR is blocked by mobile though, which is why it was done it multiple pieces. |
|
I believe the temporary performance hit is due to some extra |
|
This is also breaking the navigation editor. The block inspector is now not showing any of the UI we're adding through filters: Please can we stop merging these unreviewed PRs? By my count that's the second major regression in a week caused by these kind of PRs. There was also another a couple of weeks ago. |
|
I agree that the performance gap here is too big to ignore. While I agree on the principle that the selectors should only be called when needed, I think the |
|
Looking into it some more, this isn't an issue with filters, but the Can we revert this? The regression is currently blocking a few PRs. |
This makes sense to me, it's a small PR that we can bring back if we figure out the issues. |
I contemplated a bit more about this proposal. If you look at it only from the performance perspective, it can’t be more performant than keeping |
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
Ideally the only piece of context is the block client ID and everything else is derived from that. Unfortunately
useBlockEditContextis exported at the package level (not sure why because it's only used within the package), but it can be rewritten withuseSelect.After this we should create a new hook to only return the client ID, but I ran into some errors there which I'm trying to pinpoint. Doing this step by step instead.
How has this been tested?
Screenshots
Types of changes
Checklist: