Block bindings: Don't use useEffect in the block bindings editor hook
#59443
Conversation
michalczaplinski
added
[Type] Enhancement
michalczaplinski
changed the title
| @@ -222,16 +216,13 @@ const withBlockBindingSupport = createHigherOrderComponent( | |||
| <> | |||
| { Object.keys( bindings ).length > 0 && ( | |||
| <BlockBindingBridge | |||
| blockProps={ props } | |||
| blockProps={ { ...props, attributes: boundAttributes } } | |||
There was a problem hiding this comment.
Instead of passing the block attributes we pass our local copy of them.
|
|
||
| updateBoundAttibute( placeholder ); | ||
| if ( htmlAttribute === 'src' || htmlAttribute === 'href' ) { | ||
| updateBoundAttibute( null, attrValue ); |
There was a problem hiding this comment.
Unlike before, we pass the second parameter (attrValue) as well so that the function can bail here if both params are the same.
| attrName, | ||
| ] ); | ||
|
|
||
| updateBoundAttibute( placeholder, attrValue ); |
There was a problem hiding this comment.
Same as the other comment:
Unlike before, we pass the second parameter (attrValue) as well so that the function can bail here if both params are the same.
michalczaplinski
force-pushed
the
update/dont-use-useeffect-block-bindings
branch
from
f239e20
to
1b91609
Compare
|
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: +8 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
michalczaplinski
requested review from
youknowriad,
gziolo,
artemiomorales and
SantosGuillamot
There was a problem hiding this comment.
My initial thinking about illuminating useEffect was that useState would also be removed and instead be replaced with computed values (useMemo or things like that). I know the fact the API uses hooks make this harder but I wonder if there are some creative ways to achieve it still (potentially changing the useSource API.
| return; | ||
| } | ||
| if ( typeof propValue !== 'undefined' ) { | ||
| updateBoundAttibute( propValue, attrValue ); |
There was a problem hiding this comment.
Is it authorized in React to call "setState" in a render function? Isn't that something that can break easily or something. cc @jsnajdr
There was a problem hiding this comment.
Yes, it's possible and legal, you just need to be careful about not triggering infinite loops.
setState in a render function is how you can implement getDerivedStateFromProps in a functional component. If such a setState call happens, the output of the render function is guaranteed to be never committed to DOM (it's using props and state that are out of sync) and a second rerender is scheduled to be done immediately.
React FAQ mentions this technique for functional getDerivedStateFromProps here: https://legacy.reactjs.org/docs/hooks-faq.html#how-do-i-implement-getderivedstatefromprops. Note how the setState functions are called directly in the ScrollView function, without any useEffect.
There's also a link to a "you probably don't need derived state" article that describes some alternatives. But I think none of them match our case, because we're not working with local component state, but with a state in an external data store. Data store, when updated, also triggers a setState-ish operation being done on your component, so the process is similar, but not identical.
There was a problem hiding this comment.
I think here we're working with normal setState it's just that it's stored in one component that is one level up in the tree (because of the need to respect the rules of hooks). So it seems this solution is legit. Thanks Jarda.
There was a problem hiding this comment.
Just to add to this: yes this is a legit react pattern for exactly the reason that Jarda explained: https://react.dev/learn/you-might-not-need-an-effect#adjusting-some-state-when-a-prop-changes:~:text=adjust%20the%20state%20directly%20during%20rendering%3A
While we are indeed using an external value (from a custom field) here, we're not setting it in store in any way. So this approach should be fine.
That said, once we start implementing the ability to edit values (after 6.5), we'd have to use useEffect() somewhere because we'd be setting a value in an external store.
There was a problem hiding this comment.
I see the Warning about rendering update the component state when rendering others:
react-dom.js?ver=18:73 Warning: Cannot update a component (`WithBlockBindingSupport(ParagraphBlock)`) while rendering a different component (`BindingConnector`). To locate the bad setState() call inside `BindingConnector`, follow the stack trace as described in https://reactjs.org/link/setstate-in-render
Could we ignore it?
There was a problem hiding this comment.
we're not setting it in store in any way. So this approach should be fine.
Is this something that we do not want to do at all?
There was a problem hiding this comment.
By the way, I have some doubts whether the data bindings should be implemented in React at all, with effects synchronizing stuff back and forth. Maybe it belongs somewhere to the core/block-editor store instead.
I just wrote down an issue (#59592) about how Gutenberg blocks implement a lot of functionality in the "view" layer, instead of doing it in the "model" layer (in MVC terminology). Inspired by the obstacles I hit when trying to lazy load the edit UI.
I know very little about how exactly block bindings work, but on the surface the React code is "suspicious" 🙂 Let me know if it makes sense for you.
There was a problem hiding this comment.
Yes, that's a grat discussion to have. Thank you for opening the issue and flagging it here 👍🏻
@kevin940726 and @talldan had several prototypes for Pattern Overrides that were trying to use the local copy of the core/block-editor store with some actions and selectors patched so everything works out of the box in React components. The biggest challenge when replicating the same idea on Block Bindings would be that for every selector run that returns the block attributes, there would be need to cross-check if the metadata.bindings is set and fetching the data from different sources. It is doable, but the concern would be the performance. I would be very happy to see that prototyped directly in core/block-editor store so we could measure the impact.
| { ...props } | ||
| attributes={ { ...props.attributes, ...boundAttributes } } | ||
| /> | ||
| <BlockEdit { ...props } attributes={ boundAttributes } /> |
There was a problem hiding this comment.
This is a dangerous change. By doing this, we would be harcoding the assignment of attributes for all blocks. It works only in the first renders because the reducer takes the block attributes value when initialized.
If, for example, you try to update the attributes of any block by dispatching the updateBlockAttributes action, it will fail. Once the block is rendered, the new changes in the store will not be reflected in the component.
Keep in mind that boundAttributes only should contain that: attributes that are bound to an external source. Probably, we should add a note.
There was a problem hiding this comment.
You're right. This pattern won't work and should not be used. Thanks for pointing it out 🙂
I'll have to think about whether avoiding useEffect() can be made to work at all then.
There was a problem hiding this comment.
So, guess that we'd need something like this:
diff --git a/packages/block-editor/src/hooks/use-bindings-attributes.js b/packages/block-editor/src/hooks/use-bindings-attributes.js
index b2bc7bdc9f..e5a757d674 100644
--- a/packages/block-editor/src/hooks/use-bindings-attributes.js
+++ b/packages/block-editor/src/hooks/use-bindings-attributes.js
@@ -204,6 +208,11 @@ const withBlockBindingSupport = createHigherOrderComponent(
props.attributes
);
+ // Update the boundAttributes variable when the block attributes change.
+ useEffect( () => {
+ updateBoundAttributes( props.attributes );
+ }, [ props.attributes ] );
+
/*
* Create binding object filtering
* only the attributes that can be bound.
But that would defeat the purpose of this PR 🙂
Alternatively, we could technically use the same pattern of "adjust the state directly during rendering" like:
diff --git a/packages/block-editor/src/hooks/use-bindings-attributes.js b/packages/block-editor/src/hooks/use-bindings-attributes.js
index b2bc7bdc9f..3732a799d9 100644
--- a/packages/block-editor/src/hooks/use-bindings-attributes.js
+++ b/packages/block-editor/src/hooks/use-bindings-attributes.js
@@ -204,6 +204,10 @@ const withBlockBindingSupport = createHigherOrderComponent(
props.attributes
);
+ if ( props.attributes !== boundAttributes ) {
+ updateBoundAttributes( props.attributes );
+ }
+
/*
* Create binding object filtering
* only the attributes that can be bound.but I'm not 100% sure if this is safe either.
There was a problem hiding this comment.
As mentioned before, handling the data propagation relying on the component makes the approach fragile. It'll get tougher when we introduce the ability to edit the property value of the external source according to attribute changes.
The riskiest is getting out of sync and/or also especially falling into an infinite loop ♾️.
|
This approach has been superseded by #60724 so the current PR can be closed. |
This is an enhancement to #59403 which avoids using
useEffect()to sync the block attributes with custom field values.As discussed in #59403 (comment)
Why?
Using a
useEffect()to synchronize the value from the custom field with the attribute means that on the initial render (before the effect runs) the value rendered in the editor will be incorrect (not coming from the custom field).It's also marginally better for performance.
How ?
We can update the local state directly in the render function. This is a recognized React pattern. We just have to be careful not to introduce an infinite loop.
Testing
Register a custom field e.g:
Add a connected block, e.g.:
Check that the editor displays the value from the custom field.
Try out other blocks: Button, Image and using
srcorurlattributes.Try connecting blocks that should not allow being connected, e.g.:
PullquoteorCode(using instructions from step 2.) and verify that there are no errors.Try connecting attributes that should not allow being connected (using instructions from 2.) and verify that there are no errors.