From 941ccacf356ee336fdd8fdfa19c23ce5de44b7c2 Mon Sep 17 00:00:00 2001 From: Robert Knight Date: Thu, 9 Nov 2023 13:55:01 +0000 Subject: [PATCH 1/2] Import `useStableCallback` hook from Via This hook is useful for wrapping callbacks to avoid unnecessary re-renders due to callback props changing. See https://github.com/hypothesis/via/pull/1129. --- src/hooks/test/use-stable-callback-test.js | 39 ++++++++++++++++++++++ src/hooks/use-stable-callback.ts | 23 +++++++++++++ 2 files changed, 62 insertions(+) create mode 100644 src/hooks/test/use-stable-callback-test.js create mode 100644 src/hooks/use-stable-callback.ts diff --git a/src/hooks/test/use-stable-callback-test.js b/src/hooks/test/use-stable-callback-test.js new file mode 100644 index 000000000..f1b7ff2ad --- /dev/null +++ b/src/hooks/test/use-stable-callback-test.js @@ -0,0 +1,39 @@ +// These tests use Preact directly, rather than via Enzyme, to simplify debugging. +import { render } from 'preact'; + +import { useStableCallback } from '../use-stable-callback'; + +describe('useStableCallback', () => { + let container; + let stableCallbackValues; + + beforeEach(() => { + container = document.createElement('div'); + stableCallbackValues = []; + }); + + function Widget({ callback }) { + const stableCallback = useStableCallback(callback); + stableCallbackValues.push(stableCallback); + return ; + } + + it('returns a wrapper with a stable identity', () => { + render( {}} />, container); + render( {}} />, container); + + assert.equal(stableCallbackValues.length, 2); + assert.equal(stableCallbackValues[0], stableCallbackValues[1]); + }); + + it('returned wrapper forwards to the latest callback', () => { + const stub = sinon.stub(); + render( {}} />, container); + render(, container); + + assert.equal(stableCallbackValues.length, 2); + stableCallbackValues.at(-1)('foo', 'bar', 'baz'); + + assert.calledWith(stub, 'foo', 'bar', 'baz'); + }); +}); diff --git a/src/hooks/use-stable-callback.ts b/src/hooks/use-stable-callback.ts new file mode 100644 index 000000000..21dd230c9 --- /dev/null +++ b/src/hooks/use-stable-callback.ts @@ -0,0 +1,23 @@ +import { useRef } from 'preact/hooks'; + +/** + * Return a function which wraps a callback to give it a stable value. + * + * The wrapper has a stable value across renders, but always forwards to the + * callback from the most recent render. This is useful if you want to use a + * callback inside a `useEffect` or `useMemo` hook without re-running the effect + * or re-computing the memoed value when the callback changes. + */ +export function useStableCallback R>( + callback: F, +): F { + const wrapper = useRef({ + callback, + call: (...args: A) => wrapper.current.callback(...args), + }); + + // On each render, save the last callback value. + wrapper.current.callback = callback; + + return wrapper.current.call as F; +} From 6640b784ea47069ea9c7da0f9ba0e1712a21e238 Mon Sep 17 00:00:00 2001 From: Robert Knight Date: Thu, 9 Nov 2023 13:57:01 +0000 Subject: [PATCH 2/2] Make `DataTable` less prone to unnecessary re-renders - Memo context provider values in various components. Otherwise every render of the component containing the `*.Provider` will cause any descendant consumers to re-render, even if the descendant has been memoed. Before this change, JSX like the following would result in a `DataTable` being re-rendered on every `Container` render: ``` function Container() { const table = useMemo(() => , [...]); return {table}; } ``` This happened because the `Scroll` component contained a context provider that was consumed by the `DataTable` and thus every render of the `Scroll` would re-render the `DataTable` child. - Memo rendered `DataTable` rows such that they only re-render if one of the following changes: - The row content - The columns - A custom `renderItem` callback - The selected row Users of `DataTable` can then more easily eliminate re-renders of large tables by memoing these props/callbacks. --- src/components/data/DataTable.tsx | 82 +++++++++++++++++++------------ src/components/data/Scroll.tsx | 10 ++-- src/components/data/Table.tsx | 16 +++--- src/components/data/TableBody.tsx | 11 +++-- src/components/data/TableFoot.tsx | 10 ++-- src/components/data/TableHead.tsx | 11 +++-- 6 files changed, 88 insertions(+), 52 deletions(-) diff --git a/src/components/data/DataTable.tsx b/src/components/data/DataTable.tsx index b68f700ea..2cf445108 100644 --- a/src/components/data/DataTable.tsx +++ b/src/components/data/DataTable.tsx @@ -1,7 +1,8 @@ import type { ComponentChildren, JSX } from 'preact'; -import { useContext, useEffect } from 'preact/hooks'; +import { useCallback, useContext, useEffect, useMemo } from 'preact/hooks'; import { useArrowKeyNavigation } from '../../hooks/use-arrow-key-navigation'; +import { useStableCallback } from '../../hooks/use-stable-callback'; import { useSyncedRef } from '../../hooks/use-synced-ref'; import type { CompositeProps } from '../../types'; import { downcastRef } from '../../util/typing'; @@ -49,6 +50,10 @@ export type DataTableProps = CompositeProps & ComponentProps & Omit, 'size' | 'rows' | 'role' | 'loading'>; +function defaultRenderItem(r: Row, field: keyof Row): ComponentChildren { + return r[field] as ComponentChildren; +} + /** * An interactive table of rows and columns with a sticky header. */ @@ -61,7 +66,7 @@ export default function DataTable({ title, selectedRow, loading = false, - renderItem = (r: Row, field: keyof Row) => r[field] as ComponentChildren, + renderItem = defaultRenderItem, onSelectRow, onConfirmRow, emptyMessage, @@ -81,23 +86,25 @@ export default function DataTable({ }); const noContent = loading || (!rows.length && emptyMessage); - const fields = columns.map(column => column.field); + const fields = useMemo(() => columns.map(column => column.field), [columns]); - function selectRow(row: Row) { + const selectRow = useStableCallback((row: Row) => { onSelectRow?.(row); - } - - function confirmRow(row: Row) { + }); + const confirmRow = useStableCallback((row: Row) => { onConfirmRow?.(row); - } + }); - function handleKeyDown(event: KeyboardEvent, row: Row) { - if (event.key === 'Enter') { - confirmRow(row); - event.preventDefault(); - event.stopPropagation(); - } - } + const handleKeyDown = useCallback( + (event: KeyboardEvent, row: Row) => { + if (event.key === 'Enter') { + confirmRow(row); + event.preventDefault(); + event.stopPropagation(); + } + }, + [confirmRow], + ); // Ensure that a selected row is visible when this table is within // a scrolling context @@ -131,6 +138,33 @@ export default function DataTable({ // excess vertical space in tables with sparse rows data. const withFoot = !loading && rows.length > 0; + const tableRows = useMemo(() => { + return rows.map((row, idx) => ( + selectRow(row)} + onFocus={() => selectRow(row)} + onDblClick={() => confirmRow(row)} + onKeyDown={event => handleKeyDown(event, row)} + > + {fields.map(field => ( + + {renderItem(row, field as keyof Row)} + + ))} + + )); + }, [ + confirmRow, + fields, + renderItem, + handleKeyDown, + rows, + selectRow, + selectedRow, + ]); + return ( ({ - {!loading && - rows.map((row, idx) => ( - selectRow(row)} - onFocus={() => selectRow(row)} - onDblClick={() => confirmRow(row)} - onKeyDown={event => handleKeyDown(event, row)} - > - {fields.map(field => ( - - {renderItem(row, field as keyof Row)} - - ))} - - ))} + {!loading && tableRows} {noContent && (
diff --git a/src/components/data/Scroll.tsx b/src/components/data/Scroll.tsx index d99cd72af..0a1f36576 100644 --- a/src/components/data/Scroll.tsx +++ b/src/components/data/Scroll.tsx @@ -1,5 +1,6 @@ import classnames from 'classnames'; import type { JSX } from 'preact'; +import { useMemo } from 'preact/hooks'; import { useSyncedRef } from '../../hooks/use-synced-ref'; import type { PresentationalProps } from '../../types'; @@ -26,9 +27,12 @@ export default function Scroll({ }: ScrollProps) { const ref = useSyncedRef(elementRef); - const scrollContext: ScrollInfo = { - scrollRef: ref, - }; + const scrollContext: ScrollInfo = useMemo( + () => ({ + scrollRef: ref, + }), + [ref], + ); return ( diff --git a/src/components/data/Table.tsx b/src/components/data/Table.tsx index 5b601c323..5e582aeba 100644 --- a/src/components/data/Table.tsx +++ b/src/components/data/Table.tsx @@ -1,5 +1,6 @@ import classnames from 'classnames'; import type { JSX } from 'preact'; +import { useMemo } from 'preact/hooks'; import { useSyncedRef } from '../../hooks/use-synced-ref'; import type { PresentationalProps } from '../../types'; @@ -34,12 +35,15 @@ export default function Table({ }: TableProps) { const ref = useSyncedRef(elementRef); - const tableContext: TableInfo = { - interactive, - stickyHeader, - borderless, - tableRef: ref, - }; + const tableContext: TableInfo = useMemo( + () => ({ + interactive, + stickyHeader, + borderless, + tableRef: ref, + }), + [borderless, interactive, stickyHeader, ref], + ); return ( diff --git a/src/components/data/TableBody.tsx b/src/components/data/TableBody.tsx index c66c63be9..80d4c4b0a 100644 --- a/src/components/data/TableBody.tsx +++ b/src/components/data/TableBody.tsx @@ -1,6 +1,6 @@ import classnames from 'classnames'; import type { JSX } from 'preact'; -import { useContext } from 'preact/hooks'; +import { useContext, useMemo } from 'preact/hooks'; import type { PresentationalProps } from '../../types'; import { downcastRef } from '../../util/typing'; @@ -22,9 +22,12 @@ export default function TableBody({ ...htmlAttributes }: TableBodyProps) { const tableContext = useContext(TableContext); - const sectionContext: TableSection = { - section: 'body', - }; + const sectionContext: TableSection = useMemo( + () => ({ + section: 'body', + }), + [], + ); return ( diff --git a/src/components/data/TableFoot.tsx b/src/components/data/TableFoot.tsx index 9be573be2..0935c8f6c 100644 --- a/src/components/data/TableFoot.tsx +++ b/src/components/data/TableFoot.tsx @@ -1,5 +1,6 @@ import classnames from 'classnames'; import type { JSX } from 'preact'; +import { useMemo } from 'preact/hooks'; import type { PresentationalProps } from '../../types'; import { downcastRef } from '../../util/typing'; @@ -20,9 +21,12 @@ export default function TableFoot({ ...htmlAttributes }: TableFootProps) { - const sectionContext: TableSection = { - section: 'foot', - }; + const sectionContext: TableSection = useMemo( + () => ({ + section: 'foot', + }), + [], + ); return ( diff --git a/src/components/data/TableHead.tsx b/src/components/data/TableHead.tsx index 96674555b..ec10f0b6a 100644 --- a/src/components/data/TableHead.tsx +++ b/src/components/data/TableHead.tsx @@ -1,6 +1,6 @@ import classnames from 'classnames'; import type { JSX } from 'preact'; -import { useContext } from 'preact/hooks'; +import { useContext, useMemo } from 'preact/hooks'; import type { PresentationalProps } from '../../types'; import { downcastRef } from '../../util/typing'; @@ -23,9 +23,12 @@ export default function TableHead({ }: TableHeadProps) { const tableContext = useContext(TableContext); - const sectionContext: TableSection = { - section: 'head', - }; + const sectionContext: TableSection = useMemo( + () => ({ + section: 'head', + }), + [], + ); return (