Skip to content

Commit

Permalink
Parser: synchronous → asynchronous execution
Browse files Browse the repository at this point in the history
In this patch **we're changing the execution model of the `post_content` parser from _synchronous_ to _asynchronous_**.

```js
const doc = '<!-- wp:paragraph --><p>Wee!</p><!-- /wp:paragraph -->';

const parseSyncNowBroken = ( doc ) => {
	const parsed = parseWithGrammar( doc );
	return parsed.length === 1;
}

const parseAsyncNowWorks = ( doc ) => {
	return parseWithGrammar( doc ).then( parsed => {
		return parsed.length === 1;
	} );
}

const usingAsyncAwait = async ( doc ) => {
	const parsed = await parseWithGrammar( doc );
	return parsed.length === 1;
}
```

So far whenever we have relied on parsing the raw content from `post_content` we have done so synchronously and waited for the parser to finish before continuing execution. When the parse is instant and built in a synchronous manner this works well. It imposes some limitations though that we may not want.

 - execution flow is straightforward
 - loading state is "free" because nothing renders until parse finishes

 - execution of the remainder of the app waits for every parse
 - cannot run parser in `WebWorker` or other context
 - cannot run parsers written in an asynchronous manner

These limitations are things we anticipated and even since the beginnings of the project we could assume that at some point we would want an asynchronous model. Recently @Hywan wrote a fast implementation of the project's parser specification but the output depends on an asynchronous model. In other words, the timing is right for us to adopt this change.

 - parsing doesn't block the UI
 - parsing can happen in a `WebWorker`, over the network, or in any asynchronous manner

 - UI _must_ become async-aware, the loading state is no longer "free"
 - race conditions _can_ appear if not planned for and avoided

Sadly once we enter an asynchronous world we invite complexities and race conditions. The code in this PR so-far doesn't address either of these. The first thing you might notice is that when loading a document in the editor we end up loading a blank document for a spit-second before we load the parsed document. If you don't see this then modify `post-parser.js` to this instead and it will become obvious…

```js
import { parse as syncParse } from './post.pegjs';

export const parse = ( document ) => new Promise( ( resolve ) => {
	setTimeout( () => resolve( syncParse( document ) ), 2500 );
} );
```

With this change we are simulating that it takes 2.5s to parse our document. You should see an empty document load in the editor immediately and then after the delay the parsed document will surprisingly appear. During that initial delay we can interact with the empty document and this means that we can create a state where we mess up the document and its history - someone will think they lost their post.

For the current parsers this shouldn't be a practical problem since the parse is fast but likely people will see an initial flash.

To mitigate this problem we need to somehow introduce a loading state for the editor: "we are in the process of loading the initial document" and that can appear as a message where the contents would otherwise reside or it could simply be a blank area deferring the render until ready. A common approach I have seen is to keep things blanked out unless the operation takes longer than a given threshold to complete so as not to jar the experience with the flashing loading message, but that's really a detail that isn't the most important thing right now.

As for the race condition we may need to consider the risk and the cost of the solution. Since I think the flash would likely be more jarring than the race condition likely it may be a problem we can feasibly defer. The biggest risk is probably when we have code rapidly calling `parse()` in sequence and the results come back out of order or if they start from different copies of the original document. One way we can mitigate this is by enforcing a constraint on the parser to be a single actor and only accept (or queue up instead) parsing requests until the current work is finished.

 - Please review the code changes here and reflect on their implications. The tests had to change and so do all functions that rely on parsing the `post_content` - they must become asynchronous themselves.
 - Please consider the race conditions and the experience implications and weigh in on how severe you estimate them to be.
 - Please share any further thoughts or ideas you may have concerning the sync vs. async behavior.
  • Loading branch information
aduth committed Aug 21, 2018
1 parent 510b38b commit 13560d1
Show file tree
Hide file tree
Showing 16 changed files with 139 additions and 107 deletions.
5 changes: 4 additions & 1 deletion docs/reference/faq.md
Original file line number Diff line number Diff line change
Expand Up @@ -161,10 +161,13 @@ This also [gives us the flexibility](https://github.com/WordPress/gutenberg/issu
We suggest you look at the [language of Gutenberg](../../docs/language.md) to learn more about how this aspect of the project works.

## How can I parse the post content back out into blocks in PHP or JS?

In JS:

```js
var blocks = wp.blocks.parse( postContent );
wp.blocks.parse( postContent ).then( function( blocks ) {
// ...
} );
```

In PHP:
Expand Down
4 changes: 2 additions & 2 deletions packages/block-library/src/paragraph/edit.native.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,9 @@ class ParagraphEdit extends Component {
...style,
minHeight: Math.max( minHeight, typeof attributes.aztecHeight === 'undefined' ? 0 : attributes.aztecHeight ),
} }
onChange={ ( event ) => {
onChange={ async ( event ) => {
// Create a React Tree from the new HTML
const newParaBlock = parse( '<!-- wp:paragraph --><p>' + event.content + '</p><!-- /wp:paragraph -->' )[ 0 ];
const newParaBlock = await parse( '<!-- wp:paragraph --><p>' + event.content + '</p><!-- /wp:paragraph -->' )[ 0 ];
setAttributes( {
...this.props.attributes,
content: newParaBlock.attributes.content,
Expand Down
8 changes: 6 additions & 2 deletions packages/blocks/src/api/parser.js
Original file line number Diff line number Diff line change
Expand Up @@ -357,7 +357,7 @@ export function createBlockWithFallback( blockNode ) {
* @return {Function} An implementation which parses the post content.
*/
const createParse = ( parseImplementation ) =>
( content ) => parseImplementation( content ).reduce( ( memo, blockNode ) => {
async ( content ) => ( await parseImplementation( content ) ).reduce( ( memo, blockNode ) => {
const block = createBlockWithFallback( blockNode );
if ( block ) {
memo.push( block );
Expand All @@ -372,6 +372,10 @@ const createParse = ( parseImplementation ) =>
*
* @return {Array} Block list.
*/
export const parseWithGrammar = createParse( grammarParse );
export const parseWithGrammar = createParse( ( postContent ) => {
const result = grammarParse( postContent );

return Promise.resolve( result );
} );

export default parseWithGrammar;
10 changes: 5 additions & 5 deletions packages/blocks/src/api/raw-handling/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ function getRawTransformations() {
* @param {Array} [options.tagName] The tag into which content will be inserted.
* @param {boolean} [options.canUserUseUnfilteredHTML] Whether or not the user can use unfiltered HTML.
*
* @return {Array|string} A list of blocks or a string, depending on `handlerMode`.
* @return {Promise<Array|string>} A list of blocks or a string, depending on `handlerMode`.
*/
export default function rawHandler( { HTML = '', plainText = '', mode = 'AUTO', tagName, canUserUseUnfilteredHTML = false } ) {
// First of all, strip any meta tags.
Expand Down Expand Up @@ -121,7 +121,7 @@ export default function rawHandler( { HTML = '', plainText = '', mode = 'AUTO',
}

if ( mode === 'INLINE' ) {
return filterInlineHTML( HTML );
return Promise.resolve( filterInlineHTML( HTML ) );
}

// An array of HTML strings and block objects. The blocks replace matched
Expand All @@ -134,14 +134,14 @@ export default function rawHandler( { HTML = '', plainText = '', mode = 'AUTO',
const hasShortcodes = pieces.length > 1;

if ( mode === 'AUTO' && ! hasShortcodes && isInlineContent( HTML, tagName ) ) {
return filterInlineHTML( HTML );
return Promise.resolve( filterInlineHTML( HTML ) );
}

const rawTransformations = getRawTransformations();
const phrasingContentSchema = getPhrasingContentSchema();
const blockContentSchema = getBlockContentSchema( rawTransformations );

return compact( flatMap( pieces, ( piece ) => {
return Promise.resolve( compact( flatMap( pieces, ( piece ) => {
// Already a block from shortcode.
if ( typeof piece !== 'string' ) {
return piece;
Expand Down Expand Up @@ -206,5 +206,5 @@ export default function rawHandler( { HTML = '', plainText = '', mode = 'AUTO',
)
);
} );
} ) );
} ) ) );
}
40 changes: 20 additions & 20 deletions packages/blocks/src/api/test/parser.js
Original file line number Diff line number Diff line change
Expand Up @@ -554,7 +554,7 @@ describe( 'block parser', () => {

// encapsulate the test cases so we can run them multiple time but with a different parse() function
function testCases( parse ) {
it( 'should parse the post content, including block attributes', () => {
it( 'should parse the post content, including block attributes', async () => {
registerBlockType( 'core/test-block', {
attributes: {
content: {
Expand All @@ -570,7 +570,7 @@ describe( 'block parser', () => {
title: 'test block',
} );

const parsed = parse(
const parsed = await parse(
`<!-- wp:core/test-block {"smoked":"yes","url":"http://google.com","chicken":"ribs & 'wings'"} -->` +
'Brisket' +
'<!-- /wp:core/test-block -->'
Expand All @@ -587,13 +587,13 @@ describe( 'block parser', () => {
expect( typeof parsed[ 0 ].clientId ).toBe( 'string' );
} );

it( 'should parse empty post content', () => {
const parsed = parse( '' );
it( 'should parse empty post content', async () => {
const parsed = await parse( '' );

expect( parsed ).toEqual( [] );
} );

it( 'should parse the post content, ignoring unknown blocks', () => {
it( 'should parse the post content, ignoring unknown blocks', async () => {
registerBlockType( 'core/test-block', {
attributes: {
content: {
Expand All @@ -606,7 +606,7 @@ describe( 'block parser', () => {
title: 'test block',
} );

const parsed = parse(
const parsed = await parse(
'<!-- wp:core/test-block -->\nRibs\n<!-- /wp:core/test-block -->' +
'<p>Broccoli</p>' +
'<!-- wp:core/unknown-block -->Ribs<!-- /wp:core/unknown-block -->'
Expand All @@ -620,23 +620,23 @@ describe( 'block parser', () => {
expect( typeof parsed[ 0 ].clientId ).toBe( 'string' );
} );

it( 'should add the core namespace to un-namespaced blocks', () => {
it( 'should add the core namespace to un-namespaced blocks', async () => {
registerBlockType( 'core/test-block', defaultBlockSettings );

const parsed = parse(
const parsed = await parse(
'<!-- wp:test-block {"fruit":"Bananas"} -->\nBananas\n<!-- /wp:test-block -->'
);

expect( parsed ).toHaveLength( 1 );
expect( parsed[ 0 ].name ).toBe( 'core/test-block' );
} );

it( 'should ignore blocks with a bad namespace', () => {
it( 'should ignore blocks with a bad namespace', async () => {
registerBlockType( 'core/test-block', defaultBlockSettings );

setUnknownTypeHandlerName( 'core/unknown-block' );

const parsed = parse(
const parsed = await parse(
'<!-- wp:test-block {"fruit":"Bananas"} -->\nBananas\n<!-- /wp:test-block -->' +
'<p>Broccoli</p>' +
'<!-- wp:core/unknown/block -->Ribs<!-- /wp:core/unknown/block -->'
Expand All @@ -645,13 +645,13 @@ describe( 'block parser', () => {
expect( parsed[ 0 ].name ).toBe( 'core/test-block' );
} );

it( 'should parse the post content, using unknown block handler', () => {
it( 'should parse the post content, using unknown block handler', async () => {
registerBlockType( 'core/test-block', defaultBlockSettings );
registerBlockType( 'core/unknown-block', unknownBlockSettings );

setUnknownTypeHandlerName( 'core/unknown-block' );

const parsed = parse(
const parsed = await parse(
'<!-- wp:test-block {"fruit":"Bananas"} -->\nBananas\n<!-- /wp:test-block -->' +
'<p>Broccoli</p>' +
'<!-- wp:core/unknown-block -->Ribs<!-- /wp:core/unknown-block -->'
Expand All @@ -665,13 +665,13 @@ describe( 'block parser', () => {
] );
} );

it( 'should parse the post content, including raw HTML at each end', () => {
it( 'should parse the post content, including raw HTML at each end', async () => {
registerBlockType( 'core/test-block', defaultBlockSettings );
registerBlockType( 'core/unknown-block', unknownBlockSettings );

setUnknownTypeHandlerName( 'core/unknown-block' );

const parsed = parse(
const parsed = await parse(
'<p>Cauliflower</p>' +
'<!-- wp:test-block {"fruit":"Bananas"} -->\nBananas\n<!-- /wp:test-block -->' +
'\n<p>Broccoli</p>\n' +
Expand All @@ -692,9 +692,9 @@ describe( 'block parser', () => {
expect( parsed[ 4 ].attributes.content ).toEqual( '<p>Romanesco</p>' );
} );

it( 'should parse blocks with empty content', () => {
it( 'should parse blocks with empty content', async () => {
registerBlockType( 'core/test-block', defaultBlockSettings );
const parsed = parse(
const parsed = await parse(
'<!-- wp:core/test-block --><!-- /wp:core/test-block -->'
);

Expand All @@ -704,10 +704,10 @@ describe( 'block parser', () => {
] );
} );

it( 'should parse void blocks', () => {
it( 'should parse void blocks', async () => {
registerBlockType( 'core/test-block', defaultBlockSettings );
registerBlockType( 'core/void-block', defaultBlockSettings );
const parsed = parse(
const parsed = await parse(
'<!-- wp:core/test-block --><!-- /wp:core/test-block -->' +
'<!-- wp:core/void-block /-->'
);
Expand All @@ -718,7 +718,7 @@ describe( 'block parser', () => {
] );
} );

it( 'should parse with unicode escaped returned to original representation', () => {
it( 'should parse with unicode escaped returned to original representation', async () => {
registerBlockType( 'core/code', {
category: 'common',
title: 'Code Block',
Expand All @@ -733,7 +733,7 @@ describe( 'block parser', () => {
const content = '$foo = "My \"escaped\" text.";';
const block = createBlock( 'core/code', { content } );
const serialized = serialize( block );
const parsed = parse( serialized );
const parsed = await parse( serialized );
expect( parsed[ 0 ].attributes.content ).toBe( content );
} );
}
Expand Down
4 changes: 2 additions & 2 deletions packages/editor/src/components/block-drop-zone/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,8 @@ class BlockDropZone extends Component {
}
}

onHTMLDrop( HTML, position ) {
const blocks = rawHandler( { HTML, mode: 'BLOCKS' } );
async onHTMLDrop( HTML, position ) {
const blocks = await rawHandler( { HTML, mode: 'BLOCKS' } );

if ( blocks.length ) {
this.props.insertBlocks( blocks, this.getInsertIndex( position ) );
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,8 @@ export default withDispatch( ( dispatch, { block } ) => {
content: block.originalContent,
} ) );
},
convertToBlocks() {
replaceBlock( block.clientId, rawHandler( {
async convertToBlocks() {
replaceBlock( block.clientId, await rawHandler( {
HTML: block.originalContent,
mode: 'BLOCKS',
} ) );
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,9 @@ export default compose(
};
} ),
withDispatch( ( dispatch, { block, canUserUseUnfilteredHTML } ) => ( {
onClick: () => dispatch( 'core/editor' ).replaceBlocks(
onClick: async () => dispatch( 'core/editor' ).replaceBlocks(
block.clientId,
rawHandler( {
await rawHandler( {
HTML: getBlockContent( block ),
mode: 'BLOCKS',
canUserUseUnfilteredHTML,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,9 @@ export default compose(
};
} ),
withDispatch( ( dispatch, { block, canUserUseUnfilteredHTML } ) => ( {
onClick: () => dispatch( 'core/editor' ).replaceBlocks(
onClick: async () => dispatch( 'core/editor' ).replaceBlocks(
block.clientId,
rawHandler( {
await rawHandler( {
HTML: serialize( block ),
mode: 'BLOCKS',
canUserUseUnfilteredHTML,
Expand Down
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 @@ -111,8 +111,8 @@ export default compose( [
onChange( content ) {
editPost( { content } );
},
onPersist( content ) {
resetBlocks( parse( content ) );
async onPersist( content ) {
resetBlocks( await parse( content ) );
checkTemplateValidity();
},
};
Expand Down
7 changes: 4 additions & 3 deletions packages/editor/src/components/rich-text/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -251,7 +251,7 @@ export class RichText extends Component {
*
* @param {PasteEvent} event The paste event as triggered by TinyMCE.
*/
onPaste( event ) {
async onPaste( event ) {
const clipboardData = event.clipboardData;
const { items = [], files = [] } = clipboardData;
const item = find( [ ...items, ...files ], ( { type } ) => /^image\/(?:jpe?g|png|gif)$/.test( type ) );
Expand Down Expand Up @@ -285,11 +285,12 @@ export class RichText extends Component {
// Note: a pasted file may have the URL as plain text.
if ( item && ! html ) {
const file = item.getAsFile ? item.getAsFile() : item;
const content = rawHandler( {
const content = await rawHandler( {
HTML: `<img src="${ createBlobURL( file ) }">`,
mode: 'BLOCKS',
tagName: this.props.tagName,
} );

const shouldReplace = this.props.onReplace && this.isEmpty();

// Allows us to ask for this information when we get a report.
Expand Down Expand Up @@ -335,7 +336,7 @@ export class RichText extends Component {
mode = 'AUTO';
}

const content = rawHandler( {
const content = await rawHandler( {
HTML: html,
plainText,
mode,
Expand Down
42 changes: 25 additions & 17 deletions packages/editor/src/store/effects.js
Original file line number Diff line number Diff line change
Expand Up @@ -117,30 +117,33 @@ export default {
]
) );
},
SETUP_EDITOR( action, { getState } ) {
SETUP_EDITOR( action, { dispatch, getState } ) {
const { post, autosave } = action;
const state = getState();
const template = getTemplate( state );
const templateLock = getTemplateLock( state );

// Parse content as blocks
let blocks;
let waitForBlocks;
let isValidTemplate = true;
if ( post.content.raw ) {
blocks = parse( post.content.raw );
waitForBlocks = parse( post.content.raw );

// Unlocked templates are considered always valid because they act as default values only.
isValidTemplate = (
! template ||
templateLock !== 'all' ||
doBlocksMatchTemplate( blocks, template )
);
waitForBlocks.then( ( blocks ) => {
// Unlocked templates are considered always valid because they
// act as default values only.
isValidTemplate = (
! template ||
templateLock !== 'all' ||
doBlocksMatchTemplate( blocks, template )
);
} );
} else if ( template ) {
blocks = synchronizeBlocksWithTemplate( [], template );
waitForBlocks = Promise.resolve( synchronizeBlocksWithTemplate( [], template ) );
} else if ( getDefaultBlockForPostFormat( post.format ) ) {
blocks = [ createBlock( getDefaultBlockForPostFormat( post.format ) ) ];
waitForBlocks = Promise.resolve( [ createBlock( getDefaultBlockForPostFormat( post.format ) ) ] );
} else {
blocks = [];
waitForBlocks = Promise.resolve( [] );
}

// Include auto draft title in edits while not flagging post as dirty
Expand All @@ -166,11 +169,16 @@ export default {
);
}

return [
setTemplateValidity( isValidTemplate ),
setupEditorState( post, blocks, edits ),
...( autosaveAction ? [ autosaveAction ] : [] ),
];
if ( autosaveAction ) {
dispatch( autosaveAction );
}

waitForBlocks.then( ( blocks ) => {
// TODO: Do we need to always set template validity, if the default
// is true and in most cases it will already be valid?
dispatch( setTemplateValidity( isValidTemplate ) );
dispatch( setupEditorState( post, blocks, edits ) );
} );
},
SYNCHRONIZE_TEMPLATE( action, { getState } ) {
const state = getState();
Expand Down
Loading

0 comments on commit 13560d1

Please sign in to comment.