Inserter: Return the same items when the state and parameters don't change #62263
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
Mamaduka
added
[Type] Bug
|
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: -65 B (0%) Total Size: 1.74 MB
ℹ️ View Unchanged
|
ellatrix
reviewed
Jun 4, 2024
| select( blockEditorStore ).getInserterItems( | ||
| rootClientId, | ||
| options | ||
| ), |
There was a problem hiding this comment.
Shouldn't we account for this in getInserterItems? Also the default there is an empty object on every call, so we should change that too?
There was a problem hiding this comment.
Shouldn't we account for this in getInserterItems?
That would be ideal, but we need to figure out how. Functions have no control over the arguments they receive.
Also the default there is an empty object on every call, so we should change that too?
Yes. We should either pass undefined or an EMTPY_OBJECT.
There was a problem hiding this comment.
Yeah, this is a good fix for now. I wonder if we should just create another private getInserterItems selector and avoid the options altogether. Actually this would also allow us to remove all the reusable blocks logic. We're adding these blocks, just to filter them back out in the UI 😄
|
Confirmed that this also fixes perf regression https://github.com/WordPress/gutenberg/actions/runs/9361910664/attempts/3#summary-25773603112 |
|
Do we still need the default parameter change to fix calls without options? |
Yes, and I just pushed an update for it. |
Mamaduka
commented
Jun 4, 2024
| @@ -76,6 +76,8 @@ const EMPTY_ARRAY = []; | |||
| */ | |||
| const EMPTY_SET = new Set(); | |||
|
|
|||
| const EMPTY_OBJECT = {}; | |||
There was a problem hiding this comment.
I've omitted the comment here. The other ones are a bit outdated and we should probably revise them together.
ellatrix
added
the
Backport to WP 6.7 Beta/RC
|
This will need to be backported |
ellatrix
reviewed
Jun 4, 2024
| @@ -1996,7 +1998,7 @@ const buildBlockTypeItem = | |||
| */ | |||
| export const getInserterItems = createRegistrySelector( ( select ) => | |||
| createSelector( | |||
| ( state, rootClientId = null, options = {} ) => { | |||
| ( state, rootClientId = null, options = EMPTY_OBJECT ) => { | |||
There was a problem hiding this comment.
Actually I guess this doesn't matter because createSelector is not aware of the default, it would just not be defined. Sorry my mistake :D
westonruter
added a commit
that referenced
this pull request
Jun 4, 2024
…dd/on-async-directives * 'trunk' of https://github.com/WordPress/gutenberg: (72 commits) Top toolbar: fix half a pixel artifacting of the bottom border. (#62225) Fix UI order for theme.json spacing sizes (#62199) Chore: Simplify a padding style on global styles. (#62291) Editor: Avoid remounts of `DocumentBar` (#62214) Add `default-spacing-sizes` and `default-font-sizes` options for classic themes (#62252) Editor: Cleanup styles and classnames (#62237) Scripts: Pin the @wordpress/scripts version to a version supported by WP 6.5 (#62234) Documentation: Better changelogs for the JSX transform upgrade (#62265) Chore: Simplify a padding style on dataviews. (#62276) MediaUpload: Remove dialog markup on close (#62168) Tabs: Prevent accidental overflow in indicator (#61979) Make edit site pagination buttons accessibly disabled. (#62267) Fix: Remove unused code from dataviews styles. (#62275) Re-enable React StrictMode (#61943) Inserter: Return the same items when the state and parameters don't change (#62263) Update instances of text-wrap: pretty to fall back to balance (#62233) Update: Slotfill documentation samples (links, code, and rephrase). (#62271) Try: Fix mover positioning. (#62226) [Mobile] - Image corrector - Check the path extension is a valid one (#62190) Update: Block styles documentation. ...
ellatrix
added a commit
that referenced
this pull request
Jun 11, 2024
…hange (#62263) * Inserter: Return the same items when the state and parameters don't change * Use stable default Co-authored-by: Mamaduka <mamaduka@git.wordpress.org> Co-authored-by: ellatrix <ellatrix@git.wordpress.org> Co-authored-by: tyxla <tyxla@git.wordpress.org>
ellatrix
added a commit
that referenced
this pull request
Jun 11, 2024
…hange (#62263) * Inserter: Return the same items when the state and parameters don't change * Use stable default Co-authored-by: Mamaduka <mamaduka@git.wordpress.org> Co-authored-by: ellatrix <ellatrix@git.wordpress.org> Co-authored-by: tyxla <tyxla@git.wordpress.org>
patil-vipul
pushed a commit
to patil-vipul/gutenberg
that referenced
this pull request
Jun 17, 2024
…hange (WordPress#62263) * Inserter: Return the same items when the state and parameters don't change * Use stable default Co-authored-by: Mamaduka <mamaduka@git.wordpress.org> Co-authored-by: ellatrix <ellatrix@git.wordpress.org> Co-authored-by: tyxla <tyxla@git.wordpress.org>
ellatrix
removed
the
Backport to WP 6.7 Beta/RC
|
This was cherry-picked to the wp/6.6 branch. |
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?
PR updates the selector in the
useBlockTypesStatehook to return the sameitemswhen the state and parameters haven't changed.This is a follow-up to #62169 (comment).
Why?
The
getInserterItemsis a memoized selector, and when an argument is different on each call, it will return different (fresh) values. This behavior makes it complicated to addarrayorobjecttype parameters to similar selectors. Example: #40718.How?
Memoize the
optionspassed to the selector on the component level.I don't like shifting this responsibility to the consumer, but I see no other way besides changing the
getInserterItemssignature.Testing Instructions
useBlockTypesStatehook generates no warning.Testing Instructions for Keyboard
Same.
Screenshots or screencast