-
Notifications
You must be signed in to change notification settings - Fork 4.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
TemplateContentPanel: Fix the 'getBlocksByName' selector call #63922
Conversation
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: 0 B Total Size: 1.76 MB ℹ️ View Unchanged
|
@@ -45,8 +45,8 @@ export default function DisableNonPageContentBlocks() { | |||
); | |||
const disabledIds = useSelect( ( select ) => { | |||
const { getBlocksByName, getBlockOrder } = select( blockEditorStore ); | |||
return getBlocksByName( [ 'core/template-part' ] ).flatMap( | |||
( clientId ) => getBlockOrder( clientId ) | |||
return getBlocksByName( 'core/template-part' ).flatMap( ( clientId ) => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem above remains with contentOnlyBlockTypes
, which is the result of a filter that may return a new array on every call. Is there any way to fix it in the selector instead, so the user of the selector doesn't need to know about it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a wrapper getBlocksByName
shouldn't be memoized, checks for shallow equality, then the underlying function is memoized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The filter is called inside useMemo,
so I think contentOnlyBlockTypes
should have a stable ref during the component's life cycle.
Is there any way to fix it in the selector instead, so the user of the selector doesn't need to know about it?
That's a tricky part with memoizing selectors that accept array or object as arguments. I floated an idea of making them variadic functions, but I've not tested my theory yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If nothing else I think we should prefer passing a string when possible.
What?
PR updates the
getBlocksByName
selector call and passes the block name as a string. The selector supports both strings and an array of strings as arguments.Why?
The
getBlocksByName
is a memoized selector passing a new array on each call, will unnecessarily invalidate the cache.Testing Instructions
Testing Instructions for Keyboard
Same
Screenshots or screencast