Skip to content

Commit

Permalink
perf: Reduce unnecessary LinkPicker re-renders (#51613)
Browse files Browse the repository at this point in the history
* perf: Reduce unnecessary LinkPicker re-renders

Combining state with an object in a single `useState` meant a new object
was set to the state regardless of whether new values were present. This
resulted in unexpected `act` warnings caused by unnecessary re-renders.

Refactoring LinkPicker to separate the two state values into separate
`useState` calls resolves this issue by using primitives that can be
compared directly, which results in `useState` not triggering re-renders
when the values are the same.

* refactor: Simplify component callback invocations

The wrapping anonymous function is unnecessary now that the callback are
simply setting a single state value. Also, passing anonymous functions
to child components can lead to unnecessary re-renders.

* build: Remove erroneous reassure argument

This argument does not exist in the older version of reassure used.
Also, the value is already passed as an environment variable.

* test: Add LinkPicker performance test
  • Loading branch information
dcalhoun committed Aug 7, 2023
1 parent 6a77b43 commit 946333d
Show file tree
Hide file tree
Showing 4 changed files with 43 additions and 15 deletions.
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -315,7 +315,7 @@
"test:native:clean": "jest --clearCache --config test/native/jest.config.js; rm -rf $TMPDIR/jest_*",
"test:native:debug": "cross-env NODE_ENV=test node --inspect-brk node_modules/.bin/jest --runInBand --verbose --config test/native/jest.config.js",
"test:native:perf": "cross-env TEST_RUNNER_ARGS='--runInBand --config test/native/jest.config.js --testMatch \"**/performance/*.native.[jt]s?(x)\"' reassure",
"test:native:perf:baseline": "cross-env TEST_RUNNER_ARGS='--runInBand --config test/native/jest.config.js --testMatch \"**/performance/*.native.[jt]s?(x)\"' reassure --baseline --testMatch \"**/performance/*.native.[jt]s?(x)\"",
"test:native:perf:baseline": "cross-env TEST_RUNNER_ARGS='--runInBand --config test/native/jest.config.js --testMatch \"**/performance/*.native.[jt]s?(x)\"' reassure --baseline",
"test:native:update": "npm run test:native -- --updateSnapshot",
"test:performance": "wp-scripts test-e2e --config packages/e2e-tests/jest.performance.config.js",
"test:performance:debug": "wp-scripts --inspect-brk test-e2e --runInBand --no-cache --verbose --config packages/e2e-tests/jest.performance.config.js --puppeteer-devtools",
Expand Down
20 changes: 7 additions & 13 deletions packages/components/src/mobile/link-picker/index.native.js
Original file line number Diff line number Diff line change
Expand Up @@ -57,10 +57,8 @@ export const LinkPicker = ( {
onLinkPicked,
onCancel: cancel,
} ) => {
const [ { value, clipboardUrl }, setValue ] = useState( {
value: initialValue,
clipboardUrl: '',
} );
const [ value, setValue ] = useState( initialValue );
const [ clipboardUrl, setClipboardUrl ] = useState( '' );
const directEntry = createDirectEntry( value );

// The title of a direct entry is displayed as the raw input value, but if we
Expand All @@ -74,7 +72,8 @@ export const LinkPicker = ( {
};

const clear = () => {
setValue( { value: '', clipboardUrl } );
setValue( '' );
setClipboardUrl( '' );
};

const omniCellStyle = usePreferredColorSchemeStyle(
Expand All @@ -89,11 +88,8 @@ export const LinkPicker = ( {

useEffect( () => {
getURLFromClipboard()
.then( ( url ) => setValue( { value, clipboardUrl: url } ) )
.catch( () => setValue( { value, clipboardUrl: '' } ) );
// Disable reason: deferring this refactor to the native team.
// see https://github.com/WordPress/gutenberg/pull/41166
// eslint-disable-next-line react-hooks/exhaustive-deps
.then( setClipboardUrl )
.catch( () => setClipboardUrl( '' ) );
}, [] );

// TODO: Localize the accessibility label.
Expand All @@ -115,9 +111,7 @@ export const LinkPicker = ( {
autoCapitalize="none"
autoCorrect={ false }
keyboardType="url"
onChangeValue={ ( newValue ) => {
setValue( { value: newValue, clipboardUrl } );
} }
onChangeValue={ setValue }
onSubmit={ onSubmit }
/* eslint-disable-next-line jsx-a11y/no-autofocus */
autoFocus
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
/**
* External dependencies
*/
import { act, measurePerformance } from 'test/helpers';
import Clipboard from '@react-native-clipboard/clipboard';

/**
* Internal dependencies
*/
import { LinkPicker } from '../../index';

describe( 'LinkPicker', () => {
const onLinkPicked = jest.fn();
const onCancel = jest.fn();
const clipboardResult = Promise.resolve( '' );
Clipboard.getString.mockReturnValue( clipboardResult );

it( 'performance is stable when clipboard results do not change', async () => {
const scenario = async () => {
// Given the clipboard result is an empty string, there are no
// user-facing changes to query. Thus, we must await the promise
// itself.
await act( () => clipboardResult );
};

await measurePerformance(
<LinkPicker
onLinkPicked={ onLinkPicked }
onCancel={ onCancel }
value=""
/>,
{ scenario }
);
} );
} );
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,6 @@ describe.each( [
)
);
if ( type === 'core/image' ) {
// Wait for side effects produced by Clipboard and link suggestions
fireEvent.press( subject.getByLabelText( /Custom URL/ ) );
}
await subject.findByLabelText( 'Apply' );
Expand Down

0 comments on commit 946333d

Please sign in to comment.