Try: Extensibility: Define anchor behavior as filtered blocks support #3318

aduth · 2017-11-02T19:10:37Z

This pull request seeks to explore an extensibility pattern for block settings, initially targeting one of Gutenberg's own extension points: block anchors. Other candidates include className and supportHTML property, with the idea being that this could develop into a general pattern for plugin authors to extend block behaviors.

Implementation notes:

These changes make use of @wordpress/[email protected] to add a new hooks registry to the blocks default export, available at wp.blocks.addFilter. The behavior here of adding a hooks registry specific to blocks (vs. a global registry) is not settled, but enabled as part of changes introduced to the hooks module in WordPress/packages#40.

With these changes, a registerBlockType filter is fired before any block completes registration. Any filters may inspect and mutate the settings before the block is registered. The anchor supports behavior uses this opportunity to:

Add a new anchor attribute
Include a new <InspectorControls /> in the edit form for managing the anchor value
Overriding serialization to inject the id attribute at save time

As implemented, this is not a faithful adaptation. The main advantage is that extended behavior is now better isolated, vs. a previous ad-hoc solution which touched block parsing, factory, serialization, block API, and editor UI. This should prove to be more maintainable and scalable, keeping generalized behavior unaware to the existence of extensions.

There are a few compromises or open questions:

Previously the anchor inspector control would display a ClipboardButton to copy a link to the post at the specified anchor. If these are meant to be generic block behaviors, we need to decide if it makes sense for the anchor extension to be aware of post state, and how it should access these values.
If there is not a single hooks registry but instead module-specific hooks, we need to decide how to draw the line between block and editor hooks, and if, taking anchor as an example, it is reasonable to expect that core or plugin extensions should need to hook to separate module hook registries.
Overriding edit and save behavior is made challenging by the facts that (a) edit can be defined as a function or a Component class, (b) save is optional and falls back to the edit implementation, (c) save can return either a string or an element. For each of these, the complexity of the logic is managed in core behaviors, but exposing these as extensibility points means that the plugin authors must accommodate for these themselves. To the previous point, it might be that we want separate actions or filters to occur in overriding the specific edit and save behaviors (rather than having the extender override the save and edit properties)

To-Do:

Restore anchor tests

Testing instructions:

Verify that when inserting a Heading block, you have the option via Inspector controls to add an anchor. After adding an anchor, saving the post, and refreshing the page, you should note that the anchor value is preserved, and that the id attribute exists in the saved markup.

codecov · 2017-11-02T19:10:40Z

Codecov Report

Merging #3318 into master will increase coverage by 0.14%.
The diff coverage is 82.35%.

@@            Coverage Diff             @@
##           master    #3318      +/-   ##
==========================================
+ Coverage   33.92%   34.07%   +0.14%     
==========================================
  Files         254      257       +3     
  Lines        6729     6753      +24     
  Branches     1219     1226       +7     
==========================================
+ Hits         2283     2301      +18     
- Misses       3751     3754       +3     
- Partials      695      698       +3

Impacted Files	Coverage Δ
blocks/api/parser.js	`98.14% <ø> (-0.07%)`	⬇️
blocks/library/heading/index.js	`28.57% <ø> (ø)`	⬆️
blocks/api/factory.js	`87.17% <ø> (-0.63%)`	⬇️
editor/modes/visual-editor/block.js	`0% <0%> (ø)`	⬆️
...or/components/block-inspector/advanced-controls.js	`0% <0%> (-4.77%)`	⬇️
blocks/block-edit/index.js	`100% <100%> (ø)`
blocks/api/serializer.js	`98.18% <100%> (-0.07%)`	⬇️
blocks/hooks/index.js	`100% <100%> (ø)`
blocks/api/registration.js	`100% <100%> (ø)`	⬆️
blocks/hooks/anchor.js	`80% <80%> (ø)`
... and 5 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 e0b29b5...7f3a223. Read the comment docs.

youknowriad

I like how this is shaping up.

To your point

Overriding edit and save behavior is made challenging

Do you think it would be a good idea to provide "helpers" as decorators.

   const newSettings = flow( [
    addInspectorControl( props => control ),
    addToolbarControl( props => control ),
    addContainer( { children } => <div>{ children }</div> ),
    extendSaveContainer( props => extraContainerProps )
   ] )( settings )

youknowriad · 2017-11-03T08:40:52Z

blocks/api/registration.js

 		name,
 		attributes: get( window._wpBlocksAttributes, name ),
 		...settings,
 	};

-	return block;
+	settings = applyFilters( 'registerBlockType', settings, name );


Should we revert the arguments to match the registerBlockType function signature? I know it's not convenient for the anchor filter because the name is useless.

It's awkward, I agree, but the behavior of filtering is such that we need the second argument to be the original value we expect to be filtered; in this case, the settings. All additional arguments are considered "extra", and in this case the name could prove valuable and is not otherwise made available.

See: https://developer.wordpress.org/reference/functions/apply_filters/

youknowriad · 2017-11-03T08:41:41Z

blocks/hooks/anchor.js

+	}
+
+	// Extend attributes with anchor determined by ID on the first node.
+	assign( settings.attributes, {


Should we avoid to edit the function parameter, Should we make this pure instead?

This sounds like something that would be difficult to enforce, and would have questionable value if not enforced consistently. If it could be enforced, I could potentially see some value in strict reference comparisons on before/after a filter is applied (similar to what we see in how Redux reducer values are used).

As implemented here, we get some performance gains from mutating the object directly rather than first creating a shallow clone. I think it's reasonable to consider the filtering as an extension of the internal behavior of registerBlockType, in which we have no qualms to mutate the settings object directly.

I could go either way personally, but the immediate value isn't obvious to me.

youknowriad · 2017-11-03T08:43:36Z

docs/block-api.md


-Anchors let you link directly to a specific block on a page. This property adds a field to define an id for the block and a button to copy the direct link.
+- `supportAnchor` (default `false`): Anchors let you link directly to a specific block on a page. This property adds a field to define an id for the block and a button to copy the direct link.


should this be anchor instead of supportAnchor?

youknowriad · 2017-11-03T08:45:15Z

blocks/hooks/anchor.js

+	};
+
+	return settings;
+}


What your thoughts on unit testing this, is it possible, simple enough?

Yes, I should plan to write some tests here.

youknowriad · 2017-11-03T13:58:33Z

Btw, I'm working on #2541 locally and the work being done here to simplify the parser and the serializer would really help when generalized to all the advanced attributes (custom className and also the generated classNamee).

aduth · 2017-11-03T16:14:49Z

Do you think it would be a good idea to provide "helpers" as decorators.

That's a neat idea to help abstract some of the more complex common behaviors!

aduth · 2017-11-08T15:22:46Z

I've rebased and taken a revised approach to try to avoid concerns around block implementers needing to handle all the conditions which can occur in extending a block (e.g. save might be a function or a component, and a function returning an element or a markup string). The idea is that by applying filters more granularly (i.e. at serialization time, in addition to registration time), a plugin extender can be confident that a filter would only be fired if all prerequisites are already met (e.g. the getSaveContent.extraProps only applies if we know that save is implemented as a component returning an element).

There's not too many downsides, but a few potential concerns around:

How many hooks (entry points) we expose and how consistently we do so
Potential performance impact of firing potentially unhandled filters and actions
Naming and use-cases for filters (naming conventions, etc)
- See: registerBlock (always firing with registered block settings), BlockEdit (overriding the element return value of a component), getSaveContent.extraProps (a conditionally-met "nested" filter)
Testing filters is not always straight-forward, because filterHandler( originalValue ) does not necessarily have the same result as applyFilters( 'handled-filter', originalValue ). Also, there is risk of memory leaks if hooks are not properly disposed.

gziolo · 2017-11-13T14:03:15Z

blocks/api/serializer.js

@@ -55,7 +56,7 @@ export function getSaveContent( blockType, attributes ) {
 			return element;
 		}

-		const extraProps = {};
+		const extraProps = applyFilters( 'getSaveContent.extraProps', {}, blockType, attributes );


This one seems to be a very specific. Maybe it should be simply called addAdvancedAttributes instead of getSaveContent.extraProps to avoid dot notation in the name.

Actually, I think getSaveContent.extraProps convey the correct meaning. I don't care much about the dot notation

gziolo · 2017-11-13T14:07:11Z

blocks/hooks/anchor.js

+}
+
+export default function anchor( { addFilter } ) {
+	addFilter( 'registerBlockType', 'core\anchor-attribute', addAttribute );


Should we distinguish filter names somehow between block lifecycle methods like registerBlockType or getSaveContent and editor components like BlockEdit? Maybe it's enough that a component starts with upper case.

To this and the previous point, I agree we need some conventions (also noted in #3318 (comment)). The dot notation was an idea for "filter within a function, but not filtering the function's return value", and the upper-case for components.

gziolo · 2017-11-13T14:10:03Z

blocks/hooks/index.js

+const hooks = createHooks();
+
+const {
+	addAction,


At this moment we use only:

addFilter

removeAllFilters (accessed directly from @wordpress/hooks)

applyFilters

I would have liked to simplify this to some pattern for exporting all available keys from createHooks(), but it's not supported by module exports.

https://stackoverflow.com/questions/29844074/es6-export-all-values-from-object

Would you suggest limiting this only to those that are currently used?

Yes, I would take a similar approach to what we do for wp.element, where we cherry-pick only what is really essential to make things work. I'm in favor of starting with a very limited lits of exposed methods and extend it whenever we have a real-life use case.

It's still not public API, so we can iterate later. Anyway, we should definitely revisit when working on documentation.

gziolo · 2017-11-13T14:18:45Z

I prefer this solution over #1023. API here is much easier to use, although it might be less flexible when compared with the decorator approach. I think we should start with the simpler solution and find out how far we can get with it. I also figured out that addInspectorControl is an example of how we can decorate existing components. So if we decide to take this approach, it will make #1023 obsolete.

If we decide to merge this PR together with #3321 (slot & fill ), the only piece left would be Redux state.

I will test it in-depth tomorrow, but I can already say that it looks solid enough that I anticipate that it's going to be accepted.

youknowriad · 2017-11-14T09:01:52Z

blocks/api/registration.js

+export function hasBlockSupport( nameOrType, feature, defaultSupports ) {
+	const blockType = 'string' === typeof nameOrType ?
+		getBlockType( nameOrType ) :
+		nameOrType;


Minor: but I tend to prefer functions with an unchanged signature. My preference would be to always use type and call getBlockType on the caller side when needed.

It also makes documenting easier when you enforce calling getBlockType explicitly. We can always provide two methods to support those 2 different ways of calling:

export function hasBlockTypeSupport( type, feature, defaultSupports ) {} export function hasBlockSupport( name, feature, defaultSupports ) { return hasBlockTypeSupport( getBlockType( name ), feature, defaultSupports ); }

The only challenge in such case is to find proper names for methods :)

youknowriad · 2017-11-14T09:08:23Z

blocks/hooks/anchor.js

+ * @param  {Object}  props   Props passed to BlockEdit
+ * @return {Element}         Filtered edit element
+ */
+export function addInspectorControl( element, props ) {


Should we expose generic versions of those? Maybe, better to wait before that move.

youknowriad

Nice work, let's get this in

gziolo · 2017-11-14T10:14:01Z

@youknowriad @aduth Last thing to confirm before I merge this PR. Are we fine with removing Copy button from anchor?

Before

After

I also noticed that it's no longer under Advanced section.

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

I will open a follow-up with a proposal how to put HTML Anchor under Advanced panel. Let's proceed with this one.

Great job @aduth! This approach should make maintaining extensions points for blocks much easier internally in Gutenberg 👏

aduth · 2017-11-14T13:21:43Z

@gziolo Yeah, losing the Copy Link button was a noted compromise of this approach. We will need to consider this as part of a general pattern for extensions accessing post state.

gziolo · 2017-11-14T13:34:27Z

I opened #3475 to move back HTML Anchor under Advanced section as noted in the comment above (#3318 (comment)).

aduth added [Feature] Block API API that allows to express the block paradigm. [Feature] Extensibility The ability to extend blocks or the editing experience labels Nov 2, 2017

youknowriad reviewed Nov 3, 2017

View reviewed changes

youknowriad mentioned this pull request Nov 3, 2017

Blocks: Adding versioning and migrations to the Block API #3327

Closed

2 tasks

aduth force-pushed the try/blocks-extensibility branch from 0a38901 to 6c22b41 Compare November 8, 2017 15:13

gziolo reviewed Nov 13, 2017

View reviewed changes

aduth force-pushed the try/blocks-extensibility branch from 6c22b41 to 357aa7d Compare November 14, 2017 02:07

youknowriad reviewed Nov 14, 2017

View reviewed changes

youknowriad approved these changes Nov 14, 2017

View reviewed changes

aduth added 3 commits November 14, 2017 10:58

Extensibility: Define anchor behavior as filtered blocks support

66a8d57

Blocks: Extract common behaviors for block edit rendering

e5bdcd1

Block API: Define granular hooks for block filtering

7f3a223

gziolo force-pushed the try/blocks-extensibility branch from 357aa7d to 7f3a223 Compare November 14, 2017 10:51

gziolo approved these changes Nov 14, 2017

View reviewed changes

gziolo merged commit cbd8cd0 into master Nov 14, 2017

gziolo deleted the try/blocks-extensibility branch November 14, 2017 10:58

youknowriad mentioned this pull request Nov 14, 2017

Extensibility: Define customClassname behavior as filtered blocks support #3472

Merged

gziolo mentioned this pull request Nov 14, 2017

Blocks: Put anchor inside block inspector's Advanced section #3475

Closed

4 tasks

aduth mentioned this pull request Nov 16, 2017

Try: Components decorator extensibility pattern #1023

Closed

mtias mentioned this pull request Nov 20, 2017

Advanced settings in blocks #3002

Closed

andreiglingeanu mentioned this pull request Dec 4, 2017

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

Closed

snyk-bot mentioned this pull request Sep 21, 2022

[Snyk] Security upgrade lerna from 3.22.1 to 5.5.2 MaxMood96/gutenberg#75

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Try: Extensibility: Define anchor behavior as filtered blocks support #3318

Try: Extensibility: Define anchor behavior as filtered blocks support #3318

aduth commented Nov 2, 2017 •

edited

Loading

codecov bot commented Nov 2, 2017 •

edited

Loading

youknowriad left a comment •

edited

Loading

youknowriad Nov 3, 2017

aduth Nov 3, 2017

youknowriad Nov 3, 2017

aduth Nov 3, 2017

youknowriad Nov 3, 2017

youknowriad Nov 3, 2017

aduth Nov 3, 2017

youknowriad commented Nov 3, 2017

aduth commented Nov 3, 2017

aduth commented Nov 8, 2017

gziolo Nov 13, 2017

youknowriad Nov 14, 2017

gziolo Nov 13, 2017

aduth Nov 14, 2017

gziolo Nov 13, 2017

aduth Nov 14, 2017

gziolo Nov 14, 2017

gziolo Nov 14, 2017

gziolo commented Nov 13, 2017

youknowriad Nov 14, 2017

gziolo Nov 14, 2017 •

edited

Loading

youknowriad Nov 14, 2017

youknowriad left a comment

gziolo commented Nov 14, 2017

gziolo commented Nov 14, 2017

aduth commented Nov 14, 2017

gziolo commented Nov 14, 2017


		Anchors let you link directly to a specific block on a page. This property adds a field to define an id for the block and a button to copy the direct link.
		- `supportAnchor` (default `false`): Anchors let you link directly to a specific block on a page. This property adds a field to define an id for the block and a button to copy the direct link.

Try: Extensibility: Define anchor behavior as filtered blocks support #3318

Try: Extensibility: Define anchor behavior as filtered blocks support #3318

Conversation

aduth commented Nov 2, 2017 • edited Loading

codecov bot commented Nov 2, 2017 • edited Loading

Codecov Report

youknowriad left a comment • 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

youknowriad commented Nov 3, 2017

aduth commented Nov 3, 2017

aduth commented Nov 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

gziolo commented Nov 13, 2017

Choose a reason for hiding this comment

gziolo Nov 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad left a comment

Choose a reason for hiding this comment

gziolo commented Nov 14, 2017

Before

After

gziolo commented Nov 14, 2017

aduth commented Nov 14, 2017

gziolo commented Nov 14, 2017

aduth commented Nov 2, 2017 •

edited

Loading

codecov bot commented Nov 2, 2017 •

edited

Loading

youknowriad left a comment •

edited

Loading

gziolo Nov 14, 2017 •

edited

Loading