Extensibility: Convert all filters for components to behave like HOCs #3827

gziolo · 2017-12-06T08:29:36Z

Description

This PR tries to bring idea exercised when working on Collaborative Editing in #3741. It turned out that it would be a better idea to use Higher-Order Components approach for using filtering hook with the existing components. This seems like a more natural way of dealing with React component and is de facto a community standard. This PR also tries to address other issues mentioned in other places:

#3800 (comment):

Alternatively we change blocks hooks to operate on the global hooks instance, then simply encourage plugin authors to register their hooks prior to the wp-blocks handle being loaded.

Related: #3493 (comment)

It's going to be updated with this PR. wp-hooks is always registered before wp-blocks, so it should be possible. This change enforces plugin developer to always register a hook before the modified module is going to to be imported.

#3621 (comment)

Let's make sure to reverse these changes now that the / hooks namespacing has been published.

c9310eb addresses it.

How Has This Been Tested?

Manually, e2e and unit tests.

Types of changes

Refactoring. No visual changes. Everything should work as before.

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows has proper inline documentation.

youknowriad · 2017-12-06T08:34:20Z

blocks/block-edit/index.js

 }

-export default BlockEdit;
+export default withFilters( 'BlockEdit' )( BlockEdit );


since the filters are "global", we should think of namespacing them: core/blocks/BlockEdit. It's not allowed right now in the hooks package and it's not required for this PR I think.

We can put Blocks.BlockEdit until hooks package gets updated.

youknowriad · 2017-12-06T08:37:03Z

blocks/hooks/index.js

@@ -1,7 +1,7 @@
 /**
 * WordPress dependencies
 */
-import { createHooks } from '@wordpress/hooks';
+import * as hooks from '@wordpress/hooks';


Should we drop this and just import it when it's used (on each of the hooks files)?

We could. I need to play with unit tests, but given that they are isolated, we should be good.

youknowriad · 2017-12-06T08:43:00Z

Good work here, I like it but I think this doesn't address the filters ordering issue yet. Imagine a plugin using reusable components from the "blocks" module to add hooks, it still depends on the "blocks" module and will be executed after the "registerBlockType" calls.

I think we should move the "registerBlockType" calls to the createEditorInstance function.

gziolo · 2017-12-06T08:59:00Z

Good work here, I like it but I think this doesn't address the filters ordering issue yet. Imagine a plugin using reusable components from the "blocks" module to add hooks, it still depends on the "blocks" module and will be executed after the "registerBlockType" calls.

Good point. I didn't take into account such use case. Let's address it separately then :)

codecov · 2017-12-06T10:01:12Z

Codecov Report

Merging #3827 into master will increase coverage by 0.38%.
The diff coverage is 75%.

@@            Coverage Diff             @@
##           master    #3827      +/-   ##
==========================================
+ Coverage    37.7%   38.08%   +0.38%     
==========================================
  Files         279      279              
  Lines        6749     7183     +434     
  Branches     1228     1348     +120     
==========================================
+ Hits         2545     2736     +191     
- Misses       3541     3718     +177     
- Partials      663      729      +66

Impacted Files	Coverage Δ
blocks/api/registration.js	`100% <ø> (ø)`	⬆️
blocks/api/serializer.js	`98% <ø> (ø)`	⬆️
blocks/hooks/index.js	`100% <ø> (ø)`	⬆️
components/higher-order/with-filters/index.js	`100% <100%> (ø)`	⬆️
blocks/hooks/generated-class-name.js	`100% <100%> (ø)`	⬆️
blocks/hooks/matchers.js	`42.85% <100%> (ø)`	⬆️
element/index.js	`100% <100%> (ø)`	⬆️
blocks/block-edit/index.js	`100% <100%> (ø)`	⬆️
blocks/hooks/custom-class-name.js	`75% <66.66%> (-9.62%)`	⬇️
blocks/hooks/anchor.js	`72.22% <66.66%> (-7.78%)`	⬇️
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 798645e...58f7748. Read the comment docs.

tg-ephox · 2017-12-07T04:07:40Z

@gziolo is it a good idea to allow hooks to change the BlockEdit element arbitrarily?

How will a plug-in implementor know what element a hook will receive if each hook can return anything it chooses?

The existing use cases for hooks are either adding a prop to BlockEdit or filling a slot. Can we make these explicit instead?

…tered

gziolo · 2017-12-08T13:12:36Z

@gziolo is it a good idea to allow hooks to change the BlockEdit element arbitrarily?

How will a plug-in implementor know what element a hook will receive if each hook can return anything it chooses?

The same issue applies to all other hooks. We don't check the return value of the individual filters. I don't know how it is handled on PHP side. If the return value of the filter is validated then we should bake in a similar logic here. I feel like this is a completely different issue than what I was concerned when I proposed the changes with this PR.

The existing use cases for hooks are either adding a prop to BlockEdit or filling a slot. Can we make these explicit instead?

We were operating inside the render method of the existing <BlockEdit /> component, which was a wrapper for the original edit component registered together with a given block. By using an array, we were able to technically prepend or append the existing edit component, but we couldn't modify it because it was passed as an element with props already applied. The other issue I noticed is that you couldn't apply new props from state to the BlockEdit component, you could only compute values based on the existing props. This PR solves all of that by enforcing to always return new component from the filter. As long as plugin author will follow that rule, everything will work as expected. On the technical level, it allows to wrap, prepend, append or replace the existing edit component. It can be done by defining new component using functional representation as used in this PR. If the component extends Component class from React, then I assume, you could also extend this class to produce a different component.

gziolo · 2017-12-08T13:23:36Z

I rebased with the latest changes. Added some small refinements. I'm planning to merge it on Monday morning if there are no blockers discovered.

aduth · 2017-12-11T15:08:34Z

blocks/hooks/custom-class-name.js

-	addFilter( 'BlockEdit', 'core-custom-class-name-inspector-control', addInspectorControl );
-	addFilter( 'getSaveContent.extraProps', 'core-custom-class-name-save-props', addSaveProps );
+export default function customClassName() {
+	addFilter( 'blocks.registerBlockType', 'core/custom-class-name/attribute', addAttribute );


I don't have strong opposition, but noting that there's a few implied conventions being introduced here:

Scoping filter names by with dot-separated namespaces

Any risk of confusion with other namespacing with /?

Multiple levels of namespacing, i.e. not just core/ but also core/custom-class-name/

Slash is not allowed for hook names... we need another patch if we want to change that.

We can revisit namespaces, too. I’m fine with having only 2 first parts or open to other ideas.

aduth · 2017-12-11T15:10:07Z

blocks/hooks/index.js

-customClassName( hooks );
-generatedClassName( hooks );
-matchers( hooks );
+anchor();


Minor: Do we need to export these as functions anymore, or can they just call to addFilter when imported?

It might not play well with unit tests. Need to verify.

In theory, we could export individual functions and apply filters when importing. I will open a follow-up so we could discuss it individually.

aduth · 2017-12-11T15:12:11Z

editor/components/block-list/block.js

@@ -534,4 +533,5 @@ export default compose(
 			isLocked: !! templateLock,
 		};
 	} ),
+	withFilters( 'editor.BlockListBlock' ),


Can you explain the reason for this reordering?

I need to double check with Collaborative Editing. If it gets executed after connect, we can reuse props passed when filtering. Otherwise the get passed too late so we need to grab them using selectors twice.

Yes, I can confirm it is correct. If withFilters would be first in the compose chain it would become the outermost component, which means React would render it first. As explained above, we want to have it last in the chain to make sure we have all other HOCs applied so we can take advantage of their modifications. This is how it makes the most sense for Collaborative Editing. Let's see how it works in general. We can refine later.

aduth · 2017-12-11T15:15:26Z

How will a plug-in implementor know what element a hook will receive if each hook can return anything it chooses?

Should a plugin implementer need to know whether it is the original BlockEdit or an enhanced one?

gziolo · 2017-12-12T06:02:25Z

Let's merge it and focus on data extensibility :) We have just released 1.9.0 so that buys some time to iterate on extensibility.

gziolo added [Feature] Extensibility The ability to extend blocks or the editing experience [Type] Enhancement A suggestion for improvement. labels Dec 6, 2017

gziolo self-assigned this Dec 6, 2017

youknowriad reviewed Dec 6, 2017

View reviewed changes

gziolo force-pushed the update/component-filters-as-hoc branch from 7e3d4d4 to 58f7748 Compare December 6, 2017 10:01

This was referenced Dec 6, 2017

FIX: HOOKS: Naming is unexpected #3621

Merged

Extensibility: Introduce BlockSave which would work similar to BlockEdit #3800

Closed

Editor: Make BlockListBlock extensible #3493

Merged

gziolo requested review from aduth, mcsf and jorgefilipecosta December 6, 2017 11:07

gziolo changed the title ~~Hooks: Convert all filters for components to behave like HOCs~~ Extensibility: Convert all filters for components to behave like HOCs Dec 8, 2017

gziolo added 7 commits December 8, 2017 12:40

Component: Update withFilter HOC to support more advanced use cases

bf62d31

Element: Update logic for getWrapperDisplayName method

befeb75

Hooks: Update all namespaces to use slashes

d564276

Hooks: Convert all filters for components to behave like HOCs

81d56ad

Hooks: Prefix all hook names with blocks

eedd1ad

Hooks: Fix rerender issue with block component hooks

0ffd9ef

Editor: Update hooks integration for BlockListBlock component

7bba1db

gziolo force-pushed the update/component-filters-as-hoc branch from 6944810 to 7bba1db Compare December 8, 2017 12:08

Extensibility: Make sure all hooks are loaded before blocks are regis…

a46d71a

…tered

gziolo force-pushed the update/component-filters-as-hoc branch from ce2bbae to a46d71a Compare December 8, 2017 12:34

aduth reviewed Dec 11, 2017

View reviewed changes

This was referenced Dec 11, 2017

Footnote numbering hooks #3685

Closed

Remove inline style colors atributes from paragraph. #3722

Closed

gziolo merged commit 050ea62 into master Dec 12, 2017

gziolo deleted the update/component-filters-as-hoc branch December 12, 2017 06:02

gziolo mentioned this pull request Dec 12, 2017

Extensibility: Apply filters without function wrappers #3933

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extensibility: Convert all filters for components to behave like HOCs #3827

Extensibility: Convert all filters for components to behave like HOCs #3827

gziolo commented Dec 6, 2017 •

edited

Loading

youknowriad Dec 6, 2017

gziolo Dec 6, 2017

youknowriad Dec 6, 2017

gziolo Dec 6, 2017

youknowriad commented Dec 6, 2017 •

edited

Loading

gziolo commented Dec 6, 2017

codecov bot commented Dec 6, 2017

tg-ephox commented Dec 7, 2017 •

edited

Loading

gziolo commented Dec 8, 2017

gziolo commented Dec 8, 2017

aduth Dec 11, 2017

gziolo Dec 11, 2017

gziolo Dec 11, 2017

aduth Dec 11, 2017

gziolo Dec 11, 2017

gziolo Dec 12, 2017

aduth Dec 11, 2017

gziolo Dec 11, 2017

gziolo Dec 12, 2017

aduth commented Dec 11, 2017

gziolo commented Dec 12, 2017

Extensibility: Convert all filters for components to behave like HOCs #3827

Extensibility: Convert all filters for components to behave like HOCs #3827

Conversation

gziolo commented Dec 6, 2017 • edited Loading

Description

How Has This Been Tested?

Types of changes

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad commented Dec 6, 2017 • edited Loading

gziolo commented Dec 6, 2017

codecov bot commented Dec 6, 2017

Codecov Report

tg-ephox commented Dec 7, 2017 • edited Loading

gziolo commented Dec 8, 2017

gziolo commented Dec 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduth commented Dec 11, 2017

gziolo commented Dec 12, 2017

gziolo commented Dec 6, 2017 •

edited

Loading

youknowriad commented Dec 6, 2017 •

edited

Loading

tg-ephox commented Dec 7, 2017 •

edited

Loading