Validate usable render behavior on registered block

[aduth]

When registering a block, we should ensure that it has at minimum a save implementation (front-end UI representation) and optionally edit.

Once validated, we can collapse logic here to return early only on settings being falsey and assume if settings exist that a edit or save function/component can be used for displaying the block:

https://github.com/WordPress/gutenberg/blob/15db036/editor/editor/mode/visual.js#L25-L32

[BE-Webdesign]

Having blocks with no settings seems to be a viable case in the tests are we changing that assumption?

[aduth]

After having spoken with @mtias , it seems there's a desire that:

• Blocks must implement both edit and save*
• We ought to update the visual editor's block rendering to render only with edit

It's quite possible other tests may need to be updated to reflect this behavior.

* Per #401, we may want to change these method names to be more descriptive. Do you have any opinions on this?

[BE-Webdesign]

Blocks must implement both edit and save*

In the branch I have going, both are being forced.

It's quite possible other tests may need to be updated to reflect this behavior.

Yeah... lots of impact on other modules currently. If everyone is open to some changes, I would like to change the API slightly to be a bit more flexible, moving into the future.

Per #401, we may want to change these method names to be more descriptive. Do you have any opinions on this?

forEditing(), and forSaving() sound better to me and I think improve readability.

Maybe .editView() and .view()? The output of save() is really the view component for the block.

[aduth]

If everyone is open to some changes, I would like to change the API slightly to be a bit more flexible, moving into the future.

Improvements are always welcome 😄

[njpanderson]

Is this definitely a good first task? Would like to see where I can help on this one.

First thoughts would be to add basic type checking of the two properties in registerBlockType but I'm assuming this isn't that simple, otherwise it surely would have been done by now?

[aduth]

First thoughts would be to add basic type checking of the two properties in registerBlockType but I'm assuming this isn't that simple, otherwise it surely would have been done by now?

That sounds like a good path to head down. In this case it's not already been done because it's not actively harming anything aside from being more flexible than we care for it to be.

There's some precedent of validation:

gutenberg/blocks/api/registration.js

Lines 35 to 52 in 740b93d

	if ( typeof name !== 'string' ) {
	console.error(
	'Block names must be strings.'
	);
	return;
	}
	if ( ! /^[a-z0-9-]+\/[a-z0-9-]+$/.test( name ) ) {
	console.error(
	'Block names must contain a namespace prefix. Example: my-plugin/my-custom-block'
	);
	return;
	}
	if ( blocks[ name ] ) {
	console.error(
	'Block "' + name + '" is already registered.'
	);
	return;
	}

See also: #508

@BE-Webdesign , given you'd assigned this to yourself but haven't had a chance to circle back around, would you mind if @njpanderson were to take it on?

[BE-Webdesign]

BE-Webdesign , given you'd assigned this to yourself but haven't had a chance to circle back around, would you mind if njpanderson were to take it on?

Nope. I can unassign myself as well. I was planning on doing this at some point, but if someone wants to do it right now all the better.

[njpanderson]

Right, thanks guys. Will give it a go :)

[njpanderson]

Ok, I've got two branches on the go currently, so I'll explain what's what:

update/stricter-block-validation

This is a basic update to registerBlockType which will at minimum require the save function and require that the edit function is indeed a function, if passed. Lots of updates to existing tests but broadly it's a simple change with inline logic added to the registration function itself.

try/prop-based-block-validation

This is a little more involved and at a very rough and ready stage right now. It's an adapted version of a "PropType" style validator I wrote for another library. I've not taken it too far just in case you all decide it's not worth pursuing. The idea is slightly different in this one:

You define an ideal set of variables and their required attributes, as well as the actual variable values. This specification is sent to a validate function which will either return true or an error message string detailing the first invalid variable.

It's pretty generic and in theory could be used anywhere in the project that there is a list of required variables and their types.

Let me know what you think and whether or not I should continue working on that second branch.

[BE-Webdesign]

@njpanderson I checked the branches out. Great work! For now I think stricter-block-validation would be good. prop-based-block-validation is also great, I am not sure whether we are going to introduce block schemas quite yet. We will probably eventually need schemas and want to have some synergy between the server-side and client-side schemas, which will probably lead to the adoption of JSON Schema validation.

[njpanderson]

Fair enough. I'll keep my try/ branch on my own repo just for now and have PR'ed the update/ branch. Thanks!

[westonruter]

Maybe this has been discussed before, and it's also being discussed on the PHP side in #1321 (PR #1322): should there not be a BlockType class in ES6 that can has the edit and save methods defined on it? And this class can then be extended? Then registerBlockType could either take as params ( id: string, settings: object ) or it could take ( block: BlockType ), where in the former case it will then instantiate the base BlockType class with the supplied settings object.

[njpanderson]

That would be quite a major refactoring of existing code from my perspective but definitely something I would be willing to take on.

One potential downside to extending an existing class might be that the edit/save methods could do anything by default and we might not be able to validate a plugin author forgetting to add them as easily, because they'll always exist. Unless we actually run the methods to see what they do, which seems like opening a huge can of worms.

Should a new issue be started for it either way?

[aduth]

@westonruter Why would the class be better? ES5 support has been the deterring factor for me. Supporting both forms is similarly a negative in my view, as more options muddies communication.

Related previous conversation with @nb: #1135 (comment)

[BE-Webdesign]

Please no ES6 classes.

[youknowriad]

It took us several years to figure out that inheritance was a bad practice, let's stick with composition and functions, please.

[youknowriad]

closing as fixed by #1345