Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Validate block settings for edit() and save() #508

Closed
wants to merge 1 commit into from
Closed

Validate block settings for edit() and save() #508

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 38 additions & 0 deletions blocks/api/registration.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,13 @@ export function registerBlock( slug, settings ) {
);
return;
}
if ( true !== validateBlockSettings( settings ) ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

console.error(
'Block not registered.'
);
// Return the block even though it was not registered.
return Object.assign( { slug }, settings );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Copy link
Contributor Author

@BE-Webdesign BE-Webdesign Apr 26, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
const block = Object.assign( { slug }, settings );
blocks[ slug ] = block;
return block;
Expand Down Expand Up @@ -104,3 +111,34 @@ export function getBlockSettings( slug ) {
export function getBlocks() {
return Object.values( blocks );
}

/**
* Validates the block settings.
*
* @param {Object} settings Block settings.
* @return {Boolean} Whether the block settings are valid.
*/
export function validateBlockSettings( settings ) {
if ( ! settings ) {
console.error(
'Block settings must specify a save() and edit() method for each block.'
);
return false;
}

if ( ! settings.save || 'function' !== typeof settings.save ) {
console.error(
'Block settings must specify a save() method for each block.'
);
return false;
}

if ( ! settings.edit || 'function' !== typeof settings.edit ) {
console.error(
'Block settings must specify a edit() method for each block.'
);
return false;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@BE-Webdesign BE-Webdesign Apr 26, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


return true;
}
29 changes: 23 additions & 6 deletions blocks/api/test/factory.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import { expect } from 'chai';
*/
import { createBlock, switchToBlockType } from '../factory';
import { getBlocks, unregisterBlock, setUnknownTypeHandler, registerBlock } from '../registration';
import { edit, save } from './function-refs';

describe( 'block factory', () => {
afterEach( () => {
Expand Down Expand Up @@ -43,9 +44,14 @@ describe( 'block factory', () => {
};
}
} ]
}
},
edit: edit,
save: save
} );
registerBlock( 'core/text-block', {
edit: edit,
save: save
} );
registerBlock( 'core/text-block', {} );

const block = {
uid: 1,
Expand All @@ -67,7 +73,10 @@ describe( 'block factory', () => {
} );

it( 'should switch the blockType of a block using the "transform to"', () => {
registerBlock( 'core/updated-text-block', {} );
registerBlock( 'core/updated-text-block', {
edit: edit,
save: save
} );
registerBlock( 'core/text-block', {
transforms: {
to: [ {
Expand All @@ -78,7 +87,9 @@ describe( 'block factory', () => {
};
}
} ]
}
},
edit: edit,
save: save
} );

const block = {
Expand All @@ -101,8 +112,14 @@ describe( 'block factory', () => {
} );

it( 'should return null if no transformation is found', () => {
registerBlock( 'core/updated-text-block', {} );
registerBlock( 'core/text-block', {} );
registerBlock( 'core/updated-text-block', {
edit: edit,
save: save
} );
registerBlock( 'core/text-block', {
edit: edit,
save: save
} );

const block = {
uid: 1,
Expand Down
13 changes: 13 additions & 0 deletions blocks/api/test/function-refs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
/**
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@BE-Webdesign BE-Webdesign Apr 26, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

* Function references used so that Chai doesn't complain about functions
* not being equal.
*
* These are methods that should be assigned to a valid block.
*/
export const edit = function edit() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, sounds good.

'tacos';
};

export const save = function save() {
'delicious';
};
51 changes: 41 additions & 10 deletions blocks/api/test/parser.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,10 @@ import {
getBlocks,
setUnknownTypeHandler,
} from '../registration';
import {
edit,
save
} from './function-refs.js';

describe( 'block parser', () => {
afterEach( () => {
Expand Down Expand Up @@ -87,7 +91,10 @@ describe( 'block parser', () => {

describe( 'createBlockWithFallback', () => {
it( 'should create the requested block if it exists', () => {
registerBlock( 'core/test-block', {} );
registerBlock( 'core/test-block', {
edit: edit,
save: save
} );

const block = createBlockWithFallback(
'core/test-block',
Expand All @@ -99,15 +106,21 @@ describe( 'block parser', () => {
} );

it( 'should create the requested block with no attributes if it exists', () => {
registerBlock( 'core/test-block', {} );
registerBlock( 'core/test-block', {
edit: edit,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

save: save
} );

const block = createBlockWithFallback( 'core/test-block', 'content' );
expect( block.blockType ).to.eql( 'core/test-block' );
expect( block.attributes ).to.eql( {} );
} );

it( 'should fall back to the unknown type handler for unknown blocks if present', () => {
registerBlock( 'core/unknown-block', {} );
registerBlock( 'core/unknown-block', {
edit: edit,
save: save
} );
setUnknownTypeHandler( 'core/unknown-block' );

const block = createBlockWithFallback(
Expand All @@ -120,7 +133,10 @@ describe( 'block parser', () => {
} );

it( 'should fall back to the unknown type handler if block type not specified', () => {
registerBlock( 'core/unknown-block', {} );
registerBlock( 'core/unknown-block', {
edit: edit,
save: save
} );
setUnknownTypeHandler( 'core/unknown-block' );

const block = createBlockWithFallback( null, 'content' );
Expand All @@ -142,7 +158,9 @@ describe( 'block parser', () => {
return {
content: rawContent,
};
}
},
edit: edit,
save: save
} );

const parsed = parse(
Expand All @@ -166,7 +184,9 @@ describe( 'block parser', () => {
return {
content: rawContent + ' & Chicken'
};
}
},
edit: edit,
save: save
} );

const parsed = parse(
Expand All @@ -184,8 +204,14 @@ describe( 'block parser', () => {
} );

it( 'should parse the post content, using unknown block handler', () => {
registerBlock( 'core/test-block', {} );
registerBlock( 'core/unknown-block', {} );
registerBlock( 'core/test-block', {
edit: edit,
save: save
} );
registerBlock( 'core/unknown-block', {
edit: edit,
save: save
} );

setUnknownTypeHandler( 'core/unknown-block' );

Expand All @@ -204,14 +230,19 @@ describe( 'block parser', () => {
} );

it( 'should parse the post content, including raw HTML at each end', () => {
registerBlock( 'core/test-block', {} );
registerBlock( 'core/test-block', {
edit: edit,
save: save
} );
registerBlock( 'core/unknown-block', {
// Currently this is the only way to test block content parsing?
attributes: function( rawContent ) {
return {
content: rawContent,
};
}
},
edit: edit,
save: save
} );

setUnknownTypeHandler( 'core/unknown-block' );
Expand Down
Loading