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.

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

Editor: Defer block validation until after source application #16569

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -1203,7 +1203,7 @@ Returns an action object used to signal that the blocks have been updated.
_Parameters_

- _blocks_ `Array`: Block Array.
- _options_ `?Object`: Optional options.
- _options_ `?WPEditorResetEditorBlocksActionOptions`: Optional options.

_Returns_

Expand Down
5 changes: 5 additions & 0 deletions packages/blocks/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
## Master

### New Features

- `parse` now accepts an options object. Current options include the ability to disable default validation (`validate`).
- New function: `validate`. Given a block or array of blocks, assigns the `isValid` property of each block corresponding to the validation result.

### Improvements

- Omitting `attributes` or `keywords` settings will now stub default values (an empty object or empty array, respectively).
Expand Down
10 changes: 10 additions & 0 deletions packages/blocks/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -746,6 +746,16 @@ _Parameters_
- _slug_ `string`: Block category slug.
- _category_ `Object`: Object containing the category properties that should be updated.

<a name="validate" href="#validate">#</a> **validate**

Given a block or array of blocks, assigns the `isValid` property of each
block corresponding to the validation result. This mutates the original
array or object.

_Parameters_

- _blocks_ `(WPBlock|Array<WPBlock>)`: Block or array of blocks to validate.

<a name="withBlockContentContext" href="#withBlockContentContext">#</a> **withBlockContentContext**

A Higher Order Component used to inject BlockContent using context to the
Expand Down
5 changes: 4 additions & 1 deletion packages/blocks/src/api/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,10 @@ export {
getSaveElement,
getSaveContent,
} from './serializer';
export { isValidBlockContent } from './validation';
export {
default as validate,
isValidBlockContent,
} from './validation';
export {
getCategories,
setCategories,
Expand Down
47 changes: 37 additions & 10 deletions packages/blocks/src/api/parser.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,24 @@ import { attr, html, text, query, node, children, prop } from './matchers';
import { normalizeBlockType } from './utils';
import { DEPRECATED_ENTRY_KEYS } from './constants';

/**
* Options for parse.
*
* @typedef {Object} WPBlocksParseOptions
*
* @property {boolean} validate Whether to validate and assign result as
* `isValid` on parsed block results.
*/

/**
* Default options for parse.
*
* @type {WPBlocksParseOptions}
*/
export const DEFAULT_PARSE_OPTIONS = {
validate: true,
};

/**
* Sources which are guaranteed to return a string value.
*
Expand Down Expand Up @@ -373,11 +391,12 @@ export function getMigratedBlock( block, parsedAttributes ) {
/**
* Creates a block with fallback to the unknown type handler.
*
* @param {Object} blockNode Parsed block node.
* @param {Object} blockNode Parsed block node.
* @param {WPBlocksParseOptions} parseOptions Parser options.
*
* @return {?Object} An initialized block object (if possible).
*/
export function createBlockWithFallback( blockNode ) {
export function createBlockWithFallback( blockNode, parseOptions ) {
const { blockName: originalName } = blockNode;
let {
attrs: attributes,
Expand Down Expand Up @@ -477,7 +496,7 @@ export function createBlockWithFallback( blockNode ) {
// provided there are no changes in attributes. The validation procedure thus compares the
// provided source value with the serialized output before there are any modifications to
// the block. When both match, the block is marked as valid.
if ( ! isFallbackBlock ) {
if ( ! isFallbackBlock && parseOptions.validate ) {
block.isValid = isValidBlockContent( blockType, block.attributes, innerHTML );
}

Expand Down Expand Up @@ -535,14 +554,22 @@ export function serializeBlockNode( blockNode, options = {} ) {
*
* @return {Function} An implementation which parses the post content.
*/
const createParse = ( parseImplementation ) =>
( content ) => parseImplementation( content ).reduce( ( memo, blockNode ) => {
const block = createBlockWithFallback( blockNode );
if ( block ) {
memo.push( block );
function createParse( parseImplementation ) {
return ( content, options = DEFAULT_PARSE_OPTIONS ) => {
if ( options !== DEFAULT_PARSE_OPTIONS ) {
options = { ...DEFAULT_PARSE_OPTIONS, ...options };
}
return memo;
}, [] );

return parseImplementation( content ).reduce( ( memo, blockNode ) => {
const block = createBlockWithFallback( blockNode, options );
if ( block ) {
memo.push( block );
}

return memo;
}, [] );
};
}

/**
* Parses the post content with a PegJS grammar and returns a list of blocks.
Expand Down
13 changes: 7 additions & 6 deletions packages/blocks/src/api/test/parser.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import deepFreeze from 'deep-freeze';
* Internal dependencies
*/
import {
DEFAULT_PARSE_OPTIONS,
getBlockAttribute,
getBlockAttributes,
createBlockWithFallback,
Expand Down Expand Up @@ -671,7 +672,7 @@ describe( 'block parser', () => {
blockName: 'core/test-block',
innerHTML: 'Bananas',
attrs: { fruit: 'Bananas' },
} );
}, DEFAULT_PARSE_OPTIONS );
expect( block.name ).toEqual( 'core/test-block' );
expect( block.attributes ).toEqual( { fruit: 'Bananas' } );
} );
Expand All @@ -682,7 +683,7 @@ describe( 'block parser', () => {
const block = createBlockWithFallback( {
blockName: 'core/test-block',
innerHTML: '',
} );
}, DEFAULT_PARSE_OPTIONS );
expect( block.name ).toEqual( 'core/test-block' );
expect( block.attributes ).toEqual( {} );
} );
Expand All @@ -695,7 +696,7 @@ describe( 'block parser', () => {
blockName: 'core/test-block',
innerHTML: 'Bananas',
attrs: { fruit: 'Bananas' },
} );
}, DEFAULT_PARSE_OPTIONS );
expect( block.name ).toBe( 'core/unregistered-block' );
expect( block.attributes.content ).toContain( 'wp:test-block' );
} );
Expand All @@ -706,7 +707,7 @@ describe( 'block parser', () => {

const block = createBlockWithFallback( {
innerHTML: 'content',
} );
}, DEFAULT_PARSE_OPTIONS );
expect( block.name ).toEqual( 'core/freeform-block' );
expect( block.attributes ).toEqual( { content: '<p>content</p>' } );
} );
Expand All @@ -715,7 +716,7 @@ describe( 'block parser', () => {
const block = createBlockWithFallback( {
blockName: 'core/test-block',
innerHTML: '',
} );
}, DEFAULT_PARSE_OPTIONS );
expect( block ).toBeUndefined();
} );

Expand Down Expand Up @@ -749,7 +750,7 @@ describe( 'block parser', () => {
blockName: 'core/test-block',
innerHTML: '<span>Bananas</span>',
attrs: { fruit: 'Bananas' },
} );
}, DEFAULT_PARSE_OPTIONS );
expect( block.name ).toEqual( 'core/test-block' );
expect( block.attributes ).toEqual( { fruit: 'Big Bananas' } );
expect( block.isValid ).toBe( true );
Expand Down
46 changes: 45 additions & 1 deletion packages/blocks/src/api/test/validation.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/**
* Internal dependencies
*/
import {
import validate, {
isValidCharacterReference,
DecodeEntityParser,
getTextPiecesSplitOnWhitespace,
Expand Down Expand Up @@ -662,4 +662,48 @@ describe( 'validation', () => {
expect( isValid ).toBe( true );
} );
} );

describe( 'validate', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we also test for validation failures?

Copy link
Member Author

Choose a reason for hiding this comment

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

Should we also test for validation failures?

I don't think it's necessary. The purpose of these test cases is not to verify validation (which are a separate set of test cases), but to ensure the assignment of the validation result.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense.

it( 'returns undefined', () => {
registerBlockType( 'core/test-block', defaultBlockSettings );

const result = validate( [
{
name: 'core/test-block',
attributes: { fruit: 'Bananas' },
originalContent: 'Bananas',
},
] );

expect( result ).toBeUndefined();
} );

it( 'mutates the block with the validation result', () => {
registerBlockType( 'core/test-block', defaultBlockSettings );

const block = {
name: 'core/test-block',
attributes: { fruit: 'Bananas' },
originalContent: 'Bananas',
};

validate( block );

expect( block.isValid ).toBe( true );
} );

it( 'mutates the blocks array with the validation result', () => {
registerBlockType( 'core/test-block', defaultBlockSettings );

const block = {
name: 'core/test-block',
attributes: { fruit: 'Bananas' },
originalContent: 'Bananas',
};

validate( [ block ] );

expect( block.isValid ).toBe( true );
} );
} );
} );
22 changes: 22 additions & 0 deletions packages/blocks/src/api/validation.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import {
isEqual,
includes,
stubTrue,
castArray,
} from 'lodash';

/**
Expand All @@ -21,6 +22,7 @@ import { decodeEntities } from '@wordpress/html-entities';
*/
import { getSaveContent } from './serializer';
import { normalizeBlockType } from './utils';
import { getBlockType } from './registration';

/**
* Globally matches any consecutive whitespace
Expand Down Expand Up @@ -649,3 +651,23 @@ export function isValidBlockContent( blockTypeOrName, attributes, originalBlockC

return isValid;
}

/**
* Given a block or array of blocks, assigns the `isValid` property of each
* block corresponding to the validation result. This mutates the original
* array or object.
*
* @param {(WPBlock|WPBlock[])} blocks Block or array of blocks to validate.
*/
export default function validate( blocks ) {
// Normalize value to array (support singular argument).
blocks = castArray( blocks );

for ( const block of blocks ) {
block.isValid = isValidBlockContent(
getBlockType( block.name ),
block.attributes,
block.originalContent
);
}
}
4 changes: 2 additions & 2 deletions packages/editor/src/components/post-text-editor/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -99,8 +99,8 @@ export default compose( [
editPost( { content } );
},
onPersist( content ) {
const blocks = parse( content );
resetEditorBlocks( blocks );
const blocks = parse( content, { validate: false } );
resetEditorBlocks( blocks, { validate: true } );
},
};
} ),
Expand Down
30 changes: 25 additions & 5 deletions packages/editor/src/store/actions.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import deprecated from '@wordpress/deprecated';
import { dispatch, select, apiFetch } from '@wordpress/data-controls';
import {
parse,
validate,
synchronizeBlocksWithTemplate,
} from '@wordpress/blocks';
import isShallowEqual from '@wordpress/is-shallow-equal';
Expand All @@ -36,6 +37,17 @@ import {
import { awaitNextStateChange, getRegistry } from './controls';
import * as sources from './block-sources';

/**
* Default values for `resetEditorBlocks` action creator options.
*
* @typedef {Object} WPEditorResetEditorBlocksActionOptions
*
* @property {boolean} validate Whether to run validator over provided blocks.
*/
const DEFAULT_RESET_EDITOR_BLOCKS_OPTIONS = {
validate: false,
};

/**
* Map of Registry instance to WeakMap of dependencies by custom source.
*
Expand Down Expand Up @@ -167,7 +179,7 @@ export function* setupEditor( post, edits, template ) {
content = post.content.raw;
}

let blocks = parse( content );
let blocks = parse( content, { validate: false } );

// Apply a template for new posts only, if exists.
const isNewPost = post.status === 'auto-draft';
Expand All @@ -183,7 +195,7 @@ export function* setupEditor( post, edits, template ) {
edits,
template,
};
yield resetEditorBlocks( blocks );
yield resetEditorBlocks( blocks, { validate: ! isNewPost } );
yield setupEditorState( post );
yield* __experimentalSubscribeSources();
}
Expand Down Expand Up @@ -901,12 +913,16 @@ export function unlockPostSaving( lockName ) {
/**
* Returns an action object used to signal that the blocks have been updated.
*
* @param {Array} blocks Block Array.
* @param {?Object} options Optional options.
* @param {Array} blocks Block Array.
* @param {?WPEditorResetEditorBlocksActionOptions} options Optional options.
*
* @return {Object} Action object
*/
export function* resetEditorBlocks( blocks, options = {} ) {
export function* resetEditorBlocks( blocks, options = DEFAULT_RESET_EDITOR_BLOCKS_OPTIONS ) {
if ( options !== DEFAULT_RESET_EDITOR_BLOCKS_OPTIONS ) {
options = { ...DEFAULT_RESET_EDITOR_BLOCKS_OPTIONS, ...options };
}

const lastBlockAttributesChange = yield select( 'core/block-editor', '__experimentalGetLastBlockAttributeChanges' );

// Sync to sources from block attributes updates.
Expand Down Expand Up @@ -943,6 +959,10 @@ export function* resetEditorBlocks( blocks, options = {} ) {
yield* resetLastBlockSourceDependencies( Array.from( updatedSources ) );
}

if ( options.validate ) {
Copy link
Member Author

@aduth aduth Jul 12, 2019

Choose a reason for hiding this comment

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

I considered not making this an option and instead the default behavior. For standard editor usage, we always call resetEditorBlocks with validate: true. It only really makes sense as an option for usage where a third-party might dispatch this action with the result of a default wp.blocks.parse function call, and thus the re-validation would be redundant (note: it would be redundant, but not otherwise harmful aside the performance overhead).

For standard editor usage, we always call resetEditorBlocks with validate: true.

Edit: In reflection, this statement is totally wrong, and the reason it's set up the way it is is so that we can call resetEditorBlocks without frequent validation, since we call it on e.g. every key press.

validate( blocks );
}

return {
type: 'RESET_EDITOR_BLOCKS',
blocks: yield* getBlocksWithSourcedAttributes( blocks ),
Expand Down