[RFC] Better isolate DataGridPro from DataGrid

oliviertassinari commented 3 years ago

Summary 💡

The two components should be better isolated. XGrid should be able to grow in complexity and package size without harming the bundle size of DataGrid. In the current configuration, both packages have the exact same size

Motivation 🔦

DataGrid is in competition in the space of a similar open-source data grid. The bundle size is one of the dimensions developers look at. I think that the biggest decision influence happens when one package looks disproportionally bigger than an equivalent solution. So this issue might be the one that has the most influence on this matter. I'm not sure that we will need to the same depth as we did in the main repository.

If you look at the bundle size of ag-Grid, you can see that it can quickly get out of hand: ag-grid-community: 193 kB gzipped. They have tried to isolate features, but the core seems to be about the same size: @ag-grid-community/core: 191 kB gzipped (it's only with the enterprise features that it seems to make a difference).
The license should be better isolated. It's not OK to have all the content of /packages/grid/_modules_ exported under an MIT licensed package.
This effort should allow us to make the grid, and its feature more composable. It would be great to invest time in a plugin system. Some prior-arts: https://datatables.net/manual/plug-ins/.

oliviertassinari commented 3 years ago

@dtassone Has made a quick-win proposal to initiate this direction. We could create two GridComponent

https://github.com/mui-org/material-ui-x/blob/a0b719281f82460d0e7f829af40add2538c0e46e/packages/grid/_modules_/grid/GridComponent.tsx#L48

This won't solve the problem but put us in a better place. It will force us to start handling the matter as we build new features, in order to identify ways to reduce the duplication.

oliviertassinari commented 3 years ago

@mbrookes How important do you think this is? Would it make sense to do it post v5.0.0 stable?

I would personally advocate doing it before the Premium features, regardless of the stable nature. For the Pro features, IMHO, the main value is about forcing us to isolate features (tree shaking, code isolation, etc.)

mbrookes commented 3 years ago

It's important, but it can wait if there are other priorities.

oliviertassinari commented 3 years ago

Regarding sharing code, we very likely want to have @material-ui/x-grid depending on @material-ui/data-grid. It would have the following advantages:

Progressive migration. People adding XGrid in a few cases don't pay the bundle size overhead twice.
Social proof. People using the premium version contributes to the downloads of the community version.
Code structure. We would be able to kill the grid/_module_ structure. Instead, we can default to the approach we use in the core components, share the same bundling approach.
Customers purchasing the Pro plan and not renewing @material-ui/x-grid might still want to update @material-ui/data-grid to benefit from the latest MIT additions (if in scope).

flaviendelangle commented 3 years ago

I think it will be hard to remove 100% of the XGrid-only code from the DataGrid bundle. Especially when this code is in the middle of a more generic hook / component that is also used by the DataGrid.

For instance everything related to those forced props :+1:

  disableMultipleColumnsFiltering?: true;
  disableMultipleColumnsSorting?: true;
  disableMultipleSelection?: true;

But if we agree that the goal is to isolate as much as possible, then I agree that a structure like you describe would be better.

Maybe some advanced hooks like sorting / filtering will diverge between DataGrid and XGrid and justify having two distincts useGridSorting, one in DataGrid and one in XGrid importing some common utils methods from the one in DataGrid. But I don't think it should be done only for bundle separation reasons, as it increase the codebase complexity.

Some non trivial examples of code separation:

Selectors from XGrid used in a DataGrid component

The component GridColumnHeadersItemCollection (that is used in DataGrid) imports the selector gridColumnReorderDragColSelector from the useGridColumnsReorder folder. Which is an XGrid-only feature and should be in the @material-ui/x-grid folder in the end.

State initialization methods

Right now, the state from DataGrid also have the XGrid only default values which are defined in XGrid-only files (getInitialGridColumnReorderState for instance).

For the types, if we keep a shared models folder then the issue won't happen. If we want to split the models folder into data-grid/src/models and x-grid/src/models then the same problem will occur.

But in both cases, having all the types in the models folder(s) will help avoid cases where we import types of XGrid in the DataGrid module from a XGrid file that is not type-only.

oliviertassinari commented 3 years ago

@flaviendelangle Agree, I think that we could work iteratively on this problem, focus where a separation/modularity would make the most sense. Thoughts on the possible solutions:

The component GridColumnHeadersItemCollection (that is used in DataGrid) imports the selector gridColumnReorderDragColSelector

We could categories this as a leaky abstraction. The logic could have very well been reverse. The column reorder hook could use the componentsProps prop to inject props on the slots it needs to access, instead of the slot pulling in values from the hook.

State initialization methods

We could move the initial state to the hook level, instead of having a centralized place.

For the types, if we keep a shared models folder

We could use module augmentation for the shared data structures. For instance: https://github.com/mui-org/material-ui/blob/40d9587f653a6e431f5ff0e3a2ca6cb0a3b8aba7/packages/material-ui-lab/src/themeAugmentation/props.d.ts#L67.

flaviendelangle commented 3 years ago

I moved the content of this message to #2293

flaviendelangle commented 3 years ago

We could categories this as a leaky abstraction. The logic could have very well been reverse. The column reorder hook could use the componentsProps prop to inject props on the slots it needs to access, instead of the slot pulling in values from the hook.

I propose we start by reducing as much import from the XGrid part to the DataGrid part And then see how much of those scenarios we find. Because I agree that we may be able to fix all of them by changing the logic.

We could use module augmentation for the shared data structures. For instance: https://github.com/mui-org/material-ui/blob/40d9587f653a6e431f5ff0e3a2ca6cb0a3b8aba7/packages/material-ui-lab/src/themeAugmentation/props.d.ts#L67.

I haven't use module augmentation enough. Would it allow us to have the DataGrid state typing on the DataGrid folder in our codebase and the XGrid state typing on the XGrid folder ?

If we start to implement XGrid-only feature hooks with their own states, then we may want to differentiate the typing of GridState between the two. This would help us avoid relying on the presence of some state that is not always there.

oliviertassinari commented 3 years ago

Would it allow us to have the DataGrid state typing on the DataGrid folder in our codebase and the XGrid state typing on the XGrid folder?

@flaviendelangle Module augmentation is global to a TypeScript project. We would need to treat the two codebases as different TypeScript projects. It might be overkill.

I think it tightly related to the prop-to-state pattern (#1562 (comment)) and we may want to first take a deeper look at the approach we want to have for the update part.

Yes, agree. I think that we have a more important issue with the state to fix before considering the modularity.

If we start to implement XGrid-only feature hooks with their own states, then we may want to differentiate the typing of GridState between the two.

Regarding the definition of the done on this issue. IMHO, these two should be enough:

the ability to implement an XGrid-only feature
not releasing commercial code under the MIT license

Extra level of modularity should probably be driven by the a. bundle-size needs and b. customizability needs. I have open this issue primarily for the license, knowing that it would also help on more dimensions.

aderchox commented 2 years ago

Not directly relevant to this issue, but I just wanted to also suggest better isolating them in the documentations. Currently as you read the docs, there's just a blue cube near each title which indicates that that feature belongs to the pro version, however, some of these features are vague and seem to have overlaps with the free features. For example consider this one: Control with external buttons. When you check the source code, some JSX is wrapped in DataGrid, and some in DataGridPro, and yet it's not quite clear for a beginner like me which exact parts are pro and which are free. What if I create my own toolbar and add some buttons to edit the data, won't the datagrid update to those edits accordingly? Of course it does I guess, but that title is weird and vague to me. So I may decide to think better of using MUI's DataGrid free completely only because I can't have its docs separately.

flaviendelangle commented 2 years ago

Pinging @joserodolfofreitas

mui / mui-x