Block Editor: Compare last action in reducer enhancer only if non-ign…

…ored
WordPress · Feb 27, 2019 · b24696f · b24696f
1 parent c8a9de2
commit b24696f
Show file tree

Hide file tree

Showing 2 changed files with 96 additions and 12 deletions.
diff --git a/packages/block-editor/src/store/reducer.js b/packages/block-editor/src/store/reducer.js
@@ -12,6 +12,7 @@ import {
 	keys,
 	isEqual,
 	isEmpty,
+	get,
 } from 'lodash';
 
 /**
@@ -202,20 +203,36 @@ function withPersistentBlockChange( reducer ) {
 
 		const isExplicitPersistentChange = action.type === 'MARK_LAST_CHANGE_AS_PERSISTENT';
 
-		if ( state !== nextState || isExplicitPersistentChange ) {
-			// Some state changes should not be considered persistent, namely
-			// those which are not a direct result of user interaction.
-			const isPersistentStateChange = ! IGNORED_ACTION_TYPES.has( action.type );
+		// Defer to previous state value (or default) unless changing or
+		// explicitly marking as persistent.
+		if ( state === nextState && ! isExplicitPersistentChange ) {
+			return {
+				...nextState,
+				isPersistentChange: get( state, [ 'isPersistentChange' ], true ),
+			};
+		}
 
-			nextState = {
+		// Some state changes should not be considered persistent, namely those
+		// which are not a direct result of user interaction.
+		const isIgnoredActionType = IGNORED_ACTION_TYPES.has( action.type );
+		if ( isIgnoredActionType ) {
+			return {
 				...nextState,
-				isPersistentChange: isExplicitPersistentChange || (
-					isPersistentStateChange &&
-					! isUpdatingSameBlockAttribute( action, lastAction )
-				),
+				isPersistentChange: false,
 			};
 		}
 
+		nextState = {
+			...nextState,
+			isPersistentChange: (
+				isExplicitPersistentChange ||
+				! isUpdatingSameBlockAttribute( action, lastAction )
+			),
+		};
+
+		// In comparing against the previous action, consider only those which
+		// would have qualified as one which would have been ignored or not
+		// have resulted in a changed state.
 		lastAction = action;
 
 		return nextState;

diff --git a/packages/block-editor/src/store/test/reducer.js b/packages/block-editor/src/store/test/reducer.js
@@ -1046,6 +1046,12 @@ describe( 'state', () => {
 			} );
 
 			describe( 'isPersistentChange', () => {
+				it( 'should default a changing state to true', () => {
+					const state = deepFreeze( blocks( undefined, {} ) );
+
+					expect( state.isPersistentChange ).toBe( true );
+				} );
+
 				it( 'should consider any non-exempt block change as persistent', () => {
 					const original = deepFreeze( blocks( undefined, {
 						type: 'RESET_BLOCKS',
@@ -1063,24 +1069,54 @@ describe( 'state', () => {
 					expect( state.isPersistentChange ).toBe( true );
 				} );
 
+				it( 'should consider any non-exempt block change as persistent across unchanging actions', () => {
+					let original = deepFreeze( blocks( undefined, {
+						type: 'RESET_BLOCKS',
+						blocks: [ {
+							clientId: 'kumquat',
+							attributes: {},
+							innerBlocks: [],
+						} ],
+					} ) );
+					original = blocks( original, {
+						type: 'NOOP',
+					} );
+					original = blocks( original, {
+						// While RECEIVE_BLOCKS changes state, it's considered
+						// as ignored, confirmed by this test.
+						type: 'RECEIVE_BLOCKS',
+						blocks: [],
+					} );
+
+					const state = blocks( original, {
+						type: 'UPDATE_BLOCK_ATTRIBUTES',
+						clientId: 'kumquat',
+						attributes: {
+							updated: false,
+						},
+					} );
+
+					expect( state.isPersistentChange ).toBe( true );
+				} );
+
 				it( 'should consider same block attribute update as exempt', () => {
-					const original = deepFreeze( blocks( undefined, {
+					let original = deepFreeze( blocks( undefined, {
 						type: 'RESET_BLOCKS',
 						blocks: [ {
 							clientId: 'kumquat',
 							attributes: {},
 							innerBlocks: [],
 						} ],
 					} ) );
-					let state = blocks( original, {
+					original = blocks( original, {
 						type: 'UPDATE_BLOCK_ATTRIBUTES',
 						clientId: 'kumquat',
 						attributes: {
 							updated: false,
 						},
 					} );
 
-					state = blocks( state, {
+					const state = blocks( original, {
 						type: 'UPDATE_BLOCK_ATTRIBUTES',
 						clientId: 'kumquat',
 						attributes: {
@@ -1091,6 +1127,37 @@ describe( 'state', () => {
 					expect( state.isPersistentChange ).toBe( false );
 				} );
 
+				it( 'should flag an explicitly marked persistent change', () => {
+					let original = deepFreeze( blocks( undefined, {
+						type: 'RESET_BLOCKS',
+						blocks: [ {
+							clientId: 'kumquat',
+							attributes: {},
+							innerBlocks: [],
+						} ],
+					} ) );
+					original = blocks( original, {
+						type: 'UPDATE_BLOCK_ATTRIBUTES',
+						clientId: 'kumquat',
+						attributes: {
+							updated: false,
+						},
+					} );
+					original = blocks( original, {
+						type: 'UPDATE_BLOCK_ATTRIBUTES',
+						clientId: 'kumquat',
+						attributes: {
+							updated: true,
+						},
+					} );
+
+					const state = blocks( original, {
+						type: 'MARK_LAST_CHANGE_AS_PERSISTENT',
+					} );
+
+					expect( state.isPersistentChange ).toBe( true );
+				} );
+
 				it( 'should not consider received blocks as persistent change', () => {
 					const state = blocks( undefined, {
 						type: 'RECEIVE_BLOCKS',