[DataGrid] Raf Timer stored in apiRef #506

dtassone · 2020-10-27T17:55:04Z

This PR allows to coordinates all calls to requestAnimationFrame

It fixes the raf violation

oliviertassinari

I'm not hands-on in the grid, so it's only feedback from a high-level perspective, the change seems to go in the opposite direction I was expecting. We are introducing a leaking side effect, an update scheduled in one use ref call can be canceled and never called again in another update, this will likely create bugs. I mean, it only works if the use raf hooks are used in the same component. Have you considered to share the update function instead of the then timer? This would guarantee the constraint and hence avoid dropped updates.

I don't understand why we need to use Raf in the first place. react-virtualized, react-window, react-virtuoso, react-virtual barely uses it. I would encourage we remove this hook from the codebase to make usage of Raf painful, the default solution being to defer rendering prioritization to React.

packages/grid/_modules_/grid/models/api/coreApi.ts

packages/grid/_modules_/grid/hooks/utils/useScrollFn.ts

oliviertassinari · 2020-10-27T19:40:34Z

I'm having a closer look at the usage of raf:

In useRow.js, we can remove it. There is a set state call that render. I haven't seen any end-to-end tests fail without. I mean, puppeteer tests are all red for me on master but karma one all passes. If this leads to a regression, it's a great time to add a nest test case.
In useColumns.js, we should be able to remove it with the migration to the new state logic, updateColumns is a public API, it should set the state and tell React to rerender, it doesn't right now, which is likely a bug. Once fixed, we can drop useRafUpdate from the file.
In useScrollFn.js, the use case looks correct, in which case, we don't need to share the request animation, it's a non-CPU intensive callback.

So it seems that we can/should remove the hook.

dtassone · 2020-10-28T11:13:49Z

I'm having a closer look at the usage of raf:

In useRow.js, we can remove it. There is a set state call that render. I haven't seen any end-to-end tests fail without. I mean, puppeteer tests are all red for me on master but karma one all passes. If this leads to a regression, it's a great time to add a nest test case.

In useColumns.js, we should be able to remove it with the migration to the new state logic, updateColumns is a public API, it should set the state and tell React to rerender, it doesn't right now, which is likely a bug. Once fixed, we can drop useRafUpdate from the file.

In useScrollFn.js, the use case looks correct, in which case, we don't need to share the request animation, it's a non-CPU intensive callback.

So it seems that we can/should remove the hook.

Please try on the fast streaming stories.

oliviertassinari · 2020-10-28T11:18:05Z

Please try on the fast streaming stories.

Isn't streaming a different concern? Why apply the same solution for the other use cases? I definitely agree that for streaming we could considering rerendering at each RAF, bypassing React. But at the same time, isn't it the same with the onScroll events (it fires a lot and yet we don't use it)? In the above exploration, I believe streaming has no impact. 1. is already backed by a React state, 2. will need to be (it won't behave correctly without), 3. can be done with a set timeout or a RAF.

dtassone · 2020-10-28T11:28:39Z

Please try on the fast streaming stories.

Isn't streaming a different concern? Why apply the same solution for the other use cases? I definitely agree that for streaming we could considering rerendering at each raf, bypassing React. But at the same time, isn't it the same with the onScroll events (it fires a lot)? In the above exploration, this has no impact. 1. is already backed by a React state, 2. will need to be (it won't behave correctly), 3. need to use a raf.

I'm not sure I understand what you mean.
If you update the rows super fast as in a streaming case then it needs to coordinate tightly with the scrolling events without blocking the UI.
I do not see it used directly in columns

oliviertassinari

We can't share rerendering between different components.

Conflicts: packages/grid/_modules_/grid/hooks/features/virtualization/useVirtualRows.ts packages/grid/_modules_/grid/hooks/utils/useScrollFn.ts

packages/grid/_modules_/grid/hooks/utils/useScrollFn.ts

packages/grid/_modules_/grid/models/api/coreApi.ts

oliviertassinari · 2020-10-29T15:20:00Z

packages/grid/x-grid/src/XGrid.test.tsx

      expect(checkbox).to.have.property('checked', true);

      fireEvent.click(screen.getByRole('cell', { name: 'Nike' }));
      await raf();
-      expect(row!.classList.contains('Mui-selected')).to.equal(false);
+      expect(row!.classList.contains('Mui-selected')).to.equal(false, 'class mui-selected 2');


Like in Jest we don't use assertion descriptions. It's very often outdated and doesn't help much.

Suggested change

expect(row!.classList.contains('Mui-selected')).to.equal(false, 'class mui-selected 2');

expect(row!.classList.contains('Mui-selected')).to.equal(false);

this is to see where the fails in the ci

Yes, I thought you were using Jest in the past, this isn't supported in Jest :p

Happy to keep it, my feedback, at its core, is that it's often ignored and get outdated after refactoring in the tests. So usually, we don't bother 🤷‍♂️

oliviertassinari · 2020-10-29T15:20:07Z

packages/grid/x-grid/src/XGrid.test.tsx

-      await raf();
-      expect(row!.classList.contains('Mui-selected')).to.equal(true);
+      await sleep(100)
+      expect(row!.classList.contains('Mui-selected')).to.equal(true, 'class mui-selected 1');


Suggested change

expect(row!.classList.contains('Mui-selected')).to.equal(true, 'class mui-selected 1');

expect(row!.classList.contains('Mui-selected')).to.equal(true);

By leaving this we know where the test can potentially failed. Up to you?

packages/grid/_modules_/grid/hooks/utils/useScrollFn.ts

packages/grid/_modules_/grid/hooks/utils/useRafUpdate.ts

Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>

…fRefact

Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>

oliviertassinari · 2020-10-31T01:12:58Z

Browser tests are flaky after the change. Something is wrong with the logic.

* add rafTimer Conflicts: packages/grid/_modules_/grid/hooks/features/virtualization/useVirtualRows.ts packages/grid/_modules_/grid/hooks/utils/useScrollFn.ts * fix cyclic dep and cleanup * fix import * fix import * trying to fix test in CI * refactor scroll * Update packages/grid/_modules_/grid/hooks/utils/useRafUpdate.ts Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com> * Update packages/grid/_modules_/grid/hooks/utils/useScrollFn.ts Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com> * fix tests * prettier * fix test * fix test * fix tests * more sleep * Update packages/grid/_modules_/grid/models/api/coreApi.ts Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com> * Update packages/grid/_modules_/grid/models/api/coreApi.ts Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com> * small refactoring Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>

dtassone requested a review from oliviertassinari October 27, 2020 17:55

oliviertassinari reviewed Oct 27, 2020

View reviewed changes

packages/grid/_modules_/grid/models/api/coreApi.ts Outdated Show resolved Hide resolved

packages/grid/_modules_/grid/models/api/coreApi.ts Outdated Show resolved Hide resolved

packages/grid/_modules_/grid/hooks/utils/useScrollFn.ts Show resolved Hide resolved

oliviertassinari requested changes Oct 28, 2020

View reviewed changes

dtassone closed this Oct 28, 2020

oliviertassinari mentioned this pull request Oct 28, 2020

[DataGrid] Experiment without raf #507

Closed

dtassone reopened this Oct 28, 2020

dtassone added 5 commits October 29, 2020 13:57

add rafTimer

da3ab50

Conflicts: packages/grid/_modules_/grid/hooks/features/virtualization/useVirtualRows.ts packages/grid/_modules_/grid/hooks/utils/useScrollFn.ts

fix cyclic dep and cleanup

2ab5093

fix import

2cbd010

fix import

c7a21ac

trying to fix test in CI

e32f411

dtassone force-pushed the rafRefact branch from 2452678 to e32f411 Compare October 29, 2020 13:13

refactor scroll

ae06865

dtassone requested a review from oliviertassinari October 29, 2020 15:16

oliviertassinari reviewed Oct 29, 2020

View reviewed changes

dtassone and others added 11 commits October 29, 2020 16:35

Update packages/grid/_modules_/grid/hooks/utils/useRafUpdate.ts

e39f5f5

Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>

Update packages/grid/_modules_/grid/hooks/utils/useScrollFn.ts

60223fa

Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>

fix tests

ca2e885

Merge branch 'rafRefact' of github.com:dtassone/material-ui-x into ra…

0a91526

…fRefact

prettier

6bbcda7

fix test

a1bd19f

fix test

5f40991

fix tests

9242605

more sleep

1288aa6

Update packages/grid/_modules_/grid/models/api/coreApi.ts

fac1f07

Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>

Update packages/grid/_modules_/grid/models/api/coreApi.ts

3794072

Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>

small refactoring

6e80039

dtassone requested a review from oliviertassinari October 30, 2020 13:45

oliviertassinari approved these changes Oct 30, 2020

View reviewed changes

dtassone merged commit 1f542a2 into mui:master Oct 30, 2020

oliviertassinari mentioned this pull request Oct 31, 2020

[DataGrid] Apply review from #412 #515

Merged

zannager added the component: data grid This is the name of the generic UI component, not the React module! label Jan 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DataGrid] Raf Timer stored in apiRef #506

[DataGrid] Raf Timer stored in apiRef #506

dtassone commented Oct 27, 2020 •

edited

oliviertassinari left a comment •

edited

oliviertassinari commented Oct 27, 2020 •

edited

dtassone commented Oct 28, 2020

oliviertassinari commented Oct 28, 2020 •

edited

dtassone commented Oct 28, 2020

oliviertassinari left a comment

oliviertassinari Oct 29, 2020

dtassone Oct 29, 2020

oliviertassinari Oct 29, 2020 •

edited

oliviertassinari Oct 29, 2020

oliviertassinari Oct 29, 2020

dtassone Oct 30, 2020

oliviertassinari commented Oct 31, 2020

	expect(row!.classList.contains('Mui-selected')).to.equal(false, 'class mui-selected 2');
	expect(row!.classList.contains('Mui-selected')).to.equal(false);

[DataGrid] Raf Timer stored in apiRef #506

[DataGrid] Raf Timer stored in apiRef #506

Conversation

dtassone commented Oct 27, 2020 • edited

oliviertassinari left a comment • edited

Choose a reason for hiding this comment

oliviertassinari commented Oct 27, 2020 • edited

dtassone commented Oct 28, 2020

oliviertassinari commented Oct 28, 2020 • edited

dtassone commented Oct 28, 2020

oliviertassinari left a comment

Choose a reason for hiding this comment

oliviertassinari Oct 29, 2020

Choose a reason for hiding this comment

dtassone Oct 29, 2020

Choose a reason for hiding this comment

oliviertassinari Oct 29, 2020 • edited

Choose a reason for hiding this comment

oliviertassinari Oct 29, 2020

Choose a reason for hiding this comment

oliviertassinari Oct 29, 2020

Choose a reason for hiding this comment

dtassone Oct 30, 2020

Choose a reason for hiding this comment

oliviertassinari commented Oct 31, 2020

dtassone commented Oct 27, 2020 •

edited

oliviertassinari left a comment •

edited

oliviertassinari commented Oct 27, 2020 •

edited

oliviertassinari commented Oct 28, 2020 •

edited

oliviertassinari Oct 29, 2020 •

edited