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.

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: do not use useSource hook conditionally #59403

Merged
merged 81 commits into from
Feb 28, 2024

Conversation

retrofox
Copy link
Contributor

@retrofox retrofox commented Feb 27, 2024

What?

This pull request enhances the mechanism by which block attributes are bound to external source properties. The primary aim is to prevent calling React hooks in the wrong way, in this case, conditionally.

On this PR:

  • Component Introduction: Two new components, <BlockBindingBridge /> and <BlockBindingConnector />, have been introduced. These components isolate and encapsulate connecting block attributes with external source properties.

  • Hook Invocation Improvement: To circumvent the issue of incorrect hook invocation within a loop, the source object is now passed down to the <BlockBindingConnector />. This ensures that the hook is called at the top level of the component, aligning with React's best practices.

  • API Modification: The useSource() API has changed. Previously, a useValue method was provided, but it has been removed in favor of directly exposing the source property's value and its handler through value and updateValue props, respectively. The updated API structure is as follows:

export default {
  name: 'namespace/source-handler',
  label: 'The custom namespace source-handler',
  useSource: (blockProps, sourceArgs) => {
    // Logic to fetch value
    const value = getFromWherever();
    const updateValue = () => {};
    // Expose value and its updater
    return {
      placeholder,
      value,
      updateValue,
    };
  }
};

Why?

Because calling the hook wrongly is not recommended. It could bring performance issues.

How?

This PR addresses the issue by introducing and components to streamline the connection between block attributes and external sources, thereby preventing incorrect hook calls. It refines the useSource() API to directly expose the source property's value.

Testing Instructions

Pretty similar to #58085

  1. Register a new custom field however you prefer. You can use a snippet similar to this:
register_meta(
	'post',
	'text_custom_field',
	array(
		'show_in_rest' => true,
		'single'       => true,
		'type'         => 'string',
		'default'	   => 'This is the content of the text custom field',
	)
);
register_meta(
	'post',
	'url_custom_field',
	array(
		'show_in_rest' => true,
		'single'       => true,
		'type'         => 'string',
		'default'	   => 'https://wpmovies.dev/wp-content/uploads/2023/04/goncharov-poster-original-1-682x1024.jpeg',
	)
);

Test paragraph

  1. Add a paragraph with the content connected to a custom field:
<!-- wp:paragraph {"metadata":{"bindings":{"content":{"source":"core/post-meta","args":{"key":"text_custom_field"}}}}} -->
<p>Value provided by the source (core/post-meta)</p>
<!-- /wp:paragraph -->
  1. Confirm it isn't possible to edit the block content.
  2. Confirm that if the meta text_custom_field value is defined ((001) Meta value in my testing block), then it should be shown in the UI:
image

@retrofox retrofox added [Type] Performance Related to performance efforts Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta [Feature] Custom Fields Anything related to the custom fields project - connecting block attributes and dynamic values labels Feb 28, 2024
@gziolo gziolo added the [Package] Block editor /packages/block-editor label Feb 28, 2024
Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

Code-wise it is looking good enough to include in WP 6.5 RC1. I will do some testing to ensure it all works as expected.

@SantosGuillamot
Copy link
Contributor

Not 100% related to this pull request, but it will affect once this other one gets merged.

There is an issue with the fallback value that is solved by this refactoring. However, we will need to update the e2e tests after the mentioned PR is merged. We'll have to update these lines to have "fallback value": line 1, line 2, & line 3.

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

Everything tests well for me, and I also see that all e2e tests on CI pass which means we should be good to go.

There is still a tiny issue that is also present in trunk that I hoped this PR would also resolve. However, I prefer we explore it separately. I included a screencast with CPU throttling enabled at 6x, but it might still need watching the video at slower pace:

Screen.Recording.2024-02-28.at.12.51.50.mov

That might be specific to the Query Loop block and its implementation. We can observe that when changing the active post, the whole Post Template renders from scratch, and in that case, you can see the fallback value stored in HTML for blocks, which gets replaced in the next render with the value coming from the source.

The great news is that the fallback loaded from the saved content is never replaced with the value from the source, which is the intended behavior I advocated to have in the WP 6.5 release.

@SantosGuillamot
Copy link
Contributor

As explained in this comment, I've updated the tests in this commit now that the mentioned PR has been merged.

@SantosGuillamot
Copy link
Contributor

There is still a tiny issue that is also present in trunk that I hoped this PR would also resolve. However, I prefer we explore it separately.

I have just pushed this commit that should solve the blinking effect. At least until we figure out a better solution.

@retrofox retrofox merged commit faf60a4 into trunk Feb 28, 2024
59 of 60 checks passed
@retrofox retrofox deleted the update/do-not-conditional-hook-in-binding branch February 28, 2024 15:56
@github-actions github-actions bot added this to the Gutenberg 17.9 milestone Feb 28, 2024
youknowriad pushed a commit that referenced this pull request Mar 4, 2024
* replace use-binding-attributes with block-binding-support

* minor enhancement

* minor change

* tweak

* do not import use-binding-attributes

* use isItPossibleToBindBlock() helper

* introduce core/entity source handler

* rename folder

* rename source name

* polish post-entity source handler

* make core/post-entity more consistent with core-data

* make entity source hand;ler more generic

* fix entity sour handl;er issues

* remove uneeded useValue () hook (crossfingers)

* minor jsdoc improvement

* clean

* rename with updateValue()

* remove core/entity binding source handler

* move useSource to Connector cmp

* move the whole dryining logic to the Connect component

* improve jsdoc

* rename to blockProps

* minor jsdoc improvements

* use a single effect to update attr and value

* discard useValue. Return value and setValue instead

* check wheter updateValue function is defined

* check prop value is defined when updating attr

* handle `placerholder`

* ensure to put attribute in sync when onmount

* remove // eslint comment

* enable editing for bound with post-meta

* move block bindiung processor to hooks/

* ensure update bound attr once when mounting

* Update packages/block-editor/src/hooks/block-binding-support/index.js

Co-authored-by: Michal <[email protected]>

* disable editing block attribute

* move changes to the use-binding-attributes file

* introduce BlockBindingBridge component

* update isItPossibleToBindBlock() import path

* introduce hasPossibleBlockBinding() helper

* use hooks API to extened blocks with bound attts

* fix propagating attr value. jsdoc

* minor changes

* minor code enhancement

* not edit bound prop for now

* jsdoc

* revert using hooks API to extrend block

* jsdoc

* update internal path

* rollback hook utils chnages

* tidy

* wrap Connector instances with a Fragment

* return original Edit instance when no bindings

* check whether useSource is defined

* Use `useSelect` and move it out of the for loop

* check attr value type

* iterare when creating BindingConnector instances

* rename helper functions

* use useSelect to get binding sources

* Update packages/block-editor/src/hooks/use-bindings-attributes.js

Co-authored-by: Michal <[email protected]>

* Update packages/block-editor/src/hooks/use-bindings-attributes.js

Co-authored-by: Michal <[email protected]>

* pass prev attr value to compare

* improve binding allowed block attributes

* sync derevied updates when updating bound attr

* improve getting attr source

* check properly bindings data

* preserve the RichTextData for block attr

* comment line just for tesrting purposes

* rebasing changes

* rollback change foir testing purposes

* change cmp prop name. improve jsdoc

* simplify checking bindins value

* use attr name as key instance

* store bound attrs values in a local state

* collect and update bound attr in a local state

* Refactor block binding functionality from e55f6bc

* pick block data from straight props

* remove conditional onPropValueChange call

* Update e2e tests

* Use `useLayoutEffect` instead of `useEffect`

---------

Co-authored-by: Michal <[email protected]>
Co-authored-by: Mario Santos <[email protected]>
@youknowriad
Copy link
Contributor

I just cherry-picked this PR to the update/packages-6.5-rc1 branch to get it included in the next release: e88f867

@youknowriad youknowriad added Backported to WP Core Pull request that has been successfully merged into WP Core and removed Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta labels Mar 4, 2024
youknowriad pushed a commit that referenced this pull request Mar 4, 2024
* replace use-binding-attributes with block-binding-support

* minor enhancement

* minor change

* tweak

* do not import use-binding-attributes

* use isItPossibleToBindBlock() helper

* introduce core/entity source handler

* rename folder

* rename source name

* polish post-entity source handler

* make core/post-entity more consistent with core-data

* make entity source hand;ler more generic

* fix entity sour handl;er issues

* remove uneeded useValue () hook (crossfingers)

* minor jsdoc improvement

* clean

* rename with updateValue()

* remove core/entity binding source handler

* move useSource to Connector cmp

* move the whole dryining logic to the Connect component

* improve jsdoc

* rename to blockProps

* minor jsdoc improvements

* use a single effect to update attr and value

* discard useValue. Return value and setValue instead

* check wheter updateValue function is defined

* check prop value is defined when updating attr

* handle `placerholder`

* ensure to put attribute in sync when onmount

* remove // eslint comment

* enable editing for bound with post-meta

* move block bindiung processor to hooks/

* ensure update bound attr once when mounting

* Update packages/block-editor/src/hooks/block-binding-support/index.js

Co-authored-by: Michal <[email protected]>

* disable editing block attribute

* move changes to the use-binding-attributes file

* introduce BlockBindingBridge component

* update isItPossibleToBindBlock() import path

* introduce hasPossibleBlockBinding() helper

* use hooks API to extened blocks with bound attts

* fix propagating attr value. jsdoc

* minor changes

* minor code enhancement

* not edit bound prop for now

* jsdoc

* revert using hooks API to extrend block

* jsdoc

* update internal path

* rollback hook utils chnages

* tidy

* wrap Connector instances with a Fragment

* return original Edit instance when no bindings

* check whether useSource is defined

* Use `useSelect` and move it out of the for loop

* check attr value type

* iterare when creating BindingConnector instances

* rename helper functions

* use useSelect to get binding sources

* Update packages/block-editor/src/hooks/use-bindings-attributes.js

Co-authored-by: Michal <[email protected]>

* Update packages/block-editor/src/hooks/use-bindings-attributes.js

Co-authored-by: Michal <[email protected]>

* pass prev attr value to compare

* improve binding allowed block attributes

* sync derevied updates when updating bound attr

* improve getting attr source

* check properly bindings data

* preserve the RichTextData for block attr

* comment line just for tesrting purposes

* rebasing changes

* rollback change foir testing purposes

* change cmp prop name. improve jsdoc

* simplify checking bindins value

* use attr name as key instance

* store bound attrs values in a local state

* collect and update bound attr in a local state

* Refactor block binding functionality from e55f6bc

* pick block data from straight props

* remove conditional onPropValueChange call

* Update e2e tests

* Use `useLayoutEffect` instead of `useEffect`

---------

Co-authored-by: Michal <[email protected]>
Co-authored-by: Mario Santos <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backported to WP Core Pull request that has been successfully merged into WP Core [Feature] Custom Fields Anything related to the custom fields project - connecting block attributes and dynamic values [Package] Block editor /packages/block-editor [Type] Performance Related to performance efforts
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants