Make it easier to prevent unnecessary DataTable re-renders
#1357
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This hook is useful for wrapping callbacks to avoid unnecessary re-renders due to callback props changing. See hypothesis/via#1129.
- 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(() => <DataTable ... />, [...]);
return <Scroll>{table}</Scroll>;
}
```
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.
robertknight
commented
Nov 9, 2023
| @@ -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; | |||
| } | |||
There was a problem hiding this comment.
Hoisted this default prop value to a top-level function so it doesn't change on each render.
robertknight
commented
Nov 9, 2023
| onConfirmRow?.(row); | ||
| } |
There was a problem hiding this comment.
Wrap callback props so we don't unnecessarily re-render even if consumers fail to useCallback these.
robertknight
commented
Nov 9, 2023
| @@ -0,0 +1,39 @@ | |||
| // These tests use Preact directly, rather than via Enzyme, to simplify debugging. | |||
There was a problem hiding this comment.
This file and use-stable-callback.ts were copied from Via.
robertknight
added a commit
to hypothesis/lms
that referenced
this pull request
Nov 9, 2023
This avoids an unnecessary re-render of the DataTable if the chapters haven't changed, which is important if the book has a lot of pages or chapters. For this change to work requires hypothesis/frontend-shared#1357.
robertknight
added a commit
to hypothesis/lms
that referenced
this pull request
Nov 9, 2023
This avoids an unnecessary re-render of the DataTable if the chapters haven't changed, which is important if the book has a lot of pages or chapters. For this change to work requires hypothesis/frontend-shared#1357.
robertknight
added a commit
to hypothesis/via
that referenced
this pull request
Nov 10, 2023
…frontend-shared This hook was copied from Via into the frontend-shared package in hypothesis/frontend-shared#1357.
robertknight
added a commit
that referenced
this pull request
Nov 10, 2023
This was an oversight from #1357.
robertknight
added a commit
that referenced
this pull request
Nov 10, 2023
This was an oversight from #1357.
robertknight
added a commit
to hypothesis/via
that referenced
this pull request
Nov 10, 2023
…frontend-shared This hook was copied from Via into the frontend-shared package in hypothesis/frontend-shared#1357.
robertknight
added a commit
to hypothesis/via
that referenced
this pull request
Nov 10, 2023
…frontend-shared This hook was copied from Via into the frontend-shared package in hypothesis/frontend-shared#1357.
robertknight
added a commit
to hypothesis/via
that referenced
this pull request
Nov 13, 2023
…frontend-shared This hook was copied from Via into the frontend-shared package in hypothesis/frontend-shared#1357.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
While adding some new UI to the VitalSource chapter/page selection dialog (see hypothesis/lms#5808) I found that the UI got very slow when working with a book that had a large (> 1000) number of entries in the chapter list. Such a large chapter list could really use virtualization to limit rendering time, but memo-ing is much easier to add and still has a significant impact.
I tried memo-ing the
DataTablebut that initially had no effect, due to the parentScrolltriggering a re-render even if theDataTablewas memoed. This PR fixes that issue, and adds a bunch of additional memo-ing insideDataTableto make it easier for downstream apps to prevent re-renders. They now need to just memo: 1) The rows, 2) the columns, 3) the selected item. For better results they can also memo the wholeDataTable.In the process I imported the useStableCallback hook from Via since it is generally useful for these kinds of optimizations and I expect it will be useful in other applications in future.