State: Refactor post dirty state as higher-order reducer

[aduth]

Fixes #3296
Related: #1996

This pull request seeks to refactor the state behavior for determining whether a post has unsaved changes. In some ways it reverts to characteristics prior to #1996 in that the proposed changes here no longer perform a diff between the current and saved variations of a post. This is proposed for two reasons:

• As of State: Reset editor undo history only at initial setup #3178, we can no longer assume to have history reset at each save, so with the implementation of the isEditedPostDirty selector we'd otherwise need a reference to blocks at the time of the last save (either tracking history at specific action intervals, or a separate reducer)
• The isEditedPostDirty selector, while memoized, contains non-trivial logic (multiple deep object comparison), and is cachebust on a regular basis (each edit), thus potentially harming performance.

As implemented here, the basic idea is that we check references on state.editor before and after any action is dispatched to the store, _using a withChangeDetection Redux higher-order reducer. Since this reducer value encompasses any edits which occur, a strict reference equality check should be sufficient for determining if an edit has been made. This is made more challenging by:

• Known editor resets (at save, at initial load)
• Cannot reflect Saved state when manually undo-ing edits (like demonstrated in Perform dirty detection as diff against saved post #1996)

The latter of these, while "nice to have", is not as critical a feature as I might have originally assumed.

Implementation notes:

The state structuring here partially reflects the proposed Ducks pattern of #3012, originally planned to be iterated toward as part of #3030, but prioritized into these changes as a critical bug fix.

Testing instructions:

Repeat testing instructions from #3296, verifying Expected Result.

Verify that Save Draft button only appears at expected intervals:

• Not on initial page load
• When changing block order
• When changing block attributes / content
• When inserting blocks
• When changing post attributes (title, date, etc.)
• When undoing or redoing

[codecov]

Codecov Report

Merging #3298 into master will decrease coverage by 0.2%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #3298      +/-   ##
==========================================
- Coverage   31.26%   31.06%   -0.21%     
==========================================
  Files         244      245       +1     
  Lines        6736     6716      -20     
  Branches     1204     1205       +1     
==========================================
- Hits         2106     2086      -20     
  Misses       3895     3895              
  Partials      735      735

Impacted Files	Coverage Δ
editor/utils/with-history/index.js	93.75% <ø> (ø)
editor/selectors.js	93.37% <100%> (-0.54%)	⬇️
editor/utils/with-change-detection/index.js	100% <100%> (ø)
editor/reducer.js	90.11% <100%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4e61b96...0304b60. Read the comment docs.

[youknowriad]

I'm wondering why this needs to be a middleware. Can't we do this simply with a "reducer enhancer":

enhancedEditorReducer( state, action ) {
	const newState = editorReducer( state, action );
	const isDirty = some( newState, ( value, key ) => key !== 'dirty' && value !== state[ key ] );
	if ( isDirty !== newState.dirty ) {
		newState.dirty = isDirty;
	}

	return newState;
}

[aduth]

I like this idea, and it sounds like a nice simplification. It would mean that the dirty flag would become part of editor state subtree, but I think that's reasonable.

[youknowriad]

Actually, I think I missed something here, When do we reset the isDirty to false. I think this enhancer needs to be in the higher level to take into account resetting the dirty value when the currentPost changes

[aduth]

Right, unless we could rely on specific action types as signals for reset (RESET_POST, UPDATE_POST), then the reducer enhancer for editor could simply listen for those.

Also in your original example, ideally we don't need the some and could do a strict equality check newState !== state.

[aduth]

I've updated the branch with suggested approach of implementing dirty detection as a higher-order reducer. In fact, this works quite well, but unfortunately it surfaced an unrelated issue with some magic that we had introduced in the undoable reducer enhancer with property getters.

The specific behavior was intended as a convenient to allow state.foo in an undoable reducer return value to map automatically to state.history.present.foo. However, this is not very compatible in composing additional reducer enhancers, since as in 93ff853 we have a need to create a shallow clone of the composed state return value. By default, the non-enumerable property getters are not included in the clone (neither as value, or the getters themselves), thus breaking the property proxying. There are some workarounds here, but ultimately I decided that the proxying behavior was harmful, and removed in 68e22aa (with additional details in the extended comment description).

Investigating e2e failure which seems to be a legitimate issue...

[aduth]

Okay, I tracked the issue down to missing a couple dependant's key updates which were more difficult to find due to use of Lodash's _.get to access nested property keys.

Fixed in e9d8a69 .

Consider that a success for e2e tests 😄 To be fair, this probably would have been surfaced if the getPostMeta selector introduced in #2740 included unit tests (cc @mcsf).

[mcsf]

Nit-pick: an undoableReducer would be the enhanced reducer, not the higher-order function producing it. Is it a reducerer? ;) I wonder if a enhance*, with* or makeUndoable-type nomenclature would be useful.

Everything else obviously looks good.

[mcsf]

Description for undoable.

[aduth]

Copypasta 😄 Fixed in 4459741

[mcsf]

Param description for undoable.

[aduth]

Nit-pick: an undoableReducer would be the enhanced reducer, not the higher-order function producing it. Is it a reducerer?

Yeah, I contemplated this, particularly in renaming what was previously undoable to undoableReducer. As with our convention on higher-order components (with- prefix), I think a pattern here could be useful, yes. Even the same prefix could work; withHistory, withChangeDetection, etc. ? asUndoableReducer, asDirtyingReducer ? Simply undoable, dirtying ?

[aduth]

Did another rebase, squashing a few commits.

In 0304b60, I renamed the higher-order reducers to a consistent with prefix (withChangeDetection, withHistory). I think as long as these are documented accordingly, it's fine enough to use as a general pattern for higher-order functions.

This is resolving a pretty noticeable bug, and there were a number of changes here particularly touching the treatment of history-enhanced reducers, so I'm inclined to merge this sooner than later to shake out any issues.

[youknowriad]

I like this, tested and seems to work better now.