Validate block settings for edit() and save()

[BE-Webdesign]

Fixes #362. Validates that a block has a save() and edit(). This is
probably a temporary solution. The API should probably be cleaned up a
bit so everything is not quite as coupled and tests can be done
differently.

[youknowriad]

Since we're adding some validation, should we validate all the mandatory attributes we have? I'm thinking of icon, title and category. Maybe we should also validate that the category is a known category.

[BE-Webdesign]

I think this is fine for now, this sets our interface as requiring save() and edit().

If there is buy-in to do more thorough validation, then I would probably redo this entire approach to become a bit more programmatic. The more and more validation we add will make things more and more sprawly, so we will probably eventually want to introduce a schema. A schema based validation could double serve to help validate incoming attributes for an actual block as well.

[aduth]

There was a similar discussion in the original validation introduced about how to name this function based on its behavior. Eventually it appears we inlined the validation. A few questions then:

• Should we inline block settings validation for consistency with the slug validation?
• Should we expect the validate to do something, like throw, or perhaps prefixing with is to reflect its boolean return value is more accurate, like isValidBlockSettings
• If we know the function returns a boolean, do we need to be so explicit with the comparison, or can we simplify to ! isValidBlockSettings ?

[BE-Webdesign]

Should we inline block settings validation for consistency with the slug validation?

Most likely I am going to make another PR and totally redo the approach, to have the validation be all in one function rather than the sort of procedural thing we have developing.

Should we expect the validate to do something, like throw, or perhaps prefixing with is to reflect its boolean return value is more accurate, like isValidBlockSettings

Prefixing sounds good. We can throw as well.

If we know the function returns a boolean, do we need to be so explicit with the comparison, or can we simplify to ! isValidBlockSettings ?

I am usually a fan of the strict checks, but can definitely change it.

[aduth]

Thinking since we already assign this as block below that we could move the condition after the variable assignment and simply return the variable instead of duplicating the logic (especially in case we change the merge logic down the road).

const block = Object.assign( { slug }, settings );
if ( ! isValidBlockSettings( settings ) ) {
	console.error(
		'Block not registered.'
	);
	// Return the block even though it was not registered.
	return block;
}

Or perhaps we reflect the intent that the global blocks object should only assign the new block if it's valid, but allow the function to continue to the final return:

const block = Object.assign( { slug }, settings );
if ( isValidBlockSettings( settings ) ) {
	blocks[ slug ] = block;
} else {
	console.error(
		'Block not registered.'
	);
}

return block;

[BE-Webdesign]

Yup, I think the second one looks better to me. Something I like to do in my apps is have a processing chain of sorts that has consistent input output behavior. It works similar to Promises.

This could just become a pipe ( flow in lowdash ) of blockSettings -> isValid -> addBlock -> return settings. That way, if done properly, you can interchange the processing for whatever needs may come and in the future stick something new in there pretty painlessly.

[aduth]

Being that this file is placed in the test directory, Mocha will try to read and interpret it as a test suite, which it's not. Perhaps we could either inline these functions where they're used, or create a nested fixtures directory?

[BE-Webdesign]

Sure. I didn't see the harm in Mocha running it. It pretty much just skips over it.

fixtures directory sounds good. I usually name it test-data. is that good?

[aduth]

I think we could choose one or the other of the named variable or function:

export function edit() {

export const edit = function() {
// OR: 
//  export const edit = () => 'tacos';

The one-liner arrow function could work well as an inline function in each test, even if duplicated. Else I tend to prefer named functions with the function keyword.

[BE-Webdesign]

Yup, sounds good.

[aduth]

Minor: We could take advantage of ES2015 object property shorthand to simplify this:

registerBlock( 'core/test-block', { edit, save } );

[BE-Webdesign]

Yeah, I wasn't sure what style should be done for that. In some projects people shy away from that as it can be confusing as to how that works.

[BE-Webdesign]

Rather than revising this right now, I think I may try a separate PR that demonstrates another way to handle this, if that approach seems more appealing, then we can go that way. If we poo poo it, then we can come back to this one and I will revise.

[BE-Webdesign]

Closing, will circle back to this at another point in the future.

[swissspidy]

Can this branch be deleted after #1345?