[DataGrid] Refactor rendering, remove rafUpdate #532
Conversation
dtassone
commented
Nov 3, 2020
•
dtassone
commented
- Fix flaky test on master
- Reduce dependency on custom requestAnimationFrame logic
| import { ApiRef } from '../../../models/api/apiRef'; | ||
| import { GridApi } from '../../../models/api/gridApi'; | ||
| import { useLogger } from '../../utils/useLogger'; | ||
| import { getInitialState } from './gridState'; | ||
|
|
||
| export const useGridApi = (apiRef: ApiRef): GridApi => { | ||
| const logger = useLogger('useGridApi'); | ||
| const [, forceUpdate] = React.useState(); |
There was a problem hiding this comment.
| const [, forceUpdate] = React.useState(); | |
| const [, forceUpdate] = React.useReducer((acc) => acc + 1, 0); |
There was a problem hiding this comment.
Why? I prefer to useState so I can pass the state I need...
There was a problem hiding this comment.
The current usage force you to call forceUpdate with an argument, and it has to have a unique reference, like a new object or a state setting that returns a unique reference. In the current logic, it's called with a function. It could be simpler if it didn't need to accept an argument, and maybe less error prone?
There was a problem hiding this comment.
it does, you just need to pass a function
There was a problem hiding this comment.
Why? I prefer to useState so I can pass the state I need...
The name forceUpdate is misleading. You have to pass the correct parameter to schedule work.
Though generally forcing a re-render is rarely needed. Whenever you forceUpdate why can't you move whatever changed into state?
There was a problem hiding this comment.
There is one case that took me some time to understand, I think that by doing
apiRef.current.forceUpdate(() => apiRef.current.state),We can skip updates when the state hasn't changed, which can be an interesting feature. I was confused by the name, we definitely need to find a different one. IMHO forceUpdate === no arguments. What about setState?
There was a problem hiding this comment.
this is not a setState, it's just to force the ui to rerender
There was a problem hiding this comment.
@dtassone Are you saying that all we need is a new reference? If its the case, I would rather do:
-apiRef.current.forceUpdate(() => apiRef.current.state),
+apiRef.current.forceUpdate({}),to remove the dependency another part of the logic.
There was a problem hiding this comment.
this is not a setState, it's just to force the ui to rerender
But it is? Assuming you refer to setState returned from the useState hook then I don't see any difference. Both setState and forceUpdate have to know what the current state is to actually force a re-rerender (which isn't clear why you need this).
packages/grid/_modules_/grid/hooks/features/virtualization/useVirtualRows.ts
Outdated
Show resolved
Hide resolved
oliviertassinari
changed the title
oliviertassinari
added
component: data grid
Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
…VirtualRows.ts Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
* refactored rendering, removed rafUpdate * fix lint * Update packages/grid/_modules_/grid/models/api/stateApi.ts Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com> * cleanup * cleanup * refactor window to ownerdoc... * Update packages/grid/_modules_/grid/components/AutoSizer.tsx Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com> * Update packages/grid/_modules_/grid/hooks/features/virtualization/useVirtualRows.ts Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com> * small refact * fix bad commit review * better story * revert refactoring as breaking the tests * prettier * slow down resize debounce * make sure the tests don't rely on the resize event * no any * fine grain timer in test * hedge against weak timing guarentees of setTimeout Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
