-
Notifications
You must be signed in to change notification settings - Fork 12
Fix memoized table object #550
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
Conversation
Coverage reportCaution An unexpected error occurred. For more details, check console
Report generated by 🧪jest coverage report action from ac1697e |
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.
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:
columnsCount: state.table && state.table.getAllColumns().length, |
But again I would proceed with caution and checking the performance implications - especially on tables that do not need this check.
042711e
to
c6c8372
Compare
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.
Check the comments but for me it's good to go.
return { | ||
sizing: state.columnSizing, | ||
expanded: state.expanded, | ||
columnVisibility: state.columnVisibility, | ||
selectedRows: state.selectedRows, | ||
grouping: state.grouping, | ||
columnsCount: state.table && state.table.getAllColumns().length, | ||
columnsCount: columns.length, | ||
columnsFilters: columns.map(({ columnDef }) => ({ |
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.
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 :)
@@ -4,13 +4,18 @@ import { useTableState } from "../../provider" | |||
import Cell from "./cell" | |||
|
|||
const rerenderSelector = state => { | |||
const columns = state.table?.getAllColumns() || [] |
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.
should this become state.table?.getAllColumns?.()
?
When we pass a value to
dataColumns
prop of theTable
component, that changes over re-renders,BodyHeader
component cannot distinguish between different versions oftable
object and keeps the first version of it.By addingtimestamp
prop totable
object each time we create it, we ensure thatmemo
recognizes each version and re-rendersBodyHeader
component to reflect latest state.