Remove useSelect subscription for data that does not need to be reactive #72849
Conversation
This file contains hidden or 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
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Size Change: +4 B (0%) Total Size: 2.38 MB
ℹ️ View Unchanged
|
Member
|
What’s the measured performance impact of the change proposed? You can also use a public method from the package that simplifies the usage. However this brings the risk that late source registration or deregistration from any of the plugins might not be immediately visible in UI without the re-render triggered from different reasons. Overall, the way block editor works without a central WP init action that would orchestrate all plugins like on the server makes it all very brittle, so reactivity mitigates some of the race conditions. |
Contributor
Author
It was quite minor, if any. Just something I came across while investigating any potential bottlenecks. I figured removing a subscription for every block is better than having it, if there aren't any downsides. But, if you think there's a risk of race conditions, then it's not worth it! I'll close this out then. |
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?
getAllBlockBindingsSources()was called viauseSelectwith empty dependencies inEditWithGeneratedProps, which wraps every block. This creates a new observer oncore/blocksfor every block. While the blocksStore rarely updates, it's cleaner to not need an extra observer for every block.Why?
We can get the value without needing it to be a subscription. This would only show up as a performance issue for tests that include many blocks on them.
How?
Replace the
useSelect( ( selector ) => {}, [] )which creates a subscription in favor ofconst { getAllBlockBindingsSources } = unlock( useSelect( blocksStore ) );which does not create a the subscription.Testing Instructions
There should be no changes from trunk
Testing Instructions for Keyboard
Screenshots or screencast