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

[DataGrid] Differenciate the pro and community versions of GridState, GridApi and GridApiRef #3648

Merged
merged 73 commits into from
Feb 14, 2022

Conversation

flaviendelangle
Copy link
Member

@flaviendelangle flaviendelangle commented Jan 17, 2022

Part of #924

Goal

To reduce the amount of imports from file that should be moved to the x-data-grid package to files that should be moved to the x-data-grid-pro package.

New behavior

Interface based on the package

If you import GridApi, GridState or GridInitialState from @mui/x-data-grid, you will have the community version.
If you import GridApi, GridState or GridInitialState from @mui/x-data-grid-pro, you will have the pro version.

Internally we use GridApiPro and GridApiCommunity while waiting for the bundle split.
Afterwards, we should be able to simplify things.

For all the interfaces depending on the api (for instance GridRenderCellParams to type its api param or GridColDef to type its renderCell params through GridRenderCellParams), I added an Api generic which extends GridApiCommon.
When exported from @mui/x-data-grid, the default generic value is the community api, when exported from @mui/x-data-grid-pro, the default generic value is the pro api.

Untyped interfaces

Some use case were to complicated to cover for now.
For instance, the columns prop of DataGrid has the type GridColumns. Otherwise, you could have errors when passing pro columns to the community version because TypeScript consider that the pro and community api are not compatible.

createSelector

The method now ask as a parameter the largest state used in one of its sub-selectors.

For instance the following selector will ask for GridApiPro because gridRowGroupingModelSelector is a pro selector.

const mySelector = createSelector(gridPageSizeSelector, gridRowGroupingModelSelector, () => null)

Note: Everything still works with the apiRef, I just simplified the typing to accept a ref which extends { instanceId: number, state: S }

What's next ?

In v6 we should drop all the api parameter to reduce as much as possible the scope of the plan-based typing.
This requires to open the apiRef to the community plan (which should be fairly easy after this PR).

@flaviendelangle flaviendelangle self-assigned this Jan 17, 2022
@mui-bot
Copy link

mui-bot commented Jan 17, 2022

These are the results for the performance tests:

Test case Unit Min Max Median Mean σ
Filter 100k rows ms 226.6 450.7 309.1 327.62 92.878
Sort 100k rows ms 386.3 796.2 616.2 610.14 145.205
Select 100k rows ms 118.3 266.4 195.2 191.78 49.049
Deselect 100k rows ms 112.1 244.9 163.2 166.46 47.103

Generated by 🚫 dangerJS against 694bfdc

@flaviendelangle flaviendelangle added component: data grid This is the name of the generic UI component, not the React module! typescript labels Jan 17, 2022
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 18, 2022
@github-actions
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 18, 2022
@github-actions
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 19, 2022
@flaviendelangle
Copy link
Member Author

flaviendelangle commented Feb 2, 2022

@m4theushw I have an implementation passing all the CI steps.
It is very verbose and I would be in favor of removing the api param whenever possible in v6 to remove the amount of interfaces and method impacted by the plan.
TS does not like when you pass a "community" interface (the GridCellParams of @mui/x-data-grid-pro) to a method which expects the same interface but in it's "pro" version. The main problem being apiRef.current.setState which is not compatible between the plan according to TS and I can't find a solution.

There are some cleaning to do on the doc generation, but I want to do the bundle split before because half of my work is going to be dropped after the split.

@github-actions
Copy link

github-actions bot commented Feb 3, 2022

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 3, 2022
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 3, 2022
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 8, 2022
@github-actions
Copy link

github-actions bot commented Feb 8, 2022

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@siriwatknp
Copy link
Member

siriwatknp commented Feb 8, 2022

👋 The migration PR has been merged.

Please follow these steps to make sure that the contents are all updated. (Sorry for the inconvenience)

  1. pull latest master from upstream to your branch
  2. if your PR has changes on the *.md or demo files, you are likely to encounter conflict because all of them have been moved to the new folder.
    2.1 revert the change on those markdown files you did
    2.2 pull latest master from upstream (you should not see conflict)
    2.3 make the changes again at docs/data/data-grid/*
  3. run yarn docs:api
    • you might see the changes in docs/pages/x/api/data-grid/* if your PR touches some of the components
    • it is okay if there is no changes

If you are struggle with the steps above, feel free to tag @siriwatknp

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 8, 2022
@github-actions
Copy link

github-actions bot commented Feb 8, 2022

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 8, 2022
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 9, 2022
@flaviendelangle
Copy link
Member Author

I am merging this one to be able to move on without blocking the releases for to long.

@flaviendelangle flaviendelangle merged commit 3afcfeb into mui:master Feb 14, 2022
@flaviendelangle flaviendelangle deleted the state-pro-community branch November 18, 2022 07:05
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! typescript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants