-
Notifications
You must be signed in to change notification settings - Fork 2
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.
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
Make it easier to prevent unnecessary DataTable
re-renders
#1357
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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<Row> = CompositeProps & | |
ComponentProps<Row> & | ||
Omit<JSX.HTMLAttributes<HTMLElement>, 'size' | 'rows' | 'role' | 'loading'>; | ||
|
||
function defaultRenderItem<Row>(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<Row>({ | |
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<Row>({ | |
}); | ||
|
||
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); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wrap callback props so we don't unnecessarily re-render even if consumers fail to |
||
}); | ||
|
||
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<Row>({ | |
// excess vertical space in tables with sparse rows data. | ||
const withFoot = !loading && rows.length > 0; | ||
|
||
const tableRows = useMemo(() => { | ||
return rows.map((row, idx) => ( | ||
<TableRow | ||
key={idx} | ||
selected={row === selectedRow} | ||
onClick={() => selectRow(row)} | ||
onFocus={() => selectRow(row)} | ||
onDblClick={() => confirmRow(row)} | ||
onKeyDown={event => handleKeyDown(event, row)} | ||
> | ||
{fields.map(field => ( | ||
<TableCell key={field}> | ||
{renderItem(row, field as keyof Row)} | ||
</TableCell> | ||
))} | ||
</TableRow> | ||
)); | ||
}, [ | ||
confirmRow, | ||
fields, | ||
renderItem, | ||
handleKeyDown, | ||
rows, | ||
selectRow, | ||
selectedRow, | ||
]); | ||
|
||
return ( | ||
<Table | ||
data-composite-component="DataTable" | ||
|
@@ -152,23 +186,7 @@ export default function DataTable<Row>({ | |
</TableRow> | ||
</TableHead> | ||
<TableBody> | ||
{!loading && | ||
rows.map((row, idx) => ( | ||
<TableRow | ||
key={idx} | ||
selected={row === selectedRow} | ||
onClick={() => selectRow(row)} | ||
onFocus={() => selectRow(row)} | ||
onDblClick={() => confirmRow(row)} | ||
onKeyDown={event => handleKeyDown(event, row)} | ||
> | ||
{fields.map(field => ( | ||
<TableCell key={field}> | ||
{renderItem(row, field as keyof Row)} | ||
</TableCell> | ||
))} | ||
</TableRow> | ||
))} | ||
{!loading && tableRows} | ||
{noContent && ( | ||
<tr> | ||
<td colSpan={columns.length} className="text-center p-3"> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
// These tests use Preact directly, rather than via Enzyme, to simplify debugging. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This file and |
||
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 <button onClick={stableCallback}>Test</button>; | ||
} | ||
|
||
it('returns a wrapper with a stable identity', () => { | ||
render(<Widget callback={() => {}} />, container); | ||
render(<Widget callback={() => {}} />, 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(<Widget callback={() => {}} />, container); | ||
render(<Widget callback={stub} />, container); | ||
|
||
assert.equal(stableCallbackValues.length, 2); | ||
stableCallbackValues.at(-1)('foo', 'bar', 'baz'); | ||
|
||
assert.calledWith(stub, 'foo', 'bar', 'baz'); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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, A extends any[], F extends (...a: A) => 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; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hoisted this default prop value to a top-level function so it doesn't change on each render.