Editor: Make sure `resetEditorBlocks` is synchronous. #21839

epiqueras · 2020-04-23T20:10:40Z

Description

#21467 introduced a resolver for getEditedEntityRecord which made resetEditorBlocks asynchronous. This sometimes makes previous calls finish executing after newer ones, essentially undoing the editor to a stale block tree.

I added comments to the relevant code to explain:

export function* resetEditorBlocks( blocks, options = {} ) {
	const {
		__unstableShouldCreateUndoLevel,
		selectionStart,
		selectionEnd,
	} = options;
	const edits = { blocks, selectionStart, selectionEnd };
	// First Run: `[ core/template-part: [] ], undefined`.
	// Second Run: `[ core/template-part: [ ...initialBlocks ] ], false`.
	console.log( blocks, __unstableShouldCreateUndoLevel );
	if ( __unstableShouldCreateUndoLevel !== false ) {
		const { id, type } = yield select( STORE_KEY, 'getCurrentPost' );
		const noChange =
			( yield select(
				'core',
				'getEditedEntityRecord',
				'postType',
				type,
				id
			) ).blocks === edits.blocks;
		// First Run: `false`.
		// Second Run: Doesn't reach this code.
		// getEditedEntityRecord having a resolver makes
		// the execution from the first run reach this point
		// after the second run finishes. This overwrites the
		// blocks with a stale value. A stale value without
		// template part inner blocks which causes the issue
		// described here.
		console.log( noChange );
		if ( noChange ) {
			return yield dispatch(
				'core',
				'__unstableCreateUndoLevel',
				'postType',
				type,
				id
			);
		}
		// We create a new function here on every persistent edit
		// to make sure the edit makes the post dirty and creates
		// a new undo level.
		edits.content = ( { blocks: blocksForSerialization = [] } ) =>
			serializeBlocks( blocksForSerialization );
	}
	yield* editPost( edits );
}

This PR fixes the issue by using a synchronous select data control like we used to have before.

But, I think that this problem could surface elsewhere, so the long-term solution should be more general. We should think about canceling stale action execution. That also raises the questions of how to define and continue with any cleanup operations, though.

How to test this?

Verify that #21804 is no longer an issue; that you can insert existing template parts on the full site editing experiment as expected.

Types of Changes

Bug Fix: resetEditorBlocks will now execute sequentially, avoiding an issue where older executions would overwrite newer ones.
New Feature: @wordpress/data-controls now has a synchronous version of select, syncSelect, which does not wait for resolvers.

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
My code has proper inline documentation.
I've included developer documentation if appropriate.
I've updated all React Native files affected by any refactorings/renamings in this PR.

github-actions · 2020-04-23T20:16:44Z

Size Change: +47 B (0%)

Total Size: 845 kB

Filename	Size	Change
`build/data-controls/index.js`	1.29 kB	+40 B (3%)
`build/editor/index.js`	43.4 kB	+7 B (0%)

ℹ️ View Unchanged

Filename	Size	Change
`build/a11y/index.js`	1.02 kB	0 B
`build/annotations/index.js`	3.62 kB	0 B
`build/api-fetch/index.js`	4.08 kB	0 B
`build/autop/index.js`	2.82 kB	0 B
`build/blob/index.js`	620 B	0 B
`build/block-directory/index.js`	6.24 kB	0 B
`build/block-directory/style-rtl.css`	760 B	0 B
`build/block-directory/style.css`	761 B	0 B
`build/block-editor/index.js`	105 kB	0 B
`build/block-editor/style-rtl.css`	10.1 kB	0 B
`build/block-editor/style.css`	10.1 kB	0 B
`build/block-library/editor-rtl.css`	7.13 kB	0 B
`build/block-library/editor.css`	7.13 kB	0 B
`build/block-library/index.js`	112 kB	0 B
`build/block-library/style-rtl.css`	7.19 kB	0 B
`build/block-library/style.css`	7.19 kB	0 B
`build/block-library/theme-rtl.css`	683 B	0 B
`build/block-library/theme.css`	685 B	0 B
`build/block-serialization-default-parser/index.js`	1.88 kB	0 B
`build/block-serialization-spec-parser/index.js`	3.1 kB	0 B
`build/blocks/index.js`	57.7 kB	0 B
`build/components/index.js`	198 kB	0 B
`build/components/style-rtl.css`	16.9 kB	0 B
`build/components/style.css`	16.9 kB	0 B
`build/compose/index.js`	6.66 kB	0 B
`build/core-data/index.js`	11.4 kB	0 B
`build/data/index.js`	8.43 kB	0 B
`build/date/index.js`	5.47 kB	0 B
`build/deprecated/index.js`	772 B	0 B
`build/dom-ready/index.js`	569 B	0 B
`build/dom/index.js`	3.1 kB	0 B
`build/edit-navigation/index.js`	3.54 kB	0 B
`build/edit-navigation/style-rtl.css`	485 B	0 B
`build/edit-navigation/style.css`	485 B	0 B
`build/edit-post/index.js`	28.2 kB	0 B
`build/edit-post/style-rtl.css`	12.5 kB	0 B
`build/edit-post/style.css`	12.5 kB	0 B
`build/edit-site/index.js`	10.8 kB	0 B
`build/edit-site/style-rtl.css`	5.25 kB	0 B
`build/edit-site/style.css`	5.24 kB	0 B
`build/edit-widgets/index.js`	8.33 kB	0 B
`build/edit-widgets/style-rtl.css`	5 kB	0 B
`build/edit-widgets/style.css`	5 kB	0 B
`build/editor/editor-styles-rtl.css`	428 B	0 B
`build/editor/editor-styles.css`	431 B	0 B
`build/editor/style-rtl.css`	3.27 kB	0 B
`build/editor/style.css`	3.27 kB	0 B
`build/element/index.js`	4.65 kB	0 B
`build/escape-html/index.js`	733 B	0 B
`build/format-library/index.js`	7.32 kB	0 B
`build/format-library/style-rtl.css`	502 B	0 B
`build/format-library/style.css`	502 B	0 B
`build/hooks/index.js`	2.13 kB	0 B
`build/html-entities/index.js`	622 B	0 B
`build/i18n/index.js`	3.56 kB	0 B
`build/is-shallow-equal/index.js`	711 B	0 B
`build/keyboard-shortcuts/index.js`	2.51 kB	0 B
`build/keycodes/index.js`	1.94 kB	0 B
`build/list-reusable-blocks/index.js`	3.12 kB	0 B
`build/list-reusable-blocks/style-rtl.css`	226 B	0 B
`build/list-reusable-blocks/style.css`	226 B	0 B
`build/media-utils/index.js`	5.29 kB	0 B
`build/notices/index.js`	1.79 kB	0 B
`build/nux/index.js`	3.4 kB	0 B
`build/nux/style-rtl.css`	616 B	0 B
`build/nux/style.css`	613 B	0 B
`build/plugins/index.js`	2.67 kB	0 B
`build/primitives/index.js`	1.49 kB	0 B
`build/priority-queue/index.js`	789 B	0 B
`build/redux-routine/index.js`	2.84 kB	0 B
`build/rich-text/index.js`	14.8 kB	0 B
`build/server-side-render/index.js`	2.67 kB	0 B
`build/shortcode/index.js`	1.7 kB	0 B
`build/token-list/index.js`	1.28 kB	0 B
`build/url/index.js`	4.02 kB	0 B
`build/viewport/index.js`	1.84 kB	0 B
`build/warning/index.js`	1.14 kB	0 B
`build/wordcount/index.js`	1.17 kB	0 B

_{compressed-size-action}

aduth · 2020-04-23T20:25:02Z

Verify that #21804 is no longer an issue; that you can insert existing template parts on the full site editing experiment as expected.

Since it took me a while to figure out the testing instructions:

(Prerequisite) Activate and save "Enable Full Site Editing" from Gutenberg > Experiments
Navigate to Posts > Add New
Insert a "Template Part" block
Enter "header" for Slug and "twentytwenty" for Theme
Press "Choose"
Note the text "Header Template Part" appears
- In master, it's wrongly empty prompt "Start writing or type / to choose block"

aduth · 2020-04-23T20:26:23Z

But, I think that this problem could surface elsewhere, so the long-term solution should be more general. We should think about canceling stale action execution. That also raises the questions of how to define and continue with any cleanup operations, though.

Do we want a new public API member syncSelect then? Should it be __unstable? Or local to the editor package like we do with some similar controls?† Or is there valid use for this beyond addressing the immediate issue here?

† I believe this example isn't done this way on purpose, merely that it was implemented before the package was generalized.

aduth · 2020-04-23T20:27:25Z

Do we risk any inaccurate value from getEditedEntityRecord by not awaiting the resolver? I guess if it wasn't awaiting anything before, this would restore that previous behavior? (Which doesn't speak to whether or not it should be waiting for e.g. getEntityRecord).

aduth · 2020-04-23T20:28:49Z

Not sure if this was introduced here or in #21804, or if it's always been an issue, but I see a lot of duplicate toolbar and inspector controls for a freshly-inserted Template Part block. Plus, when trying to interact with the block, it locks up my editor.

It doesn't seem to be an issue when saving and reloading.

Edit: Testing it further, it might only be an issue when inserting a Template Part block when there are already Template Part blocks in the post for the same template part. In my case, I effectively had three of the same template part.

aduth · 2020-04-23T20:33:53Z

But, I think that this problem could surface elsewhere, so the long-term solution should be more general. We should think about canceling stale action execution. That also raises the questions of how to define and continue with any cleanup operations, though.

Do we want a new public API member syncSelect then? Should it be __unstable? Or local to the editor package like we do with some similar controls?† Or is there valid use for this beyond addressing the immediate issue here?

On the topic, while I don't think it's inaccurate to name it as such, the specific use of "sync" leads me to think of it differently, in-fact more like how it actually behaves today. When I see the name, I consider it like Node's readFileSync, where it would wait before proceeding. The actual behavior is more like don't wait, just use state as it exists at the moment.

aduth

I'd probably prefer to hide syncSelect from the public API in its current form, but otherwise it appears to fix the issue in my testing.

Separately, we should consider some automated testing coverage for this behavior, especially since it's fixing a regression.

epiqueras · 2020-04-23T20:53:23Z

Do we risk any inaccurate value from getEditedEntityRecord by not awaiting the resolver? I guess if it wasn't awaiting anything before, this would restore that previous behavior? (Which doesn't speak to whether or not it should be waiting for e.g. getEntityRecord).

No, it should be fine.

Not sure if this was introduced here or in #21804, or if it's always been an issue, but I see a lot of duplicate toolbar and inspector controls for a freshly-inserted Template Part block. Plus, when trying to interact with the block, it locks up my editor.

Yes, this is because they are the same template parts, so they share block instances/IDs. We need to fix this.

On the topic, while I don't think it's inaccurate to name it as such, the specific use of "sync" leads me to think of it differently, in-fact more like how it actually behaves today. When I see the name, I consider it like Node's readFileSync, where it would wait before proceeding. The actual behavior is more like don't wait, just use state as it exists at the moment.

What would you call it? noResolverSelect seemed too wordy.

I'd probably prefer to hide syncSelect from the public API in its current form, but otherwise it appears to fix the issue in my testing.

Agreed, I'll make it unstable for now. But, we need to arrive at a long-term solution because this will also be a problem for third parties.

Separately, we should consider some automated testing coverage for this behavior, especially since it's fixing a regression.

Yes, I want to start with e2e tests for template parts, which should indirectly test a lot of the new behaviors.

aduth · 2020-04-23T21:02:07Z

What would you call it? noResolverSelect seemed too wordy.

I guess it doesn't really matter much if it's going to be internal / unstable. I don't really have great name alternatives in mind, maybe something reflecting the "immediacy" or "current state" aspects of the behavior. I also kinda wished it would be an option of the existing select, but given the variadic nature of the existing function, we can't easily add an "options" argument. I'm not too worried about changing anything.

aduth · 2020-04-23T21:08:33Z

I'd probably prefer to hide syncSelect from the public API in its current form, but otherwise it appears to fix the issue in my testing.

Agreed, I'll make it unstable for now. But, we need to arrive at a long-term solution because this will also be a problem for third parties.

Yeah, but unless I'm mistaken, we don't want anything like this to be the end solution, which also makes it a perfect use-case for __unstable. The fact that this became an issue isn't a result of us explicitly wanting to not wait for resolvers attached to the selectors called, it's just that we unintentionally depended on that behavior. I think what you've suggested about cancelling or queueing these actions / resolvers to occur in a predictable way would be much more durable, and I'd doubt it would require the syncSelect introduced here.

epiqueras · 2020-04-23T21:13:58Z

I guess it doesn't really matter much if it's going to be internal / unstable. I don't really have great name alternatives in mind, maybe something reflecting the "immediacy" or "current state" aspects of the behavior. I also kinda wished it would be an option of the existing select, but given the variadic nature of the existing function, we can't easily add an "options" argument. I'm not too worried about changing anything.

Yeah, I reached for that first as well.

Yeah, but unless I'm mistaken, we don't want anything like this to be the end solution, which also makes it a perfect use-case for __unstable. The fact that this became an issue isn't a result of us explicitly wanting to not wait for resolvers attached to the selectors called, it's just that we unintentionally depended on that behavior. I think what you've suggested about cancelling or queueing these actions / resolvers to occur in a predictable way would be much more durable, and I'd doubt it would require the syncSelect introduced here.

Exactly, that's what I was referring too.

epiqueras added 2 commits April 23, 2020 12:59

Data Controls: Add a synchronous select control.

1ebd7d2

Editor: Use syncSelect in resetEditorBlocks.

da630d6

epiqueras added [Type] Bug An existing feature does not function as intended [Feature] Nested / Inner Blocks Anything related to the experience of nested/inner blocks inside a larger container, like Group or P [Feature] Full Site Editing labels Apr 23, 2020

epiqueras self-assigned this Apr 23, 2020

epiqueras requested review from Addison-Stavlo and aduth April 23, 2020 20:11

aduth approved these changes Apr 23, 2020

View reviewed changes

Data Controls: Make syncSelect "__unstable".

c14a336

epiqueras merged commit 8eb5a47 into master Apr 23, 2020

epiqueras deleted the fix/async-reset-editor-blocks branch April 23, 2020 21:15

github-actions bot added this to the Gutenberg 8.0 milestone Apr 23, 2020

jsnajdr mentioned this pull request Oct 13, 2020

Migrate the 'editor' package to builtin data controls #25990

Merged

dependabot bot mentioned this pull request Mar 18, 2021

Bump @wordpress/blocks from 6.24.2 to 8.0.0 LeoColomb/wp-classic-to-blocks#50

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Editor: Make sure `resetEditorBlocks` is synchronous. #21839

Editor: Make sure `resetEditorBlocks` is synchronous. #21839

epiqueras commented Apr 23, 2020

github-actions bot commented Apr 23, 2020 •

edited

Loading

aduth commented Apr 23, 2020

aduth commented Apr 23, 2020

aduth commented Apr 23, 2020

aduth commented Apr 23, 2020 •

edited

Loading

aduth commented Apr 23, 2020

aduth left a comment

epiqueras commented Apr 23, 2020

aduth commented Apr 23, 2020 •

edited

Loading

aduth commented Apr 23, 2020

epiqueras commented Apr 23, 2020

Editor: Make sure resetEditorBlocks is synchronous. #21839

Editor: Make sure resetEditorBlocks is synchronous. #21839

Conversation

epiqueras commented Apr 23, 2020

Description

How to test this?

Types of Changes

Checklist:

github-actions bot commented Apr 23, 2020 • edited Loading

aduth commented Apr 23, 2020

aduth commented Apr 23, 2020

aduth commented Apr 23, 2020

aduth commented Apr 23, 2020 • edited Loading

aduth commented Apr 23, 2020

aduth left a comment

Choose a reason for hiding this comment

epiqueras commented Apr 23, 2020

aduth commented Apr 23, 2020 • edited Loading

aduth commented Apr 23, 2020

epiqueras commented Apr 23, 2020

Editor: Make sure `resetEditorBlocks` is synchronous. #21839

Editor: Make sure `resetEditorBlocks` is synchronous. #21839

github-actions bot commented Apr 23, 2020 •

edited

Loading

aduth commented Apr 23, 2020 •

edited

Loading

aduth commented Apr 23, 2020 •

edited

Loading