Fix memoized table object #550
Conversation
Coverage reportCaution An unexpected error occurred. For more details, check console
Report generated by 🧪jest coverage report action from ac1697e |
novykh
left a comment
novykh
left a comment
There was a problem hiding this comment.
This has a number of performance issues. Better to update the selector on the component you want to rerender when columns change.
I think you need this:
But again I would proceed with caution and checking the performance implications - especially on tables that do not need this check.
kapantzak
force-pushed
the
fix-memoized-table-object
branch
from
042711e to
c6c8372
Compare
novykh
left a comment
novykh
left a comment
There was a problem hiding this comment.
Check the comments but for me it's good to go.
| grouping: state.grouping, | ||
| columnsCount: state.table && state.table.getAllColumns().length, | ||
| columnsCount: columns.length, | ||
| columnsFilters: columns.map(({ columnDef }) => ({ |
There was a problem hiding this comment.
Does it need to be an object with filterOptions key? Why not just return the columnDef?.meta?.filter?.options? It will be easier for the diff checker to run shallower by one level :)
| import Cell from "./cell" | ||
|
|
||
| const rerenderSelector = state => { | ||
| const columns = state.table?.getAllColumns() || [] |
There was a problem hiding this comment.
should this become state.table?.getAllColumns?.()?
When we pass a value to
dataColumnsprop of theTablecomponent, that changes over re-renders,BodyHeadercomponent cannot distinguish between different versions oftableobject and keeps the first version of it.By addingtimestampprop totableobject each time we create it, we ensure thatmemorecognizes each version and re-rendersBodyHeadercomponent to reflect latest state.