Editor: Make BlockListBlock extensible #3493

gziolo · 2017-11-15T12:07:18Z

Description

This PR tries to enable another extensibility point that is required to implement Collaborative Editing as a plugin. The changes proposed here will allow to display the frozen mode for the block when it is being externally modified by one of the collaborators.

This PR introduces withFilter HOC that would simplify the process of exposing components for extensibility.

It also proposes wrapperDisplayName utility method that would remove some code duplication from the existing higher-order components.

I also moved all hooks and exposed them under wp.utils.* namespace. I'm open to suggestions here. Maybe wp.editor.* would be a better choice here, but my rationale here was that I couldn't find any reference to @wordpress/editor from blocks or components namespace. It seems like no other namespace should depend on the editor. The same applies to the case where components would depend on blocks. That's why utils felt like a better compromise. I'm happy to iterate on that. - wp.hooks it is.

How Has This Been Tested?

Manually. Added the following to verify that the component gets updated:

// to add
wp.hooks.addFilter( 'Editor.BlockItem', 'coediting/frozen-mode', ( currentElement ) => [ wp.element.createElement( 'h1', {}, 'Frozen!!!' ), currentElement ] );

// to remove
wp.hooks.removeAllFilters( 'Editor.BlockItem' );

If you add this in the JS console, you need to click on any block to refresh rendered blocks.

Screenshots (jpeg or gifs if applicable):

Editor view all <BlockListBlock /> components wrapped with the dummy text:

Checklist:

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

codecov · 2017-11-15T12:13:23Z

Codecov Report

Merging #3493 into master will decrease coverage by 1.71%.
The diff coverage is 37.93%.

@@            Coverage Diff             @@
##           master    #3493      +/-   ##
==========================================
- Coverage   37.53%   35.82%   -1.72%     
==========================================
  Files         278      269       -9     
  Lines        6724     6753      +29     
  Branches     1226     1220       -6     
==========================================
- Hits         2524     2419     -105     
- Misses       3539     3659     +120     
- Partials      661      675      +14

Impacted Files	Coverage Δ
blocks/api/serializer.js	`98.18% <ø> (+0.18%)`	⬆️
blocks/api/registration.js	`100% <ø> (ø)`	⬆️
blocks/hooks/index.js	`100% <ø> (ø)`	⬆️
editor/modes/visual-editor/block.js	`0% <0%> (ø)`
components/higher-order/with-api-data/index.js	`83.75% <100%> (-0.21%)`	⬇️
hooks/index.js	`100% <100%> (ø)`
blocks/block-edit/index.js	`100% <100%> (ø)`	⬆️
element/index.js	`100% <100%> (ø)`	⬆️
components/higher-order/with-filters/index.js	`100% <100%> (ø)`
editor/components/document-outline/index.js	`17.85% <0%> (-57.15%)`	⬇️
... and 131 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 9eb67f8...3133ba1. Read the comment docs.

gziolo · 2017-11-15T12:20:36Z

components/higher-order/with-filters/index.js

+// TODO: Use withHooks HOC from https://github.com/WordPress/gutenberg/pull/3321.
+const hooks = createHooks();
+
+export default function withFilters( hookName ) {


Unit tests would be lovely here, but I prefer to wait for #3321 before I come up with the idea how to test it :)

aduth

I like the higher-order components you've introduced here. 👍

To the question of moving hooks to utils, it touches on prior discussion in Slack meetings, #3318, and #3321 (comment) which asks the question of whether it would be more sensible to have a single global hooks registry. It is starting to seem more as though it should. In general, I'm not a fan of the "utils" naming; it risks becoming a dumping ground. It might be nice if we could reuse @wordpress/hooks (wp.hooks) for this purpose, maybe in addition to exposing the hooks factory, it also exposes its own internal instance of one? Seems it could have some downsides, but still maybe better than moving to utils.

aduth · 2017-11-17T15:13:09Z

components/higher-order/wrapper-display-name/index.js

+ */
+import { startCase } from 'lodash';
+
+export default function wrapperDisplayName( wrapperPrefix, WrappedComponent ) {


This is not really a higher-order component, since the return value from the function is a string, not the enhanced component. I think we'd want to do one of either:

Move this elsewhere to better reflect that it is not a higher-order component

Make it in-fact a higher-order component, i.e. return an enhanced component with displayName assigned

My preference is toward the latter.

True, it’s only related logic. It’s not HOC.

Just to confirm. Option 2 would be a HOC called inside another HOC that would update displaName, right?

Just to confirm. Option 2 would be a HOC called inside another HOC that would update displaName, right?

Yes. Though now that I'm thinking about it, layering components like this risks being a performance hazard (reference).

Unless we do perf optimization and structure like a HOC but behind the scenes only update displayName on the component:

const withDisplayName = ( wrapperPrefix, SourceComponent ) => ( WrappedComponent ) => { WrappedComponent.displayName = ...; return WrappedComponent; }

After some thinking, I'd rather move it to @wordpress/utils to components.js file and use it as it is.

Would it be a better fit in @wordpress/element ? utils as a dumping ground is not scalable.

Moved and documented.

aduth · 2017-11-17T15:13:44Z

editor/modes/visual-editor/block.js

@@ -411,7 +412,7 @@ class VisualEditorBlock extends Component {
 	}
 }

-export default connect(
+export default withFilters( 'VisualEditorBlock' )( connect(


As we start to add multiple higher-order components, might be nice to use flow (flowRight) to compose them.

Or expose compose as an alias of flowRight 😎

I introduced compose as part of @wordpress/element.

aduth · 2017-11-17T15:15:06Z

components/higher-order/with-filters/README.md

+
+`withFilters` is a part of [Native Gutenberg Extensibility](https://github.com/WordPress/gutenberg/issues/3330). It is also a React [higher-order component](https://facebook.github.io/react/docs/higher-order-components.html).
+
+Wrapping a component with `withFilters` provides a unique `instanceId` to serve this purpose.


Is the instance ID bit relevant to someone using this higher-order component? Is it even true (looking at the implementation)? Maybe drop this sentence.

Nope, I copied it and forgot to update ...

gziolo · 2017-11-17T20:32:13Z

I totally agree that wp.utils isn't perfect. @wordpres/hooks + wp.hooks would be nice, but it might be tricky to setup because hooks library has also @wordpress/hooks signature. Technically, we could alias it in package.json to mitigate the issue or something along those lines. Another option is to use wp.extensibility or a similar namespace. I don't feel like it should be exposed as part of the wp.editor for the reasons I described above. It seems like from design point of view wp.editor should be a top-level package that consumes all other packages.

aduth · 2017-11-18T03:08:39Z

@wordpres/hooks + wp.hooks would be nice, but it might be tricky to setup because hooks library has also @wordpress/hooks signature.

One hope I have is that all @wordpress/ packages would expose themselves on the global as equivalent wp. . More what I'm thinking is that in addition to import createHooks from '@wordpress/hooks'; that it also exports a default global shared instance via import { applyFilters } from '@wordpress/hooks';

gziolo · 2017-11-20T08:19:01Z

It looks like it was exposed in WP core for a short while the way @aduth described. See https://core.trac.wordpress.org/browser/trunk/tests/qunit/wp-includes/js/wp-hooks.js?rev=41375. @adamsilverstein can you confirm that it is going to be possible to use hooks with a shared global instance using:

wp.hooks.applyFilters( ... );
wp.hooks.addFilter( ... );

I bet this is how it was planned to work according to tests I can see here. I think we can mirror that behavior until it lands in WP core.

youknowriad · 2017-11-20T08:19:57Z

What if we couple this to a provider? Meaning we retrieve the applyFilters from context. This makes the component reusable in a generic way. The downside is "another provider".

Also, we need to make "VisualBlockEditor" extensible, but this component will be used in the future for page building, template building. We should ask the question if we want it to be extensible in the PostEditor only or in all usages and how a plugin can decide in which context (post editor, page editor) he wants to extend the component.

For example: Even if it's not true but what if the collaborative editing is meant only for the post editor?

gziolo · 2017-11-20T09:36:03Z

What if we couple this to a provider? Meaning we retrieve the applyFilters from context. This makes the component reusable in a generic way. The downside is "another provider".

This was something @aduth explored in #3321. It is another provider to maintain, which also needs to work properly with the newly introduced Attempt Recovery feature after editor explodes because of JS error.

For example: Even if it's not true but what if the collaborative editing is meant only for the post editor?

I see the collaborative editing perfectly usable everywhere, but I understand your point 😄 The thing is that it's very hard to anticipate at the moment all the possible usages. That's why I'm proposing also withFilter HOC to give us more flexibility in the future. Do you think it would be possible to expose such context with the arguments passed down to the filters? In withFilters we have the following:

return applyFilters( hookName, <WrappedComponent { ...props } />, props );

Could we pass context together with props to enabled filtering?

gziolo · 2017-11-20T09:40:51Z

Another option I can think of is to chain filters as follows:

return applyFilters(
    hookName,
    applyFilters( `${ hookName }.${ contextName }`, <WrappedComponent { ...props } />, props ),
    props
);

So you could register global filter or specific to post or page editor.

gziolo · 2017-11-20T10:54:03Z

editor/modes/visual-editor/block.js

-			dispatch( editPost( { meta } ) );
-		},
-	} )
+export default compose(


It looks pretty cool with compose in my opinion 😃

As discussed with @youknowriad on Slack, I'm happy to replace all flowRight occurrences in the existing codebase once this lands.

The other thought I had was using a higher-order component pattern for composing, like:

withComposition( [ withFoo, withBar ] )( OriginalComponent );

But I think I like the element-exposed export just as well.

gziolo · 2017-11-20T10:55:10Z

I addressed all comments. The only part left here is to make a final call for the hooks public API.

adamsilverstein · 2017-11-21T03:30:44Z

@adamsilverstein can you confirm that it is going to be possible to use hooks with a shared global instance using:

I do think it makes the most sense for core to expose hooks on the wp global and to use hooks here as wp.hooks, because core acts as a platform, and every plugin or theme will want to tie into the hooks core exposes. Individual plugins or themes might want to create their own hooks registry and I still like createHooks as a way to do that.

youknowriad · 2017-11-21T13:52:59Z

Are they published to npm?

No they are not, they can not be because they may have server dependencies and use globals

youknowriad · 2017-11-21T13:54:50Z

They just serve to build the "bundles" corresponding to the WP registered scripts

aduth · 2017-11-21T13:55:38Z

It really seems like we're going out of our way adding new concepts (which for other developers' sake should always be done with caution) just to avoid having a singleton instance from the module.

youknowriad · 2017-11-21T13:57:21Z

@aduth Fair point, but honestly I don't think we're adding a new concept, I think we're removing an ambiguity because right now, if you import @wordpress/* you do not know if it's an external dependency or not.

I'm convinced we'd have this issue more broadly later, but I'm ok postponing if necessary.

aduth · 2017-11-21T14:00:02Z

Ok, fine to have a difference of opinion. 😄 My thinking is that we should want the direct mapping of WordPress namespace to WordPress global (the latter exposed in the context of WP only), and not needing to care whether the import resolves to one or the other.

gziolo · 2017-11-29T10:41:45Z

It's ready for a final check.

In the meantime VisualEditorBlock got renamed to BlockListBlock, but I exposed it for hooks as Editor.Block. We should consider moving it to its own folder to align with that choice or pick a different name for the hook.

I found an easy way to integrate @wordpress/hooks with the current codebase using Webpack config. It is still exposed in its own build file hooks/build/index.js to avoid the case when it gets bundles into 2 different libraries wp.blocks and wp.components. If this pattern will get accepted, we can do a similar move for a11y or other packages we consume from WordPress/packages repository when their source would get duplicated in several places.

Another reason why I had to provide a build process for hooks was public API that had to be exposed under global.wp.hooks. It seemed like the easiest way.

I'd like to merge it tomorrow morning and see how it works with Collaborative Editing. Let me know if you find any blockers.

mtias · 2017-11-29T11:58:39Z

BlockListBlock is a tongue-twister. May I suggest BlockItem?

gziolo · 2017-11-29T13:02:57Z

BlockListBlock is a tongue-twister. May I suggest BlockItem?

That would make it Editor.BlockItem as the hook name. I'm fine with that.

jorgefilipecosta · 2017-11-29T13:20:11Z

Hi @gziolo, I did some tests and it looks like this works as specified. 👍 Also, the withFilters HoC is a great addition and will for sure simplify extensibility.

I just got a question that made me really curious:

The changes proposed here will allow to display the frozen mode for the block when it is being externally modified by one of the collaborators.

Would not our current extensibility point 'BlockEdit' hook, also allow this? Instead of returning the block editor we can display frozen, right? Also extending in this point we have access to setAttributes prop and other helpers that may be useful. This question is probably trivial and I'm missing something.

gziolo · 2017-11-29T13:38:39Z

Would not our current extensibility point 'BlockEdit' hook, also allow this? Instead of returning the block editor we can display frozen, right? Also extending in this point we have access to setAttributes prop and other helpers that may be useful. This question is probably trivial and I'm missing something.

Yes, you are 100% right in the majority of cases. The thing here is we want to hide all toolbars and controls when block will be frozen when in coediting mode. It's going to be wrapped with border explaining which user is editing this block and all events need to be disabled. Another thing is that user might be editing in visual or HTML mode, but we want to display always visual mode. So there is much more going on in that case. I tried to integrate with the existing hooks, but it wasn't enough because I was still able to pick this block when doing multi-select or bring focus on it. It would be a really bad experience if you could combine and transform a few paragraphs into the list when another user is still writing their paragraph.

aduth · 2017-11-30T01:50:39Z

.gitignore

@@ -2,6 +2,7 @@
 build
 coverage
 cypress
+hooks


What is this for?

We build wp.hooks file here: hooks/build/index.js as you discovered. I should mention that explicitly.

aduth · 2017-11-30T01:52:03Z

blocks/api/test/factory.js

@@ -21,6 +21,11 @@ describe( 'block factory', () => {
 		title: 'block title',
 	};

+	beforeAll( () => {
+		// Load all hooks that modify blocks
+		require( 'blocks/hooks' );


Curious, would this be something we'd want to consider moving into general test setup? Seems easy to overlook when adding new tests.

Yes, that's a valid question. I don't have an answer yet :)

My concern is that whenever we add this kind of code, it gives me an impression that we are doing integration tests rather than unit tests. Saying that, I'm wondering if creating a new integration tests configuration which loads all blocks and hooks on startup would be a good idea here. We need to come up with a wider strategy for all the different types of testing.

aduth · 2017-11-30T01:53:40Z

blocks/hooks/index.js

@@ -14,36 +14,22 @@ import matchers from './matchers';
 const hooks = createHooks();


Will we drop block-specifics hooks registry in a future PR in favor of the new singleton instance?

This would make it possible to update block related hooks using both wp.blocks.addFilter and wp.hooks.addFilter, which I wanted to avoid. Documentation promotes the former, so that's why we might keep it as it is. I'm open for discussion about this one. It seems not consistent to register block with wp.blocks.registerBlockType, but update it with wp.hooks.addFilter. Hope it helps to understand the decision process here.

It looks like we are dropping block-specific hooks with #3827.

aduth · 2017-11-30T01:57:00Z

element/index.js

+ * @param {String} wrapperName Wrapper name to prepend to the display name.
+ * @return {String}            Wrapped display name.
+ */
+export function wrapperDisplayName( BaseComponent, wrapperName ) {


Minor: Something about the function name I'm not a fan of, doesn't really indicate what it's for; wondering if it's useful enough a utility to simply retrieve a usable component name i.e. getDisplayName and concatenate manually when we need to; otherwise maybe getWrapperDisplayName ?

Let's call it getWrapperDisplayName and revisit later if necessary.

aduth · 2017-11-30T02:01:04Z

webpack.config.js

+			return memo;
+		}, {} ),
+		packageNames.reduce( ( memo, packageName ) => {
+			memo[ packageName ] = `./node_modules/@wordpress/${ packageName }`;


Okay, I think I understand the gitignore change now, if we're creating a custom build for hooks to expose the wp.hooks global. I think there's an issue though, that it's not whitelisted to be included in the plugin zip (see build-plugin-zip.sh). I think we have a few of these "list of all top-level folders" scattered throughout the codebase that might need to be audited as well.

Yes, I will add it to the whitelist. It seems like this is the only place to update.

…mponents

…Item`

gziolo · 2017-11-30T13:11:01Z

Let's move on with this one and revisit next week if it works well for us.

gziolo added [Feature] Extensibility The ability to extend blocks or the editing experience [Status] In Progress Tracking issues with work in progress labels Nov 15, 2017

gziolo self-assigned this Nov 15, 2017

gziolo commented Nov 15, 2017

View reviewed changes

gziolo force-pushed the add/visual-editor-block-filtering branch from 51c4648 to e2a61f5 Compare November 15, 2017 12:22

gziolo requested review from aduth and youknowriad November 15, 2017 12:24

gziolo force-pushed the add/visual-editor-block-filtering branch from e2a61f5 to 4806455 Compare November 17, 2017 12:55

gziolo requested a review from mcsf November 17, 2017 13:17

gziolo force-pushed the add/visual-editor-block-filtering branch from 4806455 to c7ce65a Compare November 17, 2017 13:28

gziolo added Needs Tests and removed [Status] In Progress Tracking issues with work in progress labels Nov 17, 2017

gziolo changed the title ~~[WIP] Editor: Make VisualEditorBlock extensible~~ Editor: Make VisualEditorBlock extensible Nov 17, 2017

gziolo requested a review from mtias November 17, 2017 14:19

aduth reviewed Nov 17, 2017

View reviewed changes

gziolo requested review from jorgefilipecosta and ellatrix November 17, 2017 20:32

gziolo commented Nov 20, 2017

View reviewed changes

gziolo requested a review from adamsilverstein November 20, 2017 11:53

gziolo mentioned this pull request Nov 20, 2017

Documentation: Bootstrapping the Extensibility API docs #3567

Merged

mtias mentioned this pull request Nov 20, 2017

Advanced settings in blocks #3002

Closed

gziolo mentioned this pull request Nov 24, 2017

Hooks: Update hooks public API to make it possible to apply to wp.hooks directly WordPress/packages#45

Merged

gziolo changed the title ~~Editor: Make VisualEditorBlock extensible~~ Editor: Make BlockListBlock extensible Nov 29, 2017

gziolo force-pushed the add/visual-editor-block-filtering branch from a61a944 to 67ce3cc Compare November 29, 2017 10:16

aduth reviewed Nov 30, 2017

View reviewed changes

gziolo added 7 commits November 30, 2017 13:41

Editor: Make VisualEditorBlock extensible

ad3ba73

Extensibility: Refactor hooks to make them reusable for editor and co…

af37f00

…mponents

Extensibility: Add tests for withFilters HOC

916a149

Components: Move wrapperDisplayName to elements namespace

cc30711

Editor: Refactor VisualEditorBlock to use compose utility for HOCs

de37e5e

Blocks: Expose blocks scoped hooks

3abbf91

Element: Update name for wrapper display name utility

f5af472

gziolo force-pushed the add/visual-editor-block-filtering branch from 67ce3cc to f5af472 Compare November 30, 2017 12:52

Editor: Expose the hook for BlockListBLock component as `Editor.BLock…

3133ba1

…Item`

gziolo merged commit aab6f93 into master Nov 30, 2017

gziolo deleted the add/visual-editor-block-filtering branch November 30, 2017 13:09

aduth mentioned this pull request Dec 5, 2017

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

Closed

This was referenced Dec 6, 2017

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

Merged

Framework: Replace flowRight occurences with compose for HOCs #3907

Merged

jorgefilipecosta mentioned this pull request Dec 22, 2017

Sibling inserter: Fix the inserter position for floated blocks #4146

Merged


		`withFilters` is a part of [Native Gutenberg Extensibility](https://github.com/WordPress/gutenberg/issues/3330). It is also a React [higher-order component](https://facebook.github.io/react/docs/higher-order-components.html).

		Wrapping a component with `withFilters` provides a unique `instanceId` to serve this purpose.

		@@ -14,36 +14,22 @@ import matchers from './matchers';
		const hooks = createHooks();

Editor: Make BlockListBlock extensible #3493

Editor: Make BlockListBlock extensible #3493

Conversation

gziolo commented Nov 15, 2017 • edited Loading

Description

How Has This Been Tested?

Screenshots (jpeg or gifs if applicable):

Checklist:

codecov bot commented Nov 15, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

aduth left a 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

gziolo Nov 17, 2017 • edited Loading

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

Choose a reason for hiding this comment

gziolo commented Nov 17, 2017

aduth commented Nov 18, 2017

gziolo commented Nov 20, 2017

youknowriad commented Nov 20, 2017

gziolo commented Nov 20, 2017

gziolo commented Nov 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo commented Nov 20, 2017

adamsilverstein commented Nov 21, 2017

youknowriad commented Nov 21, 2017

youknowriad commented Nov 21, 2017 • edited Loading

aduth commented Nov 21, 2017

youknowriad commented Nov 21, 2017

aduth commented Nov 21, 2017

gziolo commented Nov 29, 2017

mtias commented Nov 29, 2017

gziolo commented Nov 29, 2017

jorgefilipecosta commented Nov 29, 2017

gziolo commented Nov 29, 2017 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo commented Nov 30, 2017

gziolo commented Nov 15, 2017 •

edited

Loading

codecov bot commented Nov 15, 2017 •

edited

Loading

gziolo Nov 17, 2017 •

edited

Loading

gziolo commented Nov 20, 2017 •

edited

Loading

youknowriad commented Nov 21, 2017 •

edited

Loading

gziolo commented Nov 29, 2017 •

edited

Loading