Block Bindings: Move logic to core functions instead of using a hook #61817
Conversation
SantosGuillamot
added
[Type] Enhancement
|
Size Change: -27 B (0%) Total Size: 1.75 MB
ℹ️ View Unchanged
|
|
Yes, so far this is what I had in mind :) |
|
It seems there are (at least) two problems with the current refactoring: 1. Pattern inner blocks are not updated properly when parent block attributes changeWhen I modify a pattern value and I click on Reset, the attribute "content" of the parent is changed correctly, but the inner blocks are not refreshed. I made a quick video to show what I mean: Edit.Page.0.Test.pattern.custom-fields.WordPress.-.21.May.2024.mp42. Tests with blocks with bindings using
|
| getValue( { select, clientId, attributeName } ) { | ||
| const { getBlock, getBlockParentsByBlockName } = | ||
| select( blockEditorStore ); | ||
| const { attributes: currentBlockAttributes } = getBlock( clientId ); |
There was a problem hiding this comment.
Why use getBlock? That does not contain the attributes right? We'll somehow have to make sure it's not recursive, or pass the attributes to this function.
There was a problem hiding this comment.
Why use getBlock? That does not contain the attributes right?
From my tests, it seems it actually includes the attributes. I decided to use that because calling getBlockAttributes causes an infinite loop. Maybe it is better to pass the attributes as an argument or prevent the loop, as you suggest. Any preference?
There was a problem hiding this comment.
I think somehow we'll have to make getBlockAttributes work, someone might decide to use it and not know it's not allowed. Also, we might have to filter getBlock as well at some point?
There was a problem hiding this comment.
Yes, I think you're right. The simplest solution that come to my mind is having an external isProcessingBindings variable which by default is false, it is set to true when bindings processing starts, and it is set to false when bindings processing finishes. Could something like that be enough? I quickly tested it and it seems to work but I don't know if there are any implications.
we might have to filter getBlock as well at some point?
Mmm, I guess so.
There was a problem hiding this comment.
I've tried to make it work with getBlockAttributes with this logic. While it works, I am not sure if it is the best approach. Could something like that work or should I look for a better solution?
| continue; | ||
| } | ||
|
|
||
| const context = {}; |
There was a problem hiding this comment.
Yes. I added a TODO comment at this point.
|
I think number (2) was the reason for not doing this earlier. This approach also "modifies" the original attribute value. |
It doesn't seem to be an issue for existing blocks with bindings or when bindings are added manually, but it seems it is broken when using |
|
A quick summary of the challenges we need to solve before this pull request could be merged: 1. How to access the block contextRight now, we are passing the block context needed by the sources in the existing hook. However, it isn't available in this new implementation while using the store. In this Slack conversation, Ella suggested syncing the context to the store, but it needs to be discussed if that's the right approach. EDIT: We started exploring this in this pull request. 2. Pattern inner blocks are not updated properly when parent block attributes changeWhen I modify a pattern value and I click on Reset, the attribute "content" of the parent is changed correctly, but the inner blocks are not refreshed. I made a quick video to show what I mean: Edit.Page.0.Test.pattern.custom-fields.WordPress.-.21.May.2024.mp43. Inserting blocks with bindings with
|
|
It would be good to have a list of the cases where we do NOT want bindings in block attributes, and a list where we DO. |
Until now, I would say that we wanted the bindings in all cases but in the save, because we didn't want to save the bindings value in the database. But I don't have a clear picture of all the uses cases where block attributes are used. |
SantosGuillamot
force-pushed
the
try/move-bindings-logic-outside-hook
branch
from
ed11e1a
to
3165915
Compare
SantosGuillamot
changed the base branch from
trunk
to
try/sync-block-context-in-store
I guess that's something we really need to understand before continuing imo. Maybe in certain React contexts it shows the sourced value? Idk. Maybe we should also think about how Bits would work, it should probably be very similar? |
|
I believe I made good progress and "solve" most of the issues to make it work as it is working right now, plus supporting functions like After rebasing the branch to the one where we are syncing the context with the store, I made some changes and fixed the bugs I encountered. Let's wait to see if tests pass (they passed locally), but if so I guess the first step is to decide if the syncing context is the right approach.
I agree we need to work on that. On the other hand, it is an issue with the current implementation as well because we are hooking in the edit. I feel this pull request would be more reliable and we can build on top of it. |
|
Flaky tests detected in 629056c. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/9266460320
|
This reverts commit 0e91129.
SantosGuillamot
force-pushed
the
try/move-bindings-logic-outside-hook
branch
from
c8fd783
to
629056c
Compare
|
This one needs to be revisited and potentially closed. My impression is that we have everything in place for Block Bindings to work efficiently on the client, but I don't remember exactly what was explored here, so I'm happy to discuss the opportunity. |
gziolo
added
the
[Status] Stale
|
We were exploring the possibility of directly returning the "bindings value" in I'm closing this pull request based on that. |
What?
Move the bindings logic to get and update the value of the connected source to
getBlockAttributesandupdateBlockAttributesinstead of having a custom hook.Why?
This is part of core and it is where it belongs. Apart from that, it ensure it works as expected in all the uses cases and not only when getting the attributes from the props and updating them with
setAttributes.How?
getValue, hook into thegetBlockAttributeselector to read the bindings.setValue, instead of updating the attribute itself, hook into theupdateBlockAttributesaction.This last step requires changing the implementation to use
dispatch, so I had to modify the e2e tests.Testing Instructions
e2e tests passing should be enough. If you want to test it manually you can:
1. Register post meta by adding this snippet to your theme's functions.php
2. Add a paragraph block bound to the custom field using the Code Editor