Block Editor: Consider RECEIVE_BLOCKS as non-persistent change (#14108)

* Block Editor: Consider RECEIVE_BLOCKS as non-persistent change * Block Editor: Compare last action in reducer enhancer only if non-ignored
WordPress · Feb 27, 2019 · 845185b · 845185b
1 parent 05570ff
commit 845185b
Show file tree

Hide file tree

Showing 3 changed files with 121 additions and 10 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';
 
 /**
@@ -187,20 +188,51 @@ export function isUpdatingSameBlockAttribute( action, lastAction ) {
 function withPersistentBlockChange( reducer ) {
 	let lastAction;
 
+	/**
+	 * Set of action types for which a blocks state change should be considered
+	 * non-persistent.
+	 *
+	 * @type {Set}
+	 */
+	const IGNORED_ACTION_TYPES = new Set( [
+		'RECEIVE_BLOCKS',
+	] );
+
 	return ( state, action ) => {
 		let nextState = reducer( state, action );
+
 		const isExplicitPersistentChange = action.type === 'MARK_LAST_CHANGE_AS_PERSISTENT';
 
-		if ( state !== nextState || isExplicitPersistentChange ) {
-			nextState = {
+		// 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 ),
+			};
+		}
+
+		// 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 ||
-					! 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,31 +1069,105 @@ 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: {
+							updated: true,
+						},
+					} );
+
+					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',
+						blocks: [ {
+							clientId: 'kumquat',
+							attributes: {},
+							innerBlocks: [],
+						} ],
+					} );
+
 					expect( state.isPersistentChange ).toBe( false );
 				} );
 			} );

diff --git a/packages/editor/src/store/reducer.js b/packages/editor/src/store/reducer.js
@@ -134,7 +134,6 @@ export const editor = flow( [
 	withHistory( {
 		resetTypes: [ 'SETUP_EDITOR_STATE' ],
 		ignoreTypes: [
-			'RECEIVE_BLOCKS',
 			'RESET_POST',
 			'UPDATE_POST',
 		],