-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Block Bindings: Refactor utils file. #64740
Conversation
Size Change: -105 B (-0.01%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
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. |
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.
Thank you for the follow up 👍
const { updateBlockAttributes } = useDispatch( blockEditorStore ); | ||
const { getBlockAttributes } = useSelect( blockEditorStore ); | ||
const { getBlockAttributes } = useRegistry().select( blockEditorStore ); |
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.
Is there a benefit to doing it for dispatch
as well?
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.
dispatch
doesn't establish event listeners, so it shouldn't make any difference. @jsnajdr, can you chime in?
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.
useDispatch
is implemented as useRegistry().dispatch( store )
, so changing the code has no effect. dispatch
indeed doesn't establish any event listeners.
Even this change for useSelect
is basically a noop, because useSelect( store )
is a special form that returns useRegistry().select( store )
. Only the callback form (useSelect( ( select ) => ... )
) establishes listeners.
All in all, the useBlockBindingsUtils
doesn't establish any local state or listeners and will never trigger rerenders. It merely returns two callbacks that will read from the store when called, and will see the value that is in the store at that moment.
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.
Ah, cool, so no change is necessary. That wasn't clear enough when reading the post on Make Core.
const { | ||
metadata: { bindings: currentBindings, ...metadata }, | ||
} = getBlockAttributes( clientId ); | ||
const newBindings = { ...currentBindings }; |
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 I am not mistaken, this will break when there is no metadata
and it is undefined. For example, when adding a binding to a new paragraph block. It tries to destructure metadata
expecting to be an object but it is undefined.
Is that right?
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.
We might need to do something like this maybe:
const { metadata: { bindings: currentBindings, ...metadata } = {} } =
getBlockAttributes( 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.
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.
Fixed in 9e98a29
const { | ||
metadata: { bindings, ...metadata }, | ||
} = getBlockAttributes( 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.
Not sure if this comment is relevant here as well because I guess removeAllBlockBindings
should not be called when there are no bindings. But maybe it is better to add a fallback just in case?
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.
It doesn't harm to add extra safety.
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.
Fixed in 9e98a29
🚢 |
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.
Seems to be working as expected now. Thanks for working on the follow-up 🙂
* Refactor block binding utils * Add security checks Co-authored-by: cbravobernal <cbravobernal@git.wordpress.org> Co-authored-by: gziolo <gziolo@git.wordpress.org> Co-authored-by: SantosGuillamot <santosguillamot@git.wordpress.org> Co-authored-by: jsnajdr <jsnajdr@git.wordpress.org>
* DataViews: Add missing styles and remove opinionated ones for generic usage (#64711) Co-authored-by: youknowriad <youknowriad@git.wordpress.org> Co-authored-by: ntsekouras <ntsekouras@git.wordpress.org> Co-authored-by: jameskoster <jameskoster@git.wordpress.org> * Shuffle: Don't call '__experimentalGetAllowedPatterns' for every block (#64736) Co-authored-by: Mamaduka <mamaduka@git.wordpress.org> Co-authored-by: tyxla <tyxla@git.wordpress.org> Co-authored-by: wpsoul <wpsoul@git.wordpress.org> * DataViews: hide sort direction control if there is no sortable fields (#64817) Co-authored-by: oandregal <oandregal@git.wordpress.org> Co-authored-by: ntsekouras <ntsekouras@git.wordpress.org> * Block Bindings: Refactor utils file. (#64740) * Refactor block binding utils * Add security checks Co-authored-by: cbravobernal <cbravobernal@git.wordpress.org> Co-authored-by: gziolo <gziolo@git.wordpress.org> Co-authored-by: SantosGuillamot <santosguillamot@git.wordpress.org> Co-authored-by: jsnajdr <jsnajdr@git.wordpress.org> * Interactivity API: Fix computeds without scope in Firefox (#64825) * Replace NO_SCOPE symbol with an object * Update changelog Co-authored-by: DAreRodz <darerodz@git.wordpress.org> Co-authored-by: luisherranz <luisherranz@git.wordpress.org> Co-authored-by: Marc-pi <mdxfr@git.wordpress.org> --------- Co-authored-by: Riad Benguella <benguella@gmail.com> Co-authored-by: youknowriad <youknowriad@git.wordpress.org> Co-authored-by: ntsekouras <ntsekouras@git.wordpress.org> Co-authored-by: jameskoster <jameskoster@git.wordpress.org> Co-authored-by: George Mamadashvili <georgemamadashvili@gmail.com> Co-authored-by: Mamaduka <mamaduka@git.wordpress.org> Co-authored-by: tyxla <tyxla@git.wordpress.org> Co-authored-by: wpsoul <wpsoul@git.wordpress.org> Co-authored-by: André <583546+oandregal@users.noreply.github.com> Co-authored-by: oandregal <oandregal@git.wordpress.org> Co-authored-by: Carlos Bravo <37012961+cbravobernal@users.noreply.github.com> Co-authored-by: cbravobernal <cbravobernal@git.wordpress.org> Co-authored-by: gziolo <gziolo@git.wordpress.org> Co-authored-by: SantosGuillamot <santosguillamot@git.wordpress.org> Co-authored-by: jsnajdr <jsnajdr@git.wordpress.org> Co-authored-by: David Arenas <david.arenas@automattic.com> Co-authored-by: DAreRodz <darerodz@git.wordpress.org> Co-authored-by: luisherranz <luisherranz@git.wordpress.org> Co-authored-by: Marc-pi <mdxfr@git.wordpress.org>
I just cherry-picked this PR to the release/19.1 branch to get it included in the next release: #64876 |
* Refactor block binding utils * Add security checks Co-authored-by: cbravobernal <cbravobernal@git.wordpress.org> Co-authored-by: gziolo <gziolo@git.wordpress.org> Co-authored-by: SantosGuillamot <santosguillamot@git.wordpress.org> Co-authored-by: jsnajdr <jsnajdr@git.wordpress.org>
What?
Some code refactor following the indications in #64102 (comment)
Testing Instructions
E2E tests should work.