Editor: Factor initial edits to own state

[aduth]

Extracted from / complements: #10844
Partially addresses: #7409

This pull request seeks to refactor the treatment of "initial edits" out of the state.editor.present.edits to allow for the existing edits state to represent only user-initiated edits to be considered for change detection. This will enable some simplification of change detection reset conditions, and works toward #10844's goal of considering "dirtiness" as a condition of a non-empty edits state.

Implementation notes:

While I think this partly moves away from a goal of eliminating the editor setup actions, it concedes in acknowledging that there should be at most a single parse which occurs at the start of an editing session, with consideration of both edits and content. Thus, it is implemented here as part of the effect handler for SETUP_EDITOR. I still have some faith this could be refactored in the future, but have chosen the most direct option to supporting #10844 here.

Testing instructions:

Verify there are no regressions in the behavior of "initial edits", notably:

• Saving a new draft with an empty title does not show "Auto Draft" upon reload
• The demo content works as expected

Ensure tests pass:

npm test

Follow-up tasks:

• With improvements here to initial edits, assignment of draft status during first auto-draft save could be easily moved to the server-side setup of a new draft.
• SETUP_EDITOR handling of blocks should be eliminated in favor of a consistent, single RESET_BLOCKS action.

[aduth]

I still need to add some test improvements here.

Edit: Added in rebased e59e7d0.

[aduth]

Related: #10827

I'd incorporated getMutateSafeObject from there to the implementation here. I think also the diff and merge functions there will help further eliminate the redundancy between edits and initialEdits reducers.

[youknowriad]

Can we consider https://github.com/mweststrate/immer instead of this?

[aduth]

Can we consider https://github.com/mweststrate/immer instead of this?

I'm not too opposed to the idea of it as a general pattern, but for one-off use it seems maybe a bit much both in terms of bundle size impact and developer knowledge barrier. Should we discuss separate to this pull request? I do think Immer seems to tackle well a pattern which is very prevalent in our stores.

[youknowriad]

getMutateSafeObject can also be considered "something to learn" but yes definitely a topic for a separate discussion.

[aduth]

There are degrees of learning 😆

[youknowriad]

This is a bit confusing? Why do we need to consider two "setup" actions here? can't we just merge both in one? It probably means only use SETUP_EDITOR_STATE here.

[aduth]

This is a bit confusing? Why do we need to consider two "setup" actions here? can't we just merge both in one? It probably means only use SETUP_EDITOR_STATE here.

Right, this confused me as well, and I left it as a note in the original comment to circle back to this:

Follow-up tasks:

[...]

SETUP_EDITOR handling of blocks should be eliminated in favor of a consistent, single RESET_BLOCKS action.

This is a bit of consistency for consistency's sake. You can see similar use in untouched existing reducers of editor.

[youknowriad]

Not sure I understand why we need to address this here? my thinking is that initialEdits should be "overriden" by regular edits anyway when saving, why do we need to change the "initial edits" when we "edit" a post?

[aduth]

The issue we need to avoid is having an initial edit resurface when an edits key is unset by UPDATE_POST.

An alternative here (perhaps better even) is to unset keys from initialEdits when an (assumed-user-initiated) EDIT_POST occurs (i.e. the initial edit can be discarded at the point where a user defines their own value).

[youknowriad]

How does a user unset an edit? editPost( { something: undefined } )? Can this be considered as revert to initial value, which means use initialEdit if defined?

[aduth]

It becomes unset when the edit matches the saved value of the post. So if there's an initial edit lingering, it will become (wrongly) assumed as an edit.

[youknowriad]

It becomes unset when the edit matches the saved value of the post

Should this be: "become unset if it matches the initial edit if defined, the saved value otherwise?

Isn't it a bug now?

[aduth]

It becomes more an issue when considering the demo post, which should be immediately saveable even if technically editor.edits would be empty (since it's all initial edits).

(We should have an E2E test for this)

[youknowriad]

One thing I noticed which is weird though in our current behavior is that I'd have expected the "save" button to be disabled when you open the demo page without any modification. And if the current behavior is the right one, that I'm completely confused :P

[youknowriad]

(I wrote the last comment without reading your response :P )

[aduth]

I think I may have mispoke here:

Edit title back to empty:

post = { title: '' };
initialEdits = { title: 'demo' };
edits = {}; // title omitted because matches post value

Data is never unset from edits when the user makes a change in the interface:

gutenberg/packages/editor/src/store/reducer.js

Lines 228 to 243 in 3511dd8

	case 'EDIT_POST':
	case 'SETUP_EDITOR_STATE':
	return reduce( action.edits, ( result, value, key ) => {
	// Only assign into result if not already same value
	if ( value !== state[ key ] ) {
	// Avoid mutating original state by creating shallow
	// clone. Should only occur once per reduce.
	if ( result === state ) {
	result = { ...state };
	}
	result[ key ] = value;
	}
	return result;
	}, state );

This only happens when they save the post, or the post is otherwise reset:

gutenberg/packages/editor/src/store/reducer.js

Lines 255 to 272 in 3511dd8

	case 'UPDATE_POST':
	case 'RESET_POST':
	const getCanonicalValue = action.type === 'UPDATE_POST' ?
	( key ) => action.edits[ key ] :
	( key ) => getPostRawValue( action.post[ key ] );
	return reduce( state, ( result, value, key ) => {
	if ( value !== getCanonicalValue( key ) ) {
	return result;
	}
	if ( state === result ) {
	result = { ...state };
	}
	delete result[ key ];
	return result;
	}, state );

Since these are the same triggers for resetting initialEdits, there shouldn't be an issue here.

[youknowriad]

👍

[aduth]

I'm working to fix up the failing tests. Must have gone wrong in the rebase.

[youknowriad]

I still don't like that the initialEdits should consider any user edits. I think we should just keep them as initial edits until the first save.

But that said, that's not a big concern, I'm approving the PR. Let's move forward.

[aduth]

I still don't like that the initialEdits should consider any user edits. I think we should just keep them as initial edits until the first save.

I'm not too opposed to dropping this. I don't think it has much of an impact in either case, given that user edits will always take precedence and never be unset until the save.

I'm going to do a final pass over the changes here to see if it makes sense / if we need more test coverage / etc.

[aduth]

I still don't like that the initialEdits should consider any user edits. I think we should just keep them as initial edits until the first save.

In looking over the proposed changes, initialEdits doesn't respond to edits. There should be effectively no changes to the state value between the initial setup and the first save. There might be some confusion in the action.edits value of the UPDATE_POST action, but this in-fact the save action.

[aduth]

I've rebased and added a number of new tests, some only incidentally related:

• 3cb31b6
• ddf6f11
• 50b5c18
• e024015

Our end-to-end utility treatment of querystrings is a bit contrary to expectations, but I opted to not mess with it too much here.

[aduth]

Aside: I wanted to refactor this test to use the query arguments for prepopulating a title (post-new.php?post_title=foo):

gutenberg/test/e2e/specs/new-post.test.js

Lines 55 to 75 in 2dddcc9

	it( 'should not focus the title if the title exists', async () => {
	// Enter a title for this post.
	await page.type( '.editor-post-title__input', 'Here is the title' );
	// Save the post as a draft.
	await page.click( '.editor-post-save-draft' );
	await page.waitForSelector( '.editor-post-saved-state.is-saved' );
	// Reload the browser so a post is loaded with a title.
	await page.reload();
	const activeElementClasses = await page.evaluate( () => {
	return Object.values( document.activeElement.classList );
	} );
	const activeElementTagName = await page.evaluate( () => {
	return document.activeElement.tagName.toLowerCase();
	} );
	expect( activeElementClasses ).not.toContain( 'editor-post-title__input' );
	// The document `body` should be the `activeElement`, because nothing is
	// focused by default when a post already has a title.
	expect( activeElementTagName ).toEqual( 'body' );
	} );

But in fact it fails, since the test makes the wrong assumption that only non-new posts can have a title. It still focuses the title for a new post, even if the title is non-empty.

cc @tofumatt re: #9608