Editor: Reshape editor state for optimized and accurate dirtiness detection #10844

aduth · 2018-10-21T01:38:18Z

This pull request seeks to refactor editor state to to enable a more sensible, accurate, and performant dirty detection routine. Rather than representing isDirty as a reflection of the entirety of the state.editor state object, it's now been moved to only handle the new state.editor.blocks, which contains byClientId and order (previously state.editor.blocksByClientId and state.editor.blockOrder). Dirty detection for fields other than content are reflected by the presence of any keys in state.editor.edits.

This helps to support:

Removal of an undesirable artificial dirtying, which was intended as a temporary solution
Avoid expensive calls to content serialization, notably in isEditedPostAutosaveable (which, as noted in Performance: save components are called for *every* block on *every* keystroke #10427, became very negatively impactful)

Previous attempts to implement this had been unsuccessful in that withChangeDetection adds a property to the combined reducer which was incompatible with Redux's combineReducers (logging an error to the console). To work around this, the combineReducers exported from @wordpress/data was swapped with an in-place-compatible, more-performant variation turbo-combine-reducers (authored by yours truly).

Testing Instructions:

Verify that there is no regressions in the behavior of autosave, namely that it occurs after delay if and only if one of content, title, or excerpt is updated.

Conversely, verify that if a field other than the three aforementioned is updated (e.g. categories added or removed), it is reflected by the UI as needing save ("Save Draft" becomes active). An autosave should not occur, but you should be prompted about unsaved changes if you attempt to leave the page before pressing "Save Draft".

Finally, verify that if changing both one of content, title, excerpt and another field, that the autosave does occur, and once completed, the editor is still in a "dirty" state ("Save Changes" present, after brief delay once save finishes). This is because autosaves do not include other fields.

ellatrix · 2018-10-22T07:48:02Z

packages/editor/src/store/selectors.js

+export function hasChangedContent( state ) {
+	return (
+		state.editor.present.blocks.isDirty ||
+		'content' in state.editor.present.edits


I don't really get what this second condition is checking for.

I don't really get what this second condition is checking for.

I'll add a comment to clarify, but it's a combination of two things which require this consideration:

While content in Visual mode is represented by the blocks state, in Text mode it is tracked by edits.content.

edits is intended to contain only the different property values, so it is expected that the mere presence of any property is an indicator that the value is different than what is known to be saved.

ellatrix · 2018-10-22T11:42:42Z

packages/editor/src/store/reducer.js

+			resetTypes: [ 'SETUP_EDITOR_STATE', 'REQUEST_POST_UPDATE_START' ],
+			ignoreTypes: [ 'RECEIVE_BLOCKS', 'RESET_POST', 'UPDATE_POST' ],
+		} ),
+	] )( {


Is everything below here just indented? It's a bit hard to see the code changes, if there are any.

Is everything below here just indented? It's a bit hard to see the code changes, if there are any.

Yes, no changes aside from indentation.

GitHub has an option to hide whitespace-only changes, which should help make things a bit clearer.

Funny that this Higher order reducer moved from "edits" to just "blocks". It almost feels like we need an editBlocks :)

ellatrix · 2018-10-22T11:49:07Z

To work around this, the combineReducers exported from @wordpress/data was swapped with an in-place-compatible, more-performant variation turbo-combine-reducers (authored by yours truly).

If we're swapping something from @wordpress/data because it is not performant enough, might it not be better to adjust combineReducers in @wordpress/data?

aduth · 2018-10-22T14:14:56Z

If we're swapping something from @wordpress/data because it is not performant enough, might it not be better to adjust combineReducers in @wordpress/data?

To be clear, we're not swapping it because of performance, we're swapping it to work around the errors which are logged by the shape validation in the Redux implementation, triggered by the main work here in using combineReducers with blocks state's withChangeDetection. Performance is a "plus".

To your question, is the point that we should bring it in rather than rely on it as a third-party dependency? I don't really feel strongly one way or the other. I implemented it as a separate project largely to disable the availability of transpilation, for performances' sake (it's a weekend side hack project, something I felt fine to "waste" time over-optimizing).

aduth · 2018-10-23T19:18:56Z

I've rebased and pushed a number of revisions, including updated unit tests.

There's failing end-to-end tests which are legitimate, and which highlight an issue I've dealt with previously: We can't naively consider emptiness of state.editor.edits object for considering dirtiness currently, because we have this concept of "initial edits" to e.g. set the 'Auto-Draft' default title to an empty string. This isn't initiated by the user, but is a field we need to send in the save, but should not trigger a dirty prompt for the user in a new post (as it does in the branch currently).

I'll need to think on this some more.

Related: #9403

aduth · 2018-10-30T18:32:30Z

Extracted substitution of combineReducers implementation separately to #11255

Edit: #11255 has been merged and this pull request rebased to drop the relevant commit.

aduth · 2018-10-30T21:05:49Z

This has been rebased and is now passing. The changes are still substantial, and I've extracted at least one more commit out (the initial edits refactoring, to #11267). It was originally developed separately, but cherry-picked into this branch to verify its viability for addressing the remaining E2E test failures.

aduth · 2018-10-31T15:35:11Z

Extracted blocksByClientId, blockOrder -> blocks.byClientId, blocks.order state shape refactoring to #11315.

Edit: #11315 has been merged and this pull request rebased to drop the relevant commit.

aduth · 2018-10-31T20:25:56Z

#11315 has been separately merged and this one rebased in response, reducing from +1,556 −1,044 changes to +550 −242.

aduth · 2018-11-06T20:43:54Z

#11267 has been separately merged and this one rebased in response, reducing from +660 -243 changes to +261 -111.

I'm not planning on extracting any other separate pull requests. This one should be considered ready for final review as-is.

youknowriad · 2018-11-07T11:11:46Z

packages/editor/src/store/selectors.js

+		return true;
+	}
+
+	return inSomeHistory( state, isEditedPostDirty );


So if I understand properly. We conder a post dirty until it's completely saved (successful response)? right? any particular reason? should we clarify why in a comment? (I know it's not new in this PR though)

So if I understand properly. We conder a post dirty until it's completely saved (successful response)? right? any particular reason? should we clarify why in a comment? (I know it's not new in this PR though)

It's because change detection (now only for blocks) is reset at the start of the save, but we still want to avoid allowing a user to reload while that request is still pending.

It's covered in this test:

gutenberg/test/e2e/specs/change-detection.test.js

Lines 216 to 230 in 26ccc15

it( 'Should prompt if changes and save is in-flight', async () => {

await page.type( '.editor-post-title__input', 'Hello World' );

// Hold the posts request so we don't deal with race conditions of the

// save completing early. Other requests should be allowed to continue,

// for example the page reload test.

await interceptSave();

// Keyboard shortcut Ctrl+S save.

await pressWithModifier( META_KEY, 'S' );

await releaseSaveIntercept();

await assertIsDirty( true );

} );

I'll see if I can form a useful comment here.

I'll see if I can form a useful comment here.

Added in rebased fd7a8ec.

youknowriad · 2018-11-07T11:14:06Z

packages/editor/src/store/selectors.js

+	// flag in place of content field comparison against the known autosave.
+	// This is not strictly accurate, and relies on a tolerance toward autosave
+	// request failures for unnecessary saves.
+	if ( hasChangedContent( state ) ) {


I guess this is the fix for the performance issue.

It might not be easy but I wonder if we can add an e2e test checking that we don't run save functions as we type :)

It might not be easy but I wonder if we can add an e2e test checking that we don't run save functions as we type :)

Hmm, might not be too hard! I'll give it a shot.

youknowriad

LGTM 👍

aduth · 2018-11-08T18:43:02Z

Consolidated list of composing pull requests:

aduth added [Status] In Progress Tracking issues with work in progress [Type] Performance Related to performance efforts [Package] Data /packages/data [Feature] Saving Related to saving functionality labels Oct 21, 2018

aduth mentioned this pull request Oct 21, 2018

Performance: save components are called for *every* block on *every* keystroke #10427

Closed

aduth requested a review from ellatrix October 21, 2018 01:42

ellatrix reviewed Oct 22, 2018

View reviewed changes

WordPress deleted a comment from ellatrix Oct 22, 2018

aduth mentioned this pull request Oct 23, 2018

Recursively step through edits to track individually changed post meta #10827

Merged

4 tasks

aduth force-pushed the update/dirty-content branch from 8067549 to 3f2afc1 Compare October 23, 2018 19:15

aduth mentioned this pull request Oct 29, 2018

Remove autosaveable check from autosave monitor #10431

Closed

4 tasks

noisysocks mentioned this pull request Oct 30, 2018

Editing post in Text Mode causes shared blocks to go away #5754

Closed

aduth force-pushed the update/dirty-content branch from 3f2afc1 to b81f0e2 Compare October 30, 2018 16:59

aduth mentioned this pull request Oct 30, 2018

Data: Use turbo-combine-reducers in place of Redux #11255

Merged

aduth force-pushed the update/dirty-content branch from b81f0e2 to 5755c05 Compare October 30, 2018 20:15

aduth mentioned this pull request Oct 30, 2018

Editor: Factor initial edits to own state #11267

Merged

2 tasks

aduth force-pushed the update/dirty-content branch from 5755c05 to d628647 Compare October 30, 2018 20:28

aduth force-pushed the update/dirty-content branch from d628647 to 05c40dd Compare October 31, 2018 15:18

aduth mentioned this pull request Oct 31, 2018

Editor: Reshape blocks state under own key #11315

Merged

aduth removed the [Status] In Progress Tracking issues with work in progress label Oct 31, 2018

aduth force-pushed the update/dirty-content branch from 05c40dd to 679c79d Compare October 31, 2018 20:25

aduth added this to the 4.3 milestone Nov 2, 2018

aduth force-pushed the update/dirty-content branch 2 times, most recently from 9fc77bb to 804a206 Compare November 6, 2018 20:42

youknowriad reviewed Nov 7, 2018

View reviewed changes

youknowriad approved these changes Nov 7, 2018

View reviewed changes

designsimply mentioned this pull request Nov 8, 2018

Extensive articles with over 1700 words and various blocks make Gutenberg very slow. #10418

Closed

aduth added 3 commits November 8, 2018 11:54

Editor: Move dirty flag to blocks substate

fd7a8ec

Editor: Optimize autosaveable condition

4e0ba38

Editor: Remove redundant artificial dirtying

aa36d29

aduth force-pushed the update/dirty-content branch from 804a206 to aa36d29 Compare November 8, 2018 16:54

aduth merged commit 005c790 into master Nov 8, 2018

aduth deleted the update/dirty-content branch November 8, 2018 18:26

designsimply mentioned this pull request Nov 8, 2018

Confirmation popup displays when closing browser tab when page hasn't changed #4687

Closed

jsnajdr mentioned this pull request Oct 19, 2022

hasChangedContent: remove obsolete blocks check #45090

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Editor: Reshape editor state for optimized and accurate dirtiness detection #10844

Editor: Reshape editor state for optimized and accurate dirtiness detection #10844

aduth commented Oct 21, 2018 •

edited

Loading

ellatrix Oct 22, 2018

aduth Oct 22, 2018 •

edited

Loading

ellatrix Oct 22, 2018

aduth Oct 22, 2018

youknowriad Nov 7, 2018

ellatrix commented Oct 22, 2018

aduth commented Oct 22, 2018 •

edited

Loading

aduth commented Oct 23, 2018

aduth commented Oct 30, 2018 •

edited

Loading

aduth commented Oct 30, 2018

aduth commented Oct 31, 2018 •

edited

Loading

aduth commented Oct 31, 2018

aduth commented Nov 6, 2018

youknowriad Nov 7, 2018 •

edited

Loading

aduth Nov 7, 2018

aduth Nov 8, 2018

youknowriad Nov 7, 2018

aduth Nov 7, 2018

youknowriad left a comment

aduth commented Nov 8, 2018

	it( 'Should prompt if changes and save is in-flight', async () => {
	await page.type( '.editor-post-title__input', 'Hello World' );

	// Hold the posts request so we don't deal with race conditions of the
	// save completing early. Other requests should be allowed to continue,
	// for example the page reload test.
	await interceptSave();

	// Keyboard shortcut Ctrl+S save.
	await pressWithModifier( META_KEY, 'S' );

	await releaseSaveIntercept();

	await assertIsDirty( true );
	} );

Editor: Reshape editor state for optimized and accurate dirtiness detection #10844

Editor: Reshape editor state for optimized and accurate dirtiness detection #10844

Conversation

aduth commented Oct 21, 2018 • edited Loading

Choose a reason for hiding this comment

aduth Oct 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ellatrix commented Oct 22, 2018

aduth commented Oct 22, 2018 • edited Loading

aduth commented Oct 23, 2018

aduth commented Oct 30, 2018 • edited Loading

aduth commented Oct 30, 2018

aduth commented Oct 31, 2018 • edited Loading

aduth commented Oct 31, 2018

aduth commented Nov 6, 2018

youknowriad Nov 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad left a comment

Choose a reason for hiding this comment

aduth commented Nov 8, 2018

aduth commented Oct 21, 2018 •

edited

Loading

aduth Oct 22, 2018 •

edited

Loading

aduth commented Oct 22, 2018 •

edited

Loading

aduth commented Oct 30, 2018 •

edited

Loading

aduth commented Oct 31, 2018 •

edited

Loading

youknowriad Nov 7, 2018 •

edited

Loading