From 5b8e6064f7f3e06107adca2a03d7d95f04231820 Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Fri, 9 Jun 2023 16:51:51 +0100 Subject: [PATCH 1/5] Remove the syncing --- .../components/link-control/use-internal-value.js | 13 +------------ 1 file changed, 1 insertion(+), 12 deletions(-) diff --git a/packages/block-editor/src/components/link-control/use-internal-value.js b/packages/block-editor/src/components/link-control/use-internal-value.js index ac58c05b10a870..2fbffcea9a2e7e 100644 --- a/packages/block-editor/src/components/link-control/use-internal-value.js +++ b/packages/block-editor/src/components/link-control/use-internal-value.js @@ -1,22 +1,11 @@ /** * WordPress dependencies */ -import { useState, useEffect } from '@wordpress/element'; +import { useState } from '@wordpress/element'; export default function useInternalValue( value ) { const [ internalValue, setInternalValue ] = useState( value || {} ); - // If the value prop changes, update the internal state. - useEffect( () => { - setInternalValue( ( prevValue ) => { - if ( value && value !== prevValue ) { - return value; - } - - return prevValue; - } ); - }, [ value ] ); - const setInternalURLInputValue = ( nextValue ) => { setInternalValue( { ...internalValue, From b9c1b169edb0b17ee493c74de5093b03b6069b9f Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Thu, 27 Jul 2023 09:41:19 +0100 Subject: [PATCH 2/5] Update docs with best practices --- .../src/components/link-control/README.md | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/packages/block-editor/src/components/link-control/README.md b/packages/block-editor/src/components/link-control/README.md index fef68318867e8a..32d89656c3eecd 100644 --- a/packages/block-editor/src/components/link-control/README.md +++ b/packages/block-editor/src/components/link-control/README.md @@ -4,6 +4,26 @@ Renders a link control. A link control is a controlled input which maintains a v It is designed to provide a standardized UI for the creation of a link throughout the Editor, see History section at bottom for further background. +## Best Practices + +### Ensuring unique instances + +It is possible that a given editor may render multiple instances of the `` component. As a result, it is important to ensure each instance is unique across the editor to avoid state "leaking" between components. + +Why would this happen? + +React's reconciliation algorithm means that if you return the same element from a component, it keeps the nodes around as an optimization, even if the props change. This means that if you render two (or more) ``s, it is possible that the `value` from one will appear in the other as you move between them. + +As a result it is recommended that consumers provide a `key` prop to each instance of ``: + +```jsx + +``` + +This will cause React to return the same component/element type but force remount a new instance, thus avoiding the issues described above. + +For more information see: https://github.com/WordPress/gutenberg/pull/34742. + ## Relationship to `` As of Gutenberg 7.4, `` became the default link creation interface within the Block Editor, replacing previous disparate uses of `` and standardizing the UI. From 263d848e2416b620577de16667e857f645a0e911 Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Thu, 27 Jul 2023 09:43:36 +0100 Subject: [PATCH 3/5] Remove unnecessary test coverage --- .../src/components/link-control/test/index.js | 42 ------------------- 1 file changed, 42 deletions(-) diff --git a/packages/block-editor/src/components/link-control/test/index.js b/packages/block-editor/src/components/link-control/test/index.js index 5c44f0295249c8..2d0fc0cb574f7b 100644 --- a/packages/block-editor/src/components/link-control/test/index.js +++ b/packages/block-editor/src/components/link-control/test/index.js @@ -2509,48 +2509,6 @@ describe( 'Controlling link title text', () => { screen.queryByRole( 'textbox', { name: 'Text' } ) ).not.toBeInTheDocument(); } ); - - it( 'should reset state upon controlled value change', async () => { - const user = userEvent.setup(); - const textValue = 'My new text value'; - const mockOnChange = jest.fn(); - - const { rerender } = render( - - ); - - await toggleSettingsDrawer( user ); - - const textInput = screen.queryByRole( 'textbox', { name: 'Text' } ); - - expect( textInput ).toBeVisible(); - - await user.clear( textInput ); - await user.keyboard( textValue ); - - // Was originally title: 'Hello Page', but we've changed it. - rerender( - - ); - - // The text input should not be showing as the form is submitted. - expect( screen.queryByRole( 'textbox', { name: 'Text' } ) ).toHaveValue( - 'Something else' - ); - } ); } ); function getSettingsDrawerToggle() { From 82aeb6f4578f799361c4e95c2f5646f6f234313d Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Thu, 21 Sep 2023 10:41:01 +0100 Subject: [PATCH 4/5] Implement suggestion sync with previous state --- .../src/components/link-control/test/index.js | 42 +++++++++++++++++++ .../link-control/use-internal-value.js | 12 ++++++ 2 files changed, 54 insertions(+) diff --git a/packages/block-editor/src/components/link-control/test/index.js b/packages/block-editor/src/components/link-control/test/index.js index 2d0fc0cb574f7b..5c44f0295249c8 100644 --- a/packages/block-editor/src/components/link-control/test/index.js +++ b/packages/block-editor/src/components/link-control/test/index.js @@ -2509,6 +2509,48 @@ describe( 'Controlling link title text', () => { screen.queryByRole( 'textbox', { name: 'Text' } ) ).not.toBeInTheDocument(); } ); + + it( 'should reset state upon controlled value change', async () => { + const user = userEvent.setup(); + const textValue = 'My new text value'; + const mockOnChange = jest.fn(); + + const { rerender } = render( + + ); + + await toggleSettingsDrawer( user ); + + const textInput = screen.queryByRole( 'textbox', { name: 'Text' } ); + + expect( textInput ).toBeVisible(); + + await user.clear( textInput ); + await user.keyboard( textValue ); + + // Was originally title: 'Hello Page', but we've changed it. + rerender( + + ); + + // The text input should not be showing as the form is submitted. + expect( screen.queryByRole( 'textbox', { name: 'Text' } ) ).toHaveValue( + 'Something else' + ); + } ); } ); function getSettingsDrawerToggle() { diff --git a/packages/block-editor/src/components/link-control/use-internal-value.js b/packages/block-editor/src/components/link-control/use-internal-value.js index 2fbffcea9a2e7e..41cada661c7e66 100644 --- a/packages/block-editor/src/components/link-control/use-internal-value.js +++ b/packages/block-editor/src/components/link-control/use-internal-value.js @@ -5,6 +5,18 @@ import { useState } from '@wordpress/element'; export default function useInternalValue( value ) { const [ internalValue, setInternalValue ] = useState( value || {} ); + const [ previousValue, setPreviousValue ] = useState( value ); + + // If the value prop changes, update the internal state. + // See: + // - https://github.com/WordPress/gutenberg/pull/51387#issuecomment-1722927384. + // - https://react.dev/reference/react/useState#storing-information-from-previous-renders. + if ( value !== previousValue ) { + setPreviousValue( value ); + if ( value !== internalValue ) { + setInternalValue( value ); + } + } const setInternalURLInputValue = ( nextValue ) => { setInternalValue( { From 1e9288f2411f531540db3e8838169b07130f1a8a Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Thu, 5 Oct 2023 11:30:23 +0100 Subject: [PATCH 5/5] Apply update from Code Review --- .../src/components/link-control/use-internal-value.js | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/packages/block-editor/src/components/link-control/use-internal-value.js b/packages/block-editor/src/components/link-control/use-internal-value.js index 41cada661c7e66..9df557be1460cf 100644 --- a/packages/block-editor/src/components/link-control/use-internal-value.js +++ b/packages/block-editor/src/components/link-control/use-internal-value.js @@ -3,6 +3,11 @@ */ import { useState } from '@wordpress/element'; +/** + * External dependencies + */ +import fastDeepEqual from 'fast-deep-equal'; + export default function useInternalValue( value ) { const [ internalValue, setInternalValue ] = useState( value || {} ); const [ previousValue, setPreviousValue ] = useState( value ); @@ -11,11 +16,9 @@ export default function useInternalValue( value ) { // See: // - https://github.com/WordPress/gutenberg/pull/51387#issuecomment-1722927384. // - https://react.dev/reference/react/useState#storing-information-from-previous-renders. - if ( value !== previousValue ) { + if ( ! fastDeepEqual( value, previousValue ) ) { setPreviousValue( value ); - if ( value !== internalValue ) { - setInternalValue( value ); - } + setInternalValue( value ); } const setInternalURLInputValue = ( nextValue ) => {