Enhance attributes to specify types and defaults

[aduth]

Closes #865
Closes #973
Supersedes #892, #988
Related: #391, #714

This pull request seeks to refactor the block type attributes property to specify not only attributes parsed from content, but also those also encoded in the block comment delimiter. Further, it optionally allows specifying type and default value of an attribute, replacing previous efforts including defaultAttributes and render-time destructuring.

The existing behavior of attributes matchers will continue to work as-is, but comment attributes are no longer inferred and must instead be explicitly defined. The value of each key in attributes can be a matcher, a constructor (type), or an object specifying any of type, matcher, and defaultValue.

See the following comment for the "kitchen sink" example: #1905 (comment)

See original kitchen sink example

it( 'should merge attributes with the parsed and default attributes', () => {
	const blockType = {
		attributes: {
			content: text( 'div' ),
			number: {
				type: Number,
				matcher: attr( 'div', 'data-number' ),
			},
			align: String,
			topic: {
				type: String,
				defaultValue: 'none',
			},
			ignoredDomMatcher: {
				matcher: ( node ) => node.innerHTML,
			},
		},
	};
const rawContent = '<div data-number="10">Ribs</div>';
const attrs = { align: 'left', invalid: true };

expect( getBlockAttributes( blockType, rawContent, attrs ) ).toEqual( {
	content: 'Ribs',
	number: 10,
	align: 'left',
	topic: 'none',
} );

} );

Block authors can potentially implement their own type coercion by specifying a constructor with valueOf implementation. There is some inspiration here from Vue.js prop validation and prop-types. (Removed)

Testing instructions:

Ensure that tests pass:

npm test

Verify that there is no regressions in block usage, particularly:

• Block default values
• Block transforms
• Block serialization of comment attributes

[westonruter]

Should the format here not try to conform to JSON Schema? This refactored attributes is very close to something that could be returned by the REST API, for any values that are JSON-serializable. There is a spec for what type can be, including non-scalar values. Ideally default would be used instead of defaultValue, even though that would mean the key would always have to be quoted.

Example from REST API schema: https://github.com/WordPress/wordpress-develop/blob/4.8.0/src/wp-includes/rest-api/endpoints/class-wp-rest-posts-controller.php#L2123-L2124

[aduth]

@westonruter Hmm, maybe. JSON Schema certainly loses some of the expressiveness that the "compact" form allows here. Additionally, how would you propose a JSON Schema approach handle sourcing data, e.g. here matcher? We may eventually need ability to source from external data like site option, user option, etc.

[westonruter]

@aduth I don't think the presence of a matcher would be a problem for JSON Schema. In fact, in the REST API controllers in WordPress there are likewise callback functions defined as part of the arrays that are used to construct the schemas that are served by REST API. For example, the context param is not defined in the JSON Schema spec but it nevertheless is returned as part of the schema response. More relevant to matcher, there is also an arg_options property which contains callbacks for sanitize_callback and validate_callback: https://github.com/WordPress/wordpress-develop/blob/4.8.0/src/wp-includes/rest-api/endpoints/class-wp-rest-posts-controller.php#L1786-L1789

You can see then that when fetching the data for the schema, the arg_options is removed from the response: https://github.com/WordPress/wordpress-develop/blob/4.8.0/src/wp-includes/rest-api/endpoints/class-wp-rest-controller.php#L286-L303

So I think we should do the same for blocks: the data structure used when registering blocks should be a superset of JSON Schema, but we should re-use as many of the formats that are defined in the specification. This will hopefully make it easier to pass block attributes between the client and the server, and we could use any libraries developed for JSON Schema to validate and utilize block schemas.

[aduth]

It's certainly tempting, as there's already quite a bit of overlap. The main downside is verbosity of JSON Schema, but the difference between foo: Number and foo: { type: 'number' } is fairly small.

I also don't know how much we want to commit to client/server interoperability of attributes. One possible need we'd have is custom transformations of parsed data during attributes parsing, supported here with custom type constructors, but not so obvious with plain JSON Schema values.

Related to #672, JSON Schema could enable some automatic form generation, e.g. https://github.com/mozilla-services/react-jsonschema-form

[westonruter]

the difference between foo: Number and foo: { type: 'number' } is fairly small.

Is it not even less, since type would be present in both? Is it not the difference between:

- foo: { type: Number, defaultValue: 'bar' }
+ foo: { type: 'number', 'default': 'bar' }

But then with JSON Schema we'd be able to also describe attributes with all of the properties that it provides, including minimum, maximum, required, format, enum, and so on.

One possible need we'd have is custom transformations of parsed data during attributes parsing, supported here with custom type constructors, but not so obvious with plain JSON Schema values.

Would you elaborate?

[aduth]

Is it not even less, since type would be present in both? Is it not the difference between:

Not in the compact form, which allows attribute values to be either the matcher or constructor directly. See included embed block as example:

attributes: {
	title: attr( 'iframe', 'title' ),
	caption: children( 'figcaption' ),
	url: String,
	align: String,
},

Would you elaborate?

The thought being parse-time derived attributes.

Struggling to think of some good real-world examples, but maybe: rounding a parsed float value to a certain number of digits, calculating (gallery) columns based on number of matched elements. Not necessarily the case that these can't be performed in the render functions, but there may be value in being able to generate these attributes external to a render, and it's otherwise hard to guarantee consistency particularly between separate edit and save implementations (already a handful of cases where we've fallen short in e.g. defaulting, normalizing between string and number, upper-and-lower case node names).

[nb]

Thanks, @aduth, this seems much better than before.

I like the simplicity of types for now – we can easily switch to proptypes or something similar in the future, if there’s interest.

Do you think the implicit connection between attributes kept in the HTML comment and them marked as a matcher is clear enough?

Are we running the default checks on each re-render or only when we create the block?

Do you think that bundling types with defaults is better than a function like getDefaultAttributes? This way developers will have a lot more flexibility to build the default values. Now if I want to have some extra logic I will need to include the types in my separately generated object.

[youknowriad]

Have you considered calling this function (getNormalizedAttributeSource) when registering the block to avoid having to do it everytime we need the block attributes?

[youknowriad]

Can't we avoid the getMatcherAttributes call and handle this while looping over sources?

[youknowriad]

I like the approach proposed here, explicitly defining all the attributes in the same place is a huge win.

Question about the types, have we surfaced use-cases for these? Is it necessary to have the attribute defined as a "Constructor"?

[aduth]

@nb @youknowriad Do you have any thoughts on @westonruter's proposal for aligning this even further to JSON Schema?

[youknowriad]

Do you have any thoughts on @westonruter's proposal for aligning this even further to JSON Schema?

Good question: Are we trying to match the JSON Schema just to match a specification or do we expect to use the JSON Schema validation/form generation mechanism?

Maybe we could stick with our "simpler" config until we find a use-case for the JSON-Schema related features. We still have some time to break the blocks API.

[nb]

@aduth this hits a more philosophical and higher-level question – what’s the role of the server-side?

The way I see the project and its strengths, the user experience when editing a block is the feature that will have the biggest impact and be the biggest differentiator. This means richer and richer experiences on the JavaScript side and will eventually bring the need for more sophisticated type checking.

Back to the question at hand – JSON Schema looks fine and has a good amount of fidelity, lends itself to introspection. I also think it’s a bit verbose compared to, let’s say, React’s prop-types, but I agree with @westonruter that this would allow having the some attribute validation to be sent to the server.

Of all the things we could have both in the browser and on the server, validation sounds the most useful. Looking at the only server-side example we have, latest-posts, knowing the types of the arguments can definitely come in handy.

Without more server-side examples, it’s hard to make an informed decision. Here are few open questions for me: how often would we need some JS logic to compute a default, or validation? Will there be sometimes a difference between attributes accepted on the server and in the browser?

[westonruter]

Without more server-side examples, it’s hard to make an informed decision.

I've tried to gather up as many of the issues for server-side blocks (dynamic blocks) as possible in https://github.com/WordPress/gutenberg/projects/6

Here are few open questions for me: how often would we need some JS logic to compute a default, or validation?

In the case of latest-posts or any dynamic block, there will be a need to obtain the default properties for the block to render. At the moment, the block's default attributes are duplicated since there is no common schema that can be used on both the client and the server. Duplicated default:

gutenberg/lib/blocks/latest-posts.php

Line 16 in 1c7aaf8

$posts_to_show = 5;

Likewise, in the case of validation, there is procedural ad hoc validation of the attributes to ensure that the values have the expected types:

gutenberg/lib/blocks/latest-posts.php

Lines 27 to 34 in 1c7aaf8

	// Basic attribute validation.
	if (
	is_numeric( $posts_to_show_attr ) &&
	$posts_to_show_attr > 0 &&
	$posts_to_show_attr < 100
	) {
	$posts_to_show = intval( $posts_to_show_attr );
	}

All of this logic could be replaced with calls to rest_validate_value_from_schema() and rest_sanitize_value_from_schema() in PHP, and for any attribute that does not pass validation and sanitization, to instead use the default value.

Will there be sometimes a difference between attributes accepted on the server and in the browser?

Maybe the server would allow a number string when the schema asks for an integer. The REST API schema sanitization logic will do that type conversion.

[aduth]

Do you think the implicit connection between attributes kept in the HTML comment and them marked as a matcher is clear enough?

At worst, this feature makes comment attributes much more obvious. At best, it elevates them to the role of the default attribute type, whereas something with a matcher (or external source) is delegated, telling the editor "instead of HTML comments, look for the value here instead".

Are we running the default checks on each re-render or only when we create the block?

We should only need to when creating the block. I'm vaguely remembering an issue you'd raised about this previously, but cannot recall specifics.

Do you think that bundling types with defaults is better than a function like getDefaultAttributes? This way developers will have a lot more flexibility to build the default values. Now if I want to have some extra logic I will need to include the types in my separately generated object.

You might also be interested in #892, which takes an approach similar to this and highlights some of the contrasting requirements.

A main difference is in consolidation and formality of comment attributes. In the #892 implementation, encodeAttributes is still a separate step, and the connection between it and getInitialAttributes is not as strong. Further, merging with comment attributes in getInitialAttributes is awkward; injecting the comment-parsed attributes is still magical, and very unobvious (why does getInitialAttributes receive an attributes object as an argument?)

I do find that the function approach succeeds in:

• Defaulting plays nicely with Object.assign
• Custom formatting / fallback logic

Have you considered calling this function (getNormalizedAttributeSource) when registering the block to avoid having to do it everytime we need the block attributes?

That sounds like a good idea.

Can't we avoid the getMatcherAttributes call and handle this while looping over sources?

Yeah, I see the inefficiency, and I think it can be improved. One potential hiccup is that getMatcherAttributes was intentionally abstracted to handle an an object of sources for re-use in the paste handling. In this case, if we were to move the logic into the sources mapping, we'd need to refactor to be able to retrieve a single matcher value.

[nb]

Given the JSON Schema idea the argument where to see the default isn’t relevant.

Are we running the default checks on each re-render or only when we create the block?
We should only need to when creating the block. I'm vaguely remembering an issue you'd raised about this previously, but cannot recall specifics.

There are plenty of opportunities to setAttribute an attribute to undefined or to a value with the wrong type.

[aduth]

There are plenty of opportunities to setAttribute an attribute to undefined or to a value with the wrong type.

In that case, maybe check undefined to restore default and validate type at either...

• setAttribute
• Before save
• Before both save and edit

[aduth]

This was on my mind a bit over the weekend. Thinking about JSON Schema, I was imagining a couple options:

1. A new file attributes.json sites adjacent the block JavaScript / PHP code and is imported into each respectively.

{
	"postsToShow": {
		"type": "number",
		"min": 0,
		"max": 20
	},
	"align": {
		"type": "string",
		"enum": [ "left", "center", "right", "wide", "full" ],
		"default": "center",
	}
}

import attributes from './attributes.json';

In PHP, this could be

json_decode( file_get_contents( dirname( __FILE__ ) . '/attributes.json' ) );

(Or automatically detected in PHP)

2. Attributes are defined in PHP block registration, and bootstrapped into client-side.

register_block( 'core/latest-posts', [
	'attributes' => [
		'postsToShow' => [
			'type' => 'number',
			'min'  => 0,
			'max'  => 20
		],
		'align' => [
			'type'    => 'string',
			'enum'    => [ 'left', 'center', 'right', 'wide', 'full' ],
			'default' => 'center',
		],
	],
] );

---

In both cases, attribute definitions are consolidated to a single location, the main difference being in the latter example that all blocks would need to register themselves server-side, even if they don't have any server-specific logic.

Another potential issue is recreating all of the attribute matching in JSON or PHP. At first I thought something like:

{
	"url": {
		"type": "string",
		"source": {
			"type": "attribute",
			"selector": "img",
			"name": "src"
		}
	}
}

But this only works well for the simplest matchers like attr, and doesn't extend well to query (pullquote example). I thought instead maybe we could recreate the function signature:

{
	"value": {
		"type": "node",
		"value": [ "query", "blockquote > p", [ "node" ] ]
	}
}

But then it became clear: How do we represent values like node and children in PHP? Do we care to? Recreating selector behavior will also be a challenge.

On the side of "Do we care to?", it's not clear that there are use-cases in the server where we'd need the node value, particularly because in most of these cases, there isn't markup for the block anyways (at least not that we care to source from). This could lead to a couple possible conclusions:

• Server-rendered sourcing does not include all the same source types as in the client (no children, no node)
• Server attributes are similar to (JSON Schema) but not identical to client-side attributes
  • Client-side continues to use source/matcher functions in JSON schema
  • Lose consolidation, instead potentially duplicating code, unless we could come up with a solution for client-side to "decorate" the server-defined attributes with source/matcher functions if necessary

Anyways, this is all a brain dump. I don't have any solutions in mind. But I'd be reluctant to move on #1902 if we're still not settled to the actual structure of attributes.

There are a few questions to the requirements though:

• Do both server and client need to know attribute structure, or only one or the other?
  • I'm inclined to say that the client must have attribute structure, for example in the Latest Posts block, being able to represent the default value for a newly created block.
• If we introduce attributes for blocks to the server, should it apply to all blocks? What are the use-cases?
  • We've highlighted attribute validation, but is that the only requirement? Is there value in exposing these attributes over the REST API, noting that even if server understands the shape, it is unable to modify or create blocks?
• Are there cases where we need to extract attributes from markup on the server?
  • Or are we assuming these would be empty-content (or fallback content) blocks, with relevant attributes solely in comment attributes

[youknowriad]

I don't know if we'd ever need to replicate the attributes parsing mechanism in PHP but regardless it's appealing to just make it a possibility (thinking block parsing API...). In their current form, the attributes can't be used server side.

Anyway, we should try to move things forward and maybe split things into smaller steps.

• This PR already solves a consistency problem in the client for the attributes declaration, I'd say we could merge it as a first step. (I still have a question about the use-fullness of types above)

• Consider a JSON schema as a second step

• Parse the JSON schema server-side (if needed) as a third

[youknowriad]

@aduth This looks good to me. Is the type mandatory here? What impact the type will have in parsing since the JSON attribute serialization already keeps the "type" information.

[aduth]

Is the type mandatory here?

To be valid JSON schema, I'd say it'd be good to enforce as mandatory.

What impact the type will have in parsing since the JSON attribute serialization already keeps the "type" information.

For comment attributes, it could serve as validation (on parse) and coercion (on save, or assignment). Imagine a { type: 'number' } that we populate from an input field. Naively one could implement as:

<input onInput={ ( event ) => setAttributes( { myNumber: event.target.value } ) } />

...which of course would assign number as a string.

[nb]

Looks good to me, too 👍

[aduth]

I'm still giving the changes a thorough look over because it touches almost everything, but I've successfully rebased and effected latest proposal into this branch.

This does not include any sort of type validation or coercion, but these don't really exist currently anyways, so I think it'd be fine to iterate in subsequent pull requests.

[codecov]

Codecov Report

Merging #1905 into master will increase coverage by 0.45%.
The diff coverage is 91.52%.

@@            Coverage Diff             @@
##           master    #1905      +/-   ##
==========================================
+ Coverage   24.96%   25.41%   +0.45%     
==========================================
  Files         151      151              
  Lines        4679     4705      +26     
  Branches      795      792       -3     
==========================================
+ Hits         1168     1196      +28     
- Misses       2965     2967       +2     
+ Partials      546      542       -4

Impacted Files	Coverage Δ
editor/effects.js	30.3% <ø> (ø)	⬆️
blocks/library/preformatted/index.js	44.44% <ø> (ø)	⬆️
blocks/library/freeform/index.js	100% <ø> (ø)	⬆️
blocks/library/table/index.js	36.36% <ø> (ø)	⬆️
blocks/library/html/index.js	23.07% <ø> (ø)	⬆️
blocks/library/paragraph/index.js	47.05% <ø> (ø)	⬆️
blocks/library/more/index.js	30% <ø> (ø)	⬆️
blocks/library/latest-posts/index.js	10% <ø> (ø)	⬆️
blocks/library/text-columns/index.js	33.33% <ø> (ø)	⬆️
blocks/api/validation.js	94.33% <ø> (ø)	⬆️
... and 17 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 d7033d5...7f9dd2b. Read the comment docs.

[mtias]

It's not clear here if this is optional.

[mtias]

"is not assigned"?

[mtias]

@aduth great work here. I am also slightly conflicted with the verbosity of type being mandatory (it's also not clear in the docs where we stand, or what we recommend). There's something about sourcing that makes me think type is to be inferred from the sourcing technique used.

In any case, this is a solid improvement in clarity.

A new file attributes.json sites adjacent the block JavaScript / PHP code and is imported into each respectively.

I had been thinking about having a block.json that would define things like name, category, attributes, and be shared between client and server.

In both cases, attribute definitions are consolidated to a single location, the main difference being in the latter example that all blocks would need to register themselves server-side, even if they don't have any server-specific logic.

Having access to a registered block, even if it doesn't have any purpose server-side, could still be beneficial for things like setting default blocks through configuration hooks, excluding blocks from certain post types, as well as defining templates in PHP that would declare certain client-side blocks to be used.

[aduth]

There's something about sourcing that makes me think type is to be inferred from the sourcing technique used.

One use case is the added ability to source a number from an attribute, e.g. { type: 'number', source: attr( 'img', 'data-number' ) }, previously always returned as string.

[aduth]

I've done another rebase pass, including review and incorporating newly added attributes, default attributes. Also as refactoring pass to extract type coercion for testing in 7f9dd2b, I added logging to warn about unexpected types for comment-serialized attributes (in development environments).

Example:

I am planning to merge this tomorrow AM (EDT), followed shortly thereafter by #1902, unless there are remaining concerns or objections.