From 9a8ad06a20cfc3d02f44b231d3f077a099b8a49d Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Thu, 1 Dec 2022 10:30:41 +0000 Subject: [PATCH 01/11] Add exception for page list to ignore its inner blocks --- .../navigation/edit/unsaved-inner-blocks.js | 51 ++++++++++++++++--- 1 file changed, 44 insertions(+), 7 deletions(-) diff --git a/packages/block-library/src/navigation/edit/unsaved-inner-blocks.js b/packages/block-library/src/navigation/edit/unsaved-inner-blocks.js index 0fa2a119446f36..fee0f4fa34105f 100644 --- a/packages/block-library/src/navigation/edit/unsaved-inner-blocks.js +++ b/packages/block-library/src/navigation/edit/unsaved-inner-blocks.js @@ -1,3 +1,8 @@ +/** + * External dependencies + */ +import { isEqual } from 'lodash'; + /** * WordPress dependencies */ @@ -51,14 +56,46 @@ export default function UnsavedInnerBlocks( { } }, [ blocks ] ); - // If the current inner blocks object is different in any way - // from the original inner blocks from the post content then the - // user has made changes to the inner blocks. At this point the inner - // blocks can be considered "dirty". - // We also make sure the current innerBlocks had a chance to be set. - const innerBlocksAreDirty = - !! originalBlocks.current && blocks !== originalBlocks.current; + let innerBlocksAreDirty = false; + + if ( + originalBlocks.current && + blocks?.length && + blocks[ 0 ]?.name === 'core/page-list' + ) { + // ignore changes to inner blocks + // track changes to attributes changing + const originalPageListBlock = originalBlocks.current[ 0 ]; + const currentPageListBlock = blocks[ 0 ]; + + const { + innerBlocks: discardedOriginalInnerBlocks, + ...originalPageListBlockWithoutInnerBlocks + } = originalPageListBlock; + + const { + innerBlocks: discardedCurrentInnerBlocks, + ...currentPageListBlockWithoutInnerBlocks + } = currentPageListBlock; + + innerBlocksAreDirty = ! isEqual( + originalPageListBlockWithoutInnerBlocks, + currentPageListBlockWithoutInnerBlocks + ); + } else { + // If the current inner blocks object is different in any way + // from the original inner blocks from the post content then the + // user has made changes to the inner blocks. At this point the inner + // blocks can be considered "dirty". + // We also make sure the current innerBlocks had a chance to be set. + innerBlocksAreDirty = + !! originalBlocks.current && blocks !== originalBlocks.current; + } + + // if ( innerBlocksAreDirty ) { + // debugger; + // } const shouldDirectInsert = useMemo( () => blocks.every( From 34bf73ede26d6f8a3fd530fcc1a750c45e74aec0 Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Thu, 1 Dec 2022 10:36:43 +0000 Subject: [PATCH 02/11] Explain what is going on --- .../src/navigation/edit/unsaved-inner-blocks.js | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/packages/block-library/src/navigation/edit/unsaved-inner-blocks.js b/packages/block-library/src/navigation/edit/unsaved-inner-blocks.js index fee0f4fa34105f..1c8d76e594dea4 100644 --- a/packages/block-library/src/navigation/edit/unsaved-inner-blocks.js +++ b/packages/block-library/src/navigation/edit/unsaved-inner-blocks.js @@ -63,9 +63,15 @@ export default function UnsavedInnerBlocks( { blocks?.length && blocks[ 0 ]?.name === 'core/page-list' ) { - // ignore changes to inner blocks - // track changes to attributes changing - + // If the blocks are a page list, we need to ignore + // inner blocks and compare remaining attributes only. + // Why? Because Page List block dynamically sets its + // child inner blocks async following a REST API request. + // This means that the inner blocks are empty when the + // block is first inserted, and then populated later once + // the REST API request resolves. This causes the original + // check to always return dirty as the inner blocks change. + // This is a workaround for this specific scenario. const originalPageListBlock = originalBlocks.current[ 0 ]; const currentPageListBlock = blocks[ 0 ]; From 1f6566195d90ad9667575d241e80a44880649c8e Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Thu, 1 Dec 2022 10:44:01 +0000 Subject: [PATCH 03/11] Only consider the first block --- .../block-library/src/navigation/edit/unsaved-inner-blocks.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/block-library/src/navigation/edit/unsaved-inner-blocks.js b/packages/block-library/src/navigation/edit/unsaved-inner-blocks.js index 1c8d76e594dea4..946f0d31275aa7 100644 --- a/packages/block-library/src/navigation/edit/unsaved-inner-blocks.js +++ b/packages/block-library/src/navigation/edit/unsaved-inner-blocks.js @@ -60,7 +60,7 @@ export default function UnsavedInnerBlocks( { if ( originalBlocks.current && - blocks?.length && + blocks?.length === 1 && blocks[ 0 ]?.name === 'core/page-list' ) { // If the blocks are a page list, we need to ignore From 1a6d06d8cfd61029cb97d8506d1a73a6e5028b0e Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Mon, 5 Dec 2022 10:35:40 +0000 Subject: [PATCH 04/11] Remove debugging --- .../block-library/src/navigation/edit/unsaved-inner-blocks.js | 3 --- 1 file changed, 3 deletions(-) diff --git a/packages/block-library/src/navigation/edit/unsaved-inner-blocks.js b/packages/block-library/src/navigation/edit/unsaved-inner-blocks.js index 946f0d31275aa7..da37014324cbc0 100644 --- a/packages/block-library/src/navigation/edit/unsaved-inner-blocks.js +++ b/packages/block-library/src/navigation/edit/unsaved-inner-blocks.js @@ -99,9 +99,6 @@ export default function UnsavedInnerBlocks( { !! originalBlocks.current && blocks !== originalBlocks.current; } - // if ( innerBlocksAreDirty ) { - // debugger; - // } const shouldDirectInsert = useMemo( () => blocks.every( From 44149e196e4a87a60085f6a7b48ba960d7cd3629 Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Mon, 5 Dec 2022 10:37:27 +0000 Subject: [PATCH 05/11] Be specific in the referential equality check comments --- .../block-library/src/navigation/edit/unsaved-inner-blocks.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/block-library/src/navigation/edit/unsaved-inner-blocks.js b/packages/block-library/src/navigation/edit/unsaved-inner-blocks.js index da37014324cbc0..78a15189c57554 100644 --- a/packages/block-library/src/navigation/edit/unsaved-inner-blocks.js +++ b/packages/block-library/src/navigation/edit/unsaved-inner-blocks.js @@ -90,8 +90,8 @@ export default function UnsavedInnerBlocks( { currentPageListBlockWithoutInnerBlocks ); } else { - // If the current inner blocks object is different in any way - // from the original inner blocks from the post content then the + // If the current inner blocks object does not display referential equality + // with the original inner blocks from the post content then the // user has made changes to the inner blocks. At this point the inner // blocks can be considered "dirty". // We also make sure the current innerBlocks had a chance to be set. From f4bfc534c573f88cc47d6a71f476431905dd13f4 Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Tue, 6 Dec 2022 09:40:10 +0000 Subject: [PATCH 06/11] Ditch Lodash for Fast Deep Equal --- .../block-library/src/navigation/edit/unsaved-inner-blocks.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/block-library/src/navigation/edit/unsaved-inner-blocks.js b/packages/block-library/src/navigation/edit/unsaved-inner-blocks.js index 78a15189c57554..e385e2b77f0171 100644 --- a/packages/block-library/src/navigation/edit/unsaved-inner-blocks.js +++ b/packages/block-library/src/navigation/edit/unsaved-inner-blocks.js @@ -1,7 +1,7 @@ /** * External dependencies */ -import { isEqual } from 'lodash'; +import fastDeepEqual from 'fast-deep-equal/es6'; /** * WordPress dependencies @@ -85,7 +85,7 @@ export default function UnsavedInnerBlocks( { ...currentPageListBlockWithoutInnerBlocks } = currentPageListBlock; - innerBlocksAreDirty = ! isEqual( + innerBlocksAreDirty = ! fastDeepEqual( originalPageListBlockWithoutInnerBlocks, currentPageListBlockWithoutInnerBlocks ); From 4c3f1f57e95452418096d7a881c4e1e5c518526f Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Tue, 6 Dec 2022 10:54:04 +0000 Subject: [PATCH 07/11] Use customised conditional deep equality --- .../navigation/edit/unsaved-inner-blocks.js | 97 ++++++++++--------- 1 file changed, 52 insertions(+), 45 deletions(-) diff --git a/packages/block-library/src/navigation/edit/unsaved-inner-blocks.js b/packages/block-library/src/navigation/edit/unsaved-inner-blocks.js index e385e2b77f0171..c0fc3ca4fcc3cf 100644 --- a/packages/block-library/src/navigation/edit/unsaved-inner-blocks.js +++ b/packages/block-library/src/navigation/edit/unsaved-inner-blocks.js @@ -1,8 +1,3 @@ -/** - * External dependencies - */ -import fastDeepEqual from 'fast-deep-equal/es6'; - /** * WordPress dependencies */ @@ -40,6 +35,46 @@ const ALLOWED_BLOCKS = [ 'core/navigation-submenu', ]; +/** + * Conditionally compares two candidates for deep equality. + * Provides an option to skip a given property of an object during comparison. + * + * @param {*} x 1st candidate for comparison + * @param {*} y 2nd candidate for comparison + * @param {Function|undefined} shouldSkip a function which can be used to skip a given property of an object. + * @return {boolean} whether the two candidates are deeply equal. + */ +const isDeepEqual = ( x, y, shouldSkip ) => { + if ( x === y ) { + return true; + } else if ( + typeof x === 'object' && + x !== null && + x !== undefined && + typeof y === 'object' && + y !== null && + y !== undefined + ) { + if ( Object.keys( x ).length !== Object.keys( y ).length ) return false; + + for ( const prop in x ) { + if ( y.hasOwnProperty( prop ) ) { + // Afford skipping a given property of an object. + if ( shouldSkip && shouldSkip( prop, x ) ) { + return true; + } + + if ( ! isDeepEqual( x[ prop ], y[ prop ], shouldSkip ) ) + return false; + } else return false; + } + + return true; + } + + return false; +}; + export default function UnsavedInnerBlocks( { blocks, createNavigationMenu, @@ -58,46 +93,18 @@ export default function UnsavedInnerBlocks( { let innerBlocksAreDirty = false; - if ( - originalBlocks.current && - blocks?.length === 1 && - blocks[ 0 ]?.name === 'core/page-list' - ) { - // If the blocks are a page list, we need to ignore - // inner blocks and compare remaining attributes only. - // Why? Because Page List block dynamically sets its - // child inner blocks async following a REST API request. - // This means that the inner blocks are empty when the - // block is first inserted, and then populated later once - // the REST API request resolves. This causes the original - // check to always return dirty as the inner blocks change. - // This is a workaround for this specific scenario. - const originalPageListBlock = originalBlocks.current[ 0 ]; - const currentPageListBlock = blocks[ 0 ]; - - const { - innerBlocks: discardedOriginalInnerBlocks, - ...originalPageListBlockWithoutInnerBlocks - } = originalPageListBlock; - - const { - innerBlocks: discardedCurrentInnerBlocks, - ...currentPageListBlockWithoutInnerBlocks - } = currentPageListBlock; - - innerBlocksAreDirty = ! fastDeepEqual( - originalPageListBlockWithoutInnerBlocks, - currentPageListBlockWithoutInnerBlocks - ); - } else { - // If the current inner blocks object does not display referential equality - // with the original inner blocks from the post content then the - // user has made changes to the inner blocks. At this point the inner - // blocks can be considered "dirty". - // We also make sure the current innerBlocks had a chance to be set. - innerBlocksAreDirty = - !! originalBlocks.current && blocks !== originalBlocks.current; - } + innerBlocksAreDirty = ! isDeepEqual( + originalBlocks.current, + blocks, + ( prop, x ) => { + // skip inner blocks of page list during comparison as they + // are always controlled and may be updated async due to + // syncing with enitiy records. + if ( x?.name && prop === 'innerBlocks' ) { + return true; + } + } + ); const shouldDirectInsert = useMemo( () => From bd9393fe0f72ff17bc6230aad1fb580cf25b0cf1 Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Tue, 6 Dec 2022 11:07:58 +0000 Subject: [PATCH 08/11] Remove unnecessary let --- .../block-library/src/navigation/edit/unsaved-inner-blocks.js | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/packages/block-library/src/navigation/edit/unsaved-inner-blocks.js b/packages/block-library/src/navigation/edit/unsaved-inner-blocks.js index c0fc3ca4fcc3cf..d63a4d9d45a196 100644 --- a/packages/block-library/src/navigation/edit/unsaved-inner-blocks.js +++ b/packages/block-library/src/navigation/edit/unsaved-inner-blocks.js @@ -91,9 +91,7 @@ export default function UnsavedInnerBlocks( { } }, [ blocks ] ); - let innerBlocksAreDirty = false; - - innerBlocksAreDirty = ! isDeepEqual( + const innerBlocksAreDirty = ! isDeepEqual( originalBlocks.current, blocks, ( prop, x ) => { From 7cd39257de521ee7f883324d891be3106e2ad1d9 Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Tue, 6 Dec 2022 11:24:36 +0000 Subject: [PATCH 09/11] Add comments for context --- .../src/navigation/edit/unsaved-inner-blocks.js | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/packages/block-library/src/navigation/edit/unsaved-inner-blocks.js b/packages/block-library/src/navigation/edit/unsaved-inner-blocks.js index d63a4d9d45a196..bbdfefc968fbe8 100644 --- a/packages/block-library/src/navigation/edit/unsaved-inner-blocks.js +++ b/packages/block-library/src/navigation/edit/unsaved-inner-blocks.js @@ -91,13 +91,21 @@ export default function UnsavedInnerBlocks( { } }, [ blocks ] ); + // If the current inner blocks is different from the original inner blocks + // from the post content then then the user has made changes to the inner blocks. + // At this point the inner blocks can be considered "dirty". + // Note: referential equality is not sufficient for comparison as the inner blocks + // of the page list are controlled and may be updated async due to syncing with + // entity records. As a result we need to perform a deep equality check skipping + // the page list's inner blocks. const innerBlocksAreDirty = ! isDeepEqual( originalBlocks.current, blocks, ( prop, x ) => { - // skip inner blocks of page list during comparison as they - // are always controlled and may be updated async due to - // syncing with enitiy records. + // Skip inner blocks of page list during comparison as they + // are **always** controlled and may be updated async due to + // syncing with enitiy records. Left unchecked this would + // inadvertently trigger the dirty state. if ( x?.name && prop === 'innerBlocks' ) { return true; } From 94743d2e45c8b4322dc6e9726c47e0f1def1bb4d Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Tue, 6 Dec 2022 12:04:14 +0000 Subject: [PATCH 10/11] Fix grammar Co-authored-by: Andrei Draganescu --- .../block-library/src/navigation/edit/unsaved-inner-blocks.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/block-library/src/navigation/edit/unsaved-inner-blocks.js b/packages/block-library/src/navigation/edit/unsaved-inner-blocks.js index bbdfefc968fbe8..3e715d4e92a6ac 100644 --- a/packages/block-library/src/navigation/edit/unsaved-inner-blocks.js +++ b/packages/block-library/src/navigation/edit/unsaved-inner-blocks.js @@ -91,8 +91,8 @@ export default function UnsavedInnerBlocks( { } }, [ blocks ] ); - // If the current inner blocks is different from the original inner blocks - // from the post content then then the user has made changes to the inner blocks. + // If the current inner blocks are different from the original inner blocks + // from the post content then the user has made changes to the inner blocks. // At this point the inner blocks can be considered "dirty". // Note: referential equality is not sufficient for comparison as the inner blocks // of the page list are controlled and may be updated async due to syncing with From ec45184fa387e4eeb7bfb1c2c4c1564c1e426845 Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Tue, 6 Dec 2022 12:28:50 +0000 Subject: [PATCH 11/11] Target fix to Page List only --- .../block-library/src/navigation/edit/unsaved-inner-blocks.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/block-library/src/navigation/edit/unsaved-inner-blocks.js b/packages/block-library/src/navigation/edit/unsaved-inner-blocks.js index 3e715d4e92a6ac..a57b221aa86ee7 100644 --- a/packages/block-library/src/navigation/edit/unsaved-inner-blocks.js +++ b/packages/block-library/src/navigation/edit/unsaved-inner-blocks.js @@ -106,7 +106,7 @@ export default function UnsavedInnerBlocks( { // are **always** controlled and may be updated async due to // syncing with enitiy records. Left unchecked this would // inadvertently trigger the dirty state. - if ( x?.name && prop === 'innerBlocks' ) { + if ( x?.name === 'core/page-list' && prop === 'innerBlocks' ) { return true; } }