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 internal prop management to prepare for bundle split #3487

Merged
merged 19 commits into from
Jan 5, 2022

Conversation

flaviendelangle
Copy link
Member

@flaviendelangle flaviendelangle commented Dec 21, 2021

Introduction

I started working on #924, but our current prop interfaces are not suited for this split.
All our code, even the one used on the Community Plan uses GridComponentProps which includes all the pro-only props.
So if we split the bundle without changing this approach, we would need to set all the pro-types inside x-data-grid (GridPinnedColumns, GridGroupingColDefOverride, etc...).

The new approach allows several things:

  • Don't have to set forced props on DataGrid for props which are only used on pro-only hooks. Even if the user sets treeData: true, he won't have the Tree Data since it's not even bundled => Done in this PR

  • Define different interfaces for some important props (initialState / apiRef / colDef for instance) => Not done in this PR

Note that this PR is a 1st step, it has some dirty imports / exports because I still have the _modules_ folder.

Current behavior

We have a single interface GridComponentProps used in our entire internal codebase.

Prop management before

New behavior

Have two interfaces in our internal codebase

  • DataGridProcessedProps: used on any code that will be bundled in the x-data-grid package
  • DataGridProProcessedProps: used on any code that will only be bundled in x-data-grid-pro package

Prop management after

Props removed from DataGrid

I removed onColumnResize and onColumnWidthChange props from DataGrid because they are only fired in useGridColumnResize so they were never actually used on DataGrid.
It is not a breaking change, people can still pass them, the only error will be a typing one.

Whats next

  1. Split GridApi, GridState and GridInitialState into a pro and a community interface

I did a MVP locally

  1. Move the pro-only code into packages/grid/x-data-grid-pro

At this point we would be sure that the Community Plan do not bundle anything that is in this folder (

  1. Move the community code into packages/grid/x-data-grid

A major thing to clarify here is how to use non-exported elements from the x-data-grid package inside the x-data-grid-pro.
For instance how to use all the feature hooks (useGridFilter, ...), the internal types (GridColumnRawLookup, ...), the default options (DATA_GRID_DEFAULT_SIMPLE_OPTIONS) etc...

We can import directly from the other package folder but it's not very clean

// The exact path is just for the example and does not match anything existing today
import { DATA_GRID_DEFAULT_SIMPLE_OPTIONS } from '../../../x-data-grid/src/props/dataGridDefaultSimpleOptions`

const DATA_GRID_PRO_DEFAULT_SIMPLE_OPTIONS = {
  ...DATA_GRID_DEFAULT_SIMPLE_OPTIONS,
  treeData: false,
  defaultGroupExpansionDepth: 0,
};

@flaviendelangle flaviendelangle self-assigned this Dec 21, 2021
@flaviendelangle flaviendelangle changed the title Prop plan [core] Rework internal prop management to prepare for bundle split Dec 21, 2021
@flaviendelangle flaviendelangle added core Infrastructure work going on behind the scenes typescript labels Dec 21, 2021
@@ -244,20 +518,6 @@ interface GridComponentOtherProps extends CommonProps {
* @param {GridCallbackDetails} details Additional details for this callback.
*/
onColumnOrderChange?: GridEventListener<GridEvents.columnOrderChange>;
Copy link
Member

Choose a reason for hiding this comment

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

Should this also be moved to DataGridProProps.ts? Users can only reorder columns in DataGridPro.

Copy link
Member Author

Choose a reason for hiding this comment

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

The event is fired by setColumnIndex which is in useGridColumns
So technically, users can create a toolbar and use apiRef.current.setColumnIndex in it.
But we said we wanted to move it to useGridColumnReorder.
If we confirm that we'll do it, I can move onColumnOrderChange to the pro interface.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, they can but if they call in a toolbar while using DataGrid it won't work correctly because only DataGridPro listens for this event.

https://github.com/mui-org/material-ui-x/blob/6208886dbb143f7365f8387ef9d91a6261a3a9d3/packages/grid/x-data-grid-pro/src/DataGridProVirtualScroller.tsx#L159

apiRef.current.setColumnIndex could be understood as part of the column reorder API, so it wouldn't be available in DataGrid.

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed
In that case I propose to move the code for setColumnIndex to useGridColumnReorder to clarify this buggy scenario and clean the typing

Copy link
Member

Choose a reason for hiding this comment

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

It can be done later.

@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 Dec 23, 2021
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 3, 2022
@github-actions
Copy link

github-actions bot commented Jan 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 Jan 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 Jan 3, 2022
@flaviendelangle flaviendelangle merged commit a3d4a99 into mui:master Jan 5, 2022
@flaviendelangle flaviendelangle deleted the prop-plan branch January 5, 2022 10:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Infrastructure work going on behind the scenes typescript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants