Skip to content
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.

Sign up for GitHub

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: Don't use useEffect in the block bindings editor hook #59443

Closed
wants to merge 4 commits into from
Closed

Block bindings: Don't use useEffect in the block bindings editor hook #59443

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5928c69
Refactor use-bindings-attributes.js to improve attribute handling
michalczaplinski Feb 28, 2024
6dd46e8
Add comment to RichTextData comparison
michalczaplinski Feb 28, 2024
1ac627f
Move back the check for attribute value
michalczaplinski Feb 28, 2024
1b91609
Rename action --> newAttributes
michalczaplinski Feb 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
81 changes: 37 additions & 44 deletions packages/block-editor/src/hooks/use-bindings-attributes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
import { getBlockType, store as blocksStore } from '@wordpress/blocks';
import { createHigherOrderComponent } from '@wordpress/compose';
import { useSelect } from '@wordpress/data';
import { useLayoutEffect, useCallback, useState } from '@wordpress/element';
import { useCallback, useReducer } from '@wordpress/element';
import { addFilter } from '@wordpress/hooks';
import { RichTextData } from '@wordpress/rich-text';

Expand Down Expand Up @@ -89,11 +89,16 @@ const BindingConnector = ( {
* If the attribute is a RichTextData instance,
* (core/paragraph, core/heading, core/button, etc.)
* compare its HTML representation with the new value.
*
* To do: it looks like a workaround.
* Consider improving the attribute and metadata fields types.
*/
if ( prevAttrValue instanceof RichTextData ) {
// If the new value is also a RichTextData instance and the value is the same, bail early.
if (
newAttrValue instanceof RichTextData &&
prevAttrValue.toHTMLString() === newAttrValue.toHTMLString()
) {
return;
}

// Bail early if the Rich Text value is the same.
if ( prevAttrValue.toHTMLString() === newAttrValue ) {
return;
Expand All @@ -106,6 +111,7 @@ const BindingConnector = ( {
newAttrValue = RichTextData.fromHTMLString( newAttrValue );
}

// Bail early if the attribute value is the same.
if ( prevAttrValue === newAttrValue ) {
return;
}
Expand All @@ -115,35 +121,26 @@ const BindingConnector = ( {
[ attrName, onPropValueChange ]
);

useLayoutEffect( () => {
if ( typeof propValue !== 'undefined' ) {
updateBoundAttibute( propValue, attrValue );
} else if ( placeholder ) {
/*
* Placeholder fallback.
* If the attribute is `src` or `href`,
* a placeholder can't be used because it is not a valid url.
* Adding this workaround until
* attributes and metadata fields types are improved and include `url`.
*/
const htmlAttribute =
getBlockType( blockName ).attributes[ attrName ].attribute;

if ( htmlAttribute === 'src' || htmlAttribute === 'href' ) {
updateBoundAttibute( null );
return;
}
if ( typeof propValue !== 'undefined' ) {
updateBoundAttibute( propValue, attrValue );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it authorized in React to call "setState" in a render function? Isn't that something that can break easily or something. cc @jsnajdr

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@michalczaplinski michalczaplinski Feb 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

image

Could we ignore it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@gziolo gziolo Mar 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

} else if ( placeholder ) {
/*
* Placeholder fallback.
* If the attribute is `src` or `href`,
* a placeholder can't be used because it is not a valid url.
* Adding this workaround until
* attributes and metadata fields types are improved and include `url`.
*/
const htmlAttribute =
getBlockType( blockName ).attributes[ attrName ].attribute;

updateBoundAttibute( placeholder );
if ( htmlAttribute === 'src' || htmlAttribute === 'href' ) {
updateBoundAttibute( null, attrValue );
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unlike before, we pass the second parameter (attrValue) as well so that the function can bail here if both params are the same.

return null;
}
}, [
updateBoundAttibute,
propValue,
attrValue,
placeholder,
blockName,
attrName,
] );

updateBoundAttibute( placeholder, attrValue );
Copy link
Contributor Author

@michalczaplinski michalczaplinski Feb 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

return null;
};
Expand Down Expand Up @@ -194,18 +191,17 @@ function BlockBindingBridge( { blockProps, bindings, onPropValueChange } ) {

const withBlockBindingSupport = createHigherOrderComponent(
( BlockEdit ) => ( props ) => {
function attributeReducer( state, newAttibutes ) {
return { ...state, ...newAttibutes };
}

/*
* Collect and update the bound attributes
* in a separate state.
*/
const [ boundAttributes, setBoundAttributes ] = useState( {} );
const updateBoundAttributes = useCallback(
( newAttributes ) =>
setBoundAttributes( ( prev ) => ( {
...prev,
...newAttributes,
} ) ),
[]
const [ boundAttributes, updateBoundAttributes ] = useReducer(
attributeReducer,
props.attributes
);

/*
Expand All @@ -222,16 +218,13 @@ const withBlockBindingSupport = createHigherOrderComponent(
<>
{ Object.keys( bindings ).length > 0 && (
<BlockBindingBridge
blockProps={ props }
blockProps={ { ...props, attributes: boundAttributes } }
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of passing the block attributes we pass our local copy of them.

bindings={ bindings }
onPropValueChange={ updateBoundAttributes }
/>
) }

<BlockEdit
{ ...props }
attributes={ { ...props.attributes, ...boundAttributes } }
/>
<BlockEdit { ...props } attributes={ boundAttributes } />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@retrofox

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.

Copy link
Contributor

@retrofox retrofox Mar 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ♾️.

</>
);
},
Expand Down
Loading