Skip to content
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

[core] Rework useGridRows high frequency update #2561

Merged
merged 27 commits into from
Sep 27, 2021

Conversation

flaviendelangle
Copy link
Member

@flaviendelangle flaviendelangle commented Sep 7, 2021

Preview: https://deploy-preview-2561--material-ui-x.netlify.app/components/data-grid/rows/#high-frequency

  • Remove the systematic 100ms delay before update and replace it with a throttleRowsMs option
  • Remove GridEvents.rowsClear and GridEvents.rowsUpdate
  • Rename InternalGridRowsState => GridRowsState for consistency
  • Make current tests pass
  • Add richer test around the throttling
  • Update the docs

New option props.throttleRowsMs

I think it's better to let the user choose if he wants to throttle or not.
Today, every Grid has a 100ms delay on update even if it's not useful in most cases.
I went for a throttle instead of a debounce to avoid scenarios when we never have enough time between two updates so we never update the state.

Consequences on the events

Before, we were updating the state when receiving an update but we weren't publishing the event right away.
I think we should avoid that.
Now, the state is only updated after the throttle period if any. And the event is published at the same time.
By default (and in the large majority of cases), their is no throttle so the update of the rows is as synchronous as any other update.

@flaviendelangle flaviendelangle self-assigned this Sep 7, 2021
@flaviendelangle flaviendelangle added the component: data grid This is the name of the generic UI component, not the React module! label Sep 8, 2021
@flaviendelangle flaviendelangle assigned DanailH and unassigned DanailH Sep 8, 2021
@flaviendelangle flaviendelangle marked this pull request as ready for review September 8, 2021 13:31
@DanailH
Copy link
Member

DanailH commented Sep 9, 2021

props.throttleRowsMs sounds to me like a good candidate for being a Pro feature (it already kind of is since you use it in combination with the apiRef but if we are to expose the apiRef then that won't be the case - based on 3. from this #2522 (comment)). I think we should make it only available in DataGridPro.

@flaviendelangle
Copy link
Member Author

flaviendelangle commented Sep 9, 2021

As I built it, the throttleRowsMs also works for frequent props.rows update so it is usable in the free version.
But we can decide to limit it to the paid version.

@oliviertassinari , @m4theushw what is your view on that topic ?
Note that it's a very simple prop to switch back to the free version if we change our mind later.

@@ -266,4 +272,5 @@ export const GRID_DEFAULT_SIMPLE_OPTIONS: GridSimpleOptions = {
showColumnRightBorder: false,
sortingOrder: ['asc' as const, 'desc' as const, null],
sortingMode: GridFeatureModeConstant.client,
throttleRowsMs: 0,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we keep the 100ms as default? AG-Grid uses 50ms: https://ag-grid.com/javascript-data-grid/data-update-high-frequency/#flush-async-transactions

If we change the default value, we need to update the useEffect to run immediately when the prop changes. I think throttleRowsMs is only for setRows and updateRows.

Copy link
Member Author

@flaviendelangle flaviendelangle Sep 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My idea was to let the default value at 0 and if the user defines a throttle value, then it applies even on rows update.
But if you prefer to have a default value other than 0, then we should probably not run throttle the props update.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's keep the default as 0. I remembered that we don't have a separate method for throttled updates, so having a non-zero default will affect updateRows too.

With this PR, it should be possible to remove the following setTimeout:

https://github.com/mui-org/material-ui-x/blob/67864f8077b842eedcfc9f4f6765b055efa09988/docs/src/pages/components/data-grid/editing/FullFeaturedCrudGrid.tsx#L91-L92

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We just need to wait for the next render in order to focus

  const handleClick = () => {
    const id = randomId();
    apiRef.current.updateRows([{ id, isNew: true }]);
    apiRef.current.setRowMode(id, 'edit');
    setTimeout(() => {
      apiRef.current.scrollToIndexes({
        rowIndex: apiRef.current.getRowsCount() - 1,
      });
      apiRef.current.setCellFocus(id, 'name');
    })
  };
Screencast.2021-09-13.14.49.58.mp4

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could think of a better API to do so later on

packages/grid/x-grid/src/tests/rows.DataGridPro.test.tsx Outdated Show resolved Hide resolved
docs/src/pages/components/data-grid/rows/rows.md Outdated Show resolved Hide resolved
docs/src/pages/components/data-grid/rows/rows.md Outdated Show resolved Hide resolved
docs/src/pages/components/data-grid/rows/rows.md Outdated Show resolved Hide resolved
@m4theushw
Copy link
Member

As I built it, the throttleRowsMs also works for frequent props.rows update so it is usable in the free version.
But we can decide to limit it to the paid version.

I would keep it only for the API calls. Users can throttle the prop update in their applications. Once I have a value for throttleRowsMs and I want to immediately change the rows I can't, unless setting throttleRowsMs to zero before.

@flaviendelangle flaviendelangle added this to the Sprint 22 milestone Sep 14, 2021
@flaviendelangle flaviendelangle mentioned this pull request Sep 16, 2021
@flaviendelangle
Copy link
Member Author

@mui-org/x can we merge this PR once the release is done ?

@flaviendelangle flaviendelangle merged commit 3322265 into mui:next Sep 27, 2021
@@ -218,6 +218,12 @@ export interface GridSimpleOptions {
* @default "client"
*/
sortingMode: GridFeatureMode;
/**
* If positive, the Grid will throttle updates coming from `apiRef.current.updateRows` and `apiRef.current.setRows`.
* It can be useful if you have a high update rate but do not want to do heavy work like filtering / sorting or rendering on each individual update.
Copy link
Member

@oliviertassinari oliviertassinari Oct 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK, no spaces when using a slash, unless it's separating multiple words.

Suggested change
* It can be useful if you have a high update rate but do not want to do heavy work like filtering / sorting or rendering on each individual update.
* It can be useful if you have a high update rate but do not want to do heavy work like filtering/sorting or rendering on each individual update.

@flaviendelangle flaviendelangle deleted the rows-throttle branch October 9, 2021 07:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: data grid This is the name of the generic UI component, not the React module! performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants