Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Header toolbar: useCallback to avoid unnecessary rerenders #32406

Merged
merged 2 commits into from
Jun 3, 2021
Merged

Header toolbar: useCallback to avoid unnecessary rerenders #32406

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b62b6f4
Header toolbar: useCallback to avoid unnecessary rerenders
gwwar Jun 2, 2021
a22ab63
Header toolbar: useCallback to avoid unnecessary rerenders in site ed…
gwwar Jun 2, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 20 additions & 14 deletions packages/edit-post/src/components/header/header-toolbar/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import {
} from '@wordpress/editor';
import { Button, ToolbarItem } from '@wordpress/components';
import { listView, plus } from '@wordpress/icons';
import { useRef } from '@wordpress/element';
import { useRef, useCallback } from '@wordpress/element';
import { store as keyboardShortcutsStore } from '@wordpress/keyboard-shortcuts';

/**
Expand All @@ -26,6 +26,10 @@ import { store as keyboardShortcutsStore } from '@wordpress/keyboard-shortcuts';
import TemplateTitle from '../template-title';
import { store as editPostStore } from '../../../store';

const preventDefault = ( event ) => {
event.preventDefault();
};

function HeaderToolbar() {
const inserterButton = useRef();
const { setIsInserterOpened, setIsListViewOpened } = useDispatch(
Expand Down Expand Up @@ -73,6 +77,10 @@ function HeaderToolbar() {
/* translators: accessibility text for the editor toolbar */
const toolbarAriaLabel = __( 'Document tools' );

const toggleListView = useCallback(
() => setIsListViewOpened( ! isListViewOpen ),
[ setIsListViewOpened, isListViewOpen ]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The setter and value here need to be dependencies since this is using a data store.

As an aside this is avoidable in similar handlers when using useState. (Eg where a state setter does not change and using a functional update to avoid a closure with stale data. )

Copy link
Contributor

@mcsf mcsf Jun 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for finding all these unnecessary renders!

Yes, the difference in ergonomics between useState entities and useSelect/useDispatch ones is striking. However, as of #31078 we encourage an "advanced" use case of useSelect that fits the current case very well:

diff --git a/packages/edit-post/src/components/header/header-toolbar/index.js b/packages/edit-post/src/components/header/header-toolbar/index.js
index f3e16a6f2b..1ddab499e2 100644
--- a/packages/edit-post/src/components/header/header-toolbar/index.js
+++ b/packages/edit-post/src/components/header/header-toolbar/index.js
@@ -77,10 +77,10 @@ function HeaderToolbar() {
 	/* translators: accessibility text for the editor toolbar */
 	const toolbarAriaLabel = __( 'Document tools' );
 
-	const toggleListView = useCallback(
-		() => setIsListViewOpened( ! isListViewOpen ),
-		[ setIsListViewOpened, isListViewOpen ]
-	);
+	const { isListViewOpened } = useSelect( editPostStore );
+	const toggleListView = useCallback( () => {
+		setIsListViewOpened( ! isListViewOpened() );
+	}, [] );
 	const overflowItems = (
 		<>
 			<ToolbarItem
diff --git a/packages/edit-site/src/components/header/index.js b/packages/edit-site/src/components/header/index.js
index 2d03cf3e57..e930c45f4b 100644
--- a/packages/edit-site/src/components/header/index.js
+++ b/packages/edit-site/src/components/header/index.js
@@ -99,10 +99,10 @@ export default function Header( {
 		}
 	}, [ isInserterOpen, setIsInserterOpened ] );
 
-	const toggleListView = useCallback(
-		() => setIsListViewOpened( ! isListViewOpen ),
-		[ setIsListViewOpened, isListViewOpen ]
-	);
+	const { isListViewOpened } = useSelect( editSiteStore );
+	const toggleListView = useCallback( () => {
+		setIsListViewOpened( ! isListViewOpened() );
+	}, [] );
 
 	return (
 		<div className="edit-site-header">

I personally took the liberty of also removing the setter from the dependencies because I don't think that object will ever change, but that's up for debate. What really matters is that the callback itself no longer needs to be recomputed based on the change of a piece of state that the callback only cares about in an eventual future.

It's not as nice as setFoo( foo => ! foo ), but it's a start.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, thanks @mcsf !

);
const overflowItems = (
<>
<ToolbarItem
Expand All @@ -90,13 +98,20 @@ function HeaderToolbar() {
isPressed={ isListViewOpen }
/* translators: button label text should, if possible, be under 16 characters. */
label={ __( 'List View' ) }
onClick={ () => setIsListViewOpened( ! isListViewOpen ) }
onClick={ toggleListView }
shortcut={ listViewShortcut }
showTooltip={ ! showIconLabels }
/>
</>
);

const openInserter = useCallback( () => {
if ( isInserterOpened ) {
// Focusing the inserter button closes the inserter popover
inserterButton.current.focus();
} else {
setIsInserterOpened( true );
}
}, [ isInserterOpened, setIsInserterOpened ] );
return (
<NavigableToolbar
className="edit-post-header-toolbar"
Expand All @@ -109,17 +124,8 @@ function HeaderToolbar() {
className="edit-post-header-toolbar__inserter-toggle"
variant="primary"
isPressed={ isInserterOpened }
onMouseDown={ ( event ) => {
event.preventDefault();
} }
onClick={ () => {
if ( isInserterOpened ) {
// Focusing the inserter button closes the inserter popover
inserterButton.current.focus();
} else {
setIsInserterOpened( true );
}
} }
onMouseDown={ preventDefault }
onClick={ openInserter }
disabled={ ! isInserterEnabled }
icon={ plus }
/* translators: button label text should, if possible, be under 16
Expand Down
37 changes: 22 additions & 15 deletions packages/edit-site/src/components/header/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/**
* WordPress dependencies
*/
import { useRef } from '@wordpress/element';
import { useCallback, useRef } from '@wordpress/element';
import { useViewportMatch } from '@wordpress/compose';
import {
ToolSelector,
Expand All @@ -27,6 +27,10 @@ import DocumentActions from './document-actions';
import TemplateDetails from '../template-details';
import { store as editSiteStore } from '../../store';

const preventDefault = ( event ) => {
event.preventDefault();
};

export default function Header( {
openEntitiesSavedStates,
isEntitiesSavedStatesOpen,
Expand Down Expand Up @@ -86,6 +90,20 @@ export default function Header( {

const isLargeViewport = useViewportMatch( 'medium' );

const openInserter = useCallback( () => {
if ( isInserterOpen ) {
// Focusing the inserter button closes the inserter popover
inserterButton.current.focus();
} else {
setIsInserterOpened( true );
}
}, [ isInserterOpen, setIsInserterOpened ] );

const toggleListView = useCallback(
() => setIsListViewOpened( ! isListViewOpen ),
[ setIsListViewOpened, isListViewOpen ]
);

return (
<div className="edit-site-header">
<div className="edit-site-header_start">
Expand All @@ -95,17 +113,8 @@ export default function Header( {
variant="primary"
isPressed={ isInserterOpen }
className="edit-site-header-toolbar__inserter-toggle"
onMouseDown={ ( event ) => {
event.preventDefault();
} }
onClick={ () => {
if ( isInserterOpen ) {
// Focusing the inserter button closes the inserter popover
inserterButton.current.focus();
} else {
setIsInserterOpened( true );
}
} }
onMouseDown={ preventDefault }
onClick={ openInserter }
icon={ plus }
label={ _x(
'Toggle block inserter',
Expand All @@ -123,9 +132,7 @@ export default function Header( {
isPressed={ isListViewOpen }
/* translators: button label text should, if possible, be under 16 characters. */
label={ __( 'List View' ) }
onClick={ () =>
setIsListViewOpened( ! isListViewOpen )
}
onClick={ toggleListView }
shortcut={ listViewShortcut }
/>
</>
Expand Down