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

Bump React #15255

Merged
merged 30 commits into from
Nov 13, 2024
Merged

Bump React #15255

merged 30 commits into from
Nov 13, 2024

Conversation

LukasTy
Copy link
Member

@LukasTy LukasTy commented Nov 4, 2024

Closes #14424.

Re-create #14424 with custom changes.
I can't force push the local branch version to upstream, creating a replica on my repo instead.

@LukasTy LukasTy added the dependencies Update of dependencies label Nov 4, 2024
@LukasTy LukasTy self-assigned this Nov 4, 2024
@mui-bot
Copy link

mui-bot commented Nov 4, 2024

Deploy preview: https://deploy-preview-15255--material-ui-x.netlify.app/

Generated by 🚫 dangerJS against 6a86fe4

}: {
syncState: (stateToSave: GridInitialState) => void;
}) {
declare module '@mui/x-data-grid' {
Copy link
Member Author

@LukasTy LukasTy Nov 4, 2024

Choose a reason for hiding this comment

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

If pro and premium package module augmentation are mixed in a single project—it goes haywire. 🙈

Copy link
Member Author

Choose a reason for hiding this comment

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

After some of this dogfooding—this is my main gripe with module augmentation. 🙈
If users extend the same types for different package versions—they come to experience unexpected behavior. 🤷

@LukasTy
Copy link
Member Author

LukasTy commented Nov 4, 2024

@mui/code-infra @flaviendelangle do you have any idea on how to resolve the TS problem?
The issue in a gist is as follows:

  • We extend the ToolbarPropsOverrides props using "define module" in some docs demos
  • The typescript run fails on the docs package, because
    {rootProps.slots.toolbar && <rootProps.slots.toolbar {...rootProps.slotProps?.toolbar} />}
    consumes that slot and all the leaking module augmentation complains about the missing required types. 🙈

@@ -64,7 +64,7 @@ export interface GridRowProps extends React.HTMLAttributes<HTMLDivElement> {
onDoubleClick?: React.MouseEventHandler<HTMLDivElement>;
onMouseEnter?: React.MouseEventHandler<HTMLDivElement>;
onMouseLeave?: React.MouseEventHandler<HTMLDivElement>;
[x: string]: any; // Allow custom attributes like data-* and aria-*
[x: `data-${string}`]: string;
Copy link
Member Author

Choose a reason for hiding this comment

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

@mui/xgrid Are you sure we need to do this?
What problem is it solving? 🤔
No other X package is doing it, and neither Core is doing it. 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

For context—no other package is providing this explicit type, if anyone needs to provide it, they are probably fighting TS as well.
Such props work on HTML and JSX elements as any prop with a - is ignored by TS.
Given that in our case these props are passed through slotProps, this causes slightly more problems, because they will show an error. 🤔

Comment on lines 37 to 38
[x: `data-${string}`]: string;
[x: `${string}Options`]: any;
Copy link
Member Author

Choose a reason for hiding this comment

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

WDYT about this approach?
Can we keep just the second line?
Or only excelOptions? 🤔

@LukasTy
Copy link
Member Author

LukasTy commented Nov 4, 2024

Compromise

If we want to unblock the bump and revert back to almost the current approach (casting everything) for demos using a custom toolbar here is a diff.
WDYT about it?

diff --git a/docs/data/data-grid/editing/FullFeaturedCrudGrid.tsx b/docs/data/data-grid/editing/FullFeaturedCrudGrid.tsx
index b74a768045..12cd6cc9be 100644
--- a/docs/data/data-grid/editing/FullFeaturedCrudGrid.tsx
+++ b/docs/data/data-grid/editing/FullFeaturedCrudGrid.tsx
@@ -18,6 +18,7 @@ import {
   GridRowId,
   GridRowModel,
   GridRowEditStopReasons,
+  GridSlots,
   GridSlotProps,
 } from '@mui/x-data-grid';
 import {
@@ -70,16 +71,14 @@ const initialRows: GridRowsProp = [
   },
 ];
 
-declare module '@mui/x-data-grid' {
-  interface ToolbarPropsOverrides {
-    setRows: (newRows: (oldRows: GridRowsProp) => GridRowsProp) => void;
-    setRowModesModel: (
-      newModel: (oldModel: GridRowModesModel) => GridRowModesModel,
-    ) => void;
-  }
+interface EditToolbarProps {
+  setRows: (newRows: (oldRows: GridRowsProp) => GridRowsProp) => void;
+  setRowModesModel: (
+    newModel: (oldModel: GridRowModesModel) => GridRowModesModel,
+  ) => void;
 }
 
-function EditToolbar(props: GridSlotProps['toolbar']) {
+function EditToolbar(props: EditToolbarProps) {
   const { setRows, setRowModesModel } = props;
 
   const handleClick = () => {
@@ -242,9 +241,9 @@ export default function FullFeaturedCrudGrid() {
         onRowModesModelChange={handleRowModesModelChange}
         onRowEditStop={handleRowEditStop}
         processRowUpdate={processRowUpdate}
-        slots={{ toolbar: EditToolbar }}
+        slots={{ toolbar: EditToolbar as unknown as GridSlots['toolbar'] }}
         slotProps={{
-          toolbar: { setRows, setRowModesModel },
+          toolbar: { setRows, setRowModesModel } as GridSlotProps['toolbar'],
         }}
       />
     </Box>
diff --git a/docs/data/data-grid/editing/StartEditButtonGrid.tsx b/docs/data/data-grid/editing/StartEditButtonGrid.tsx
index a1e7a86e69..45da690258 100644
--- a/docs/data/data-grid/editing/StartEditButtonGrid.tsx
+++ b/docs/data/data-grid/editing/StartEditButtonGrid.tsx
@@ -10,6 +10,7 @@ import {
   GridEventListener,
   GridCellModesModel,
   GridSlotProps,
+  GridSlots,
 } from '@mui/x-data-grid';
 import {
   randomCreatedDate,
@@ -22,16 +23,14 @@ interface SelectedCellParams {
   field: string;
 }
 
-declare module '@mui/x-data-grid' {
-  interface ToolbarPropsOverrides {
-    selectedCellParams: SelectedCellParams | null;
-    cellModesModel: GridCellModesModel;
-    setCellModesModel: (value: GridCellModesModel) => void;
-    cellMode: 'view' | 'edit';
-  }
+interface EditToolbarProps {
+  selectedCellParams?: SelectedCellParams;
+  cellModesModel: GridCellModesModel;
+  setCellModesModel: (value: GridCellModesModel) => void;
+  cellMode: 'view' | 'edit';
 }
 
-function EditToolbar(props: GridSlotProps['toolbar']) {
+function EditToolbar(props: EditToolbarProps) {
   const { selectedCellParams, cellMode, cellModesModel, setCellModesModel } = props;
 
   const handleSaveOrEdit = () => {
@@ -149,14 +148,14 @@ export default function StartEditButtonGrid() {
         cellModesModel={cellModesModel}
         onCellEditStop={handleCellEditStop}
         onCellModesModelChange={(model) => setCellModesModel(model)}
-        slots={{ toolbar: EditToolbar }}
+        slots={{ toolbar: EditToolbar as unknown as GridSlots['toolbar'] }}
         slotProps={{
           toolbar: {
             cellMode,
             selectedCellParams,
             cellModesModel,
             setCellModesModel,
-          },
+          } as GridSlotProps['toolbar'],
           cell: {
             onFocus: handleCellFocus,
           },
diff --git a/docs/data/data-grid/filtering/CustomFilterPanelPosition.tsx b/docs/data/data-grid/filtering/CustomFilterPanelPosition.tsx
index 6dfc79cb74..d300354a99 100644
--- a/docs/data/data-grid/filtering/CustomFilterPanelPosition.tsx
+++ b/docs/data/data-grid/filtering/CustomFilterPanelPosition.tsx
@@ -2,6 +2,7 @@ import * as React from 'react';
 import {
   DataGrid,
   GridSlotProps,
+  GridSlots,
   GridToolbarContainer,
   GridToolbarFilterButton,
 } from '@mui/x-data-grid';
@@ -9,15 +10,11 @@ import { useDemoData } from '@mui/x-data-grid-generator';
 
 const VISIBLE_FIELDS = ['name', 'rating', 'country', 'dateCreated', 'isAdmin'];
 
-declare module '@mui/x-data-grid' {
-  interface ToolbarPropsOverrides {
-    setFilterButtonEl: React.Dispatch<
-      React.SetStateAction<HTMLButtonElement | null>
-    >;
-  }
+interface CustomToolbarProps {
+  setFilterButtonEl: React.Dispatch<React.SetStateAction<HTMLButtonElement | null>>;
 }
 
-function CustomToolbar({ setFilterButtonEl }: GridSlotProps['toolbar']) {
+function CustomToolbar({ setFilterButtonEl }: CustomToolbarProps) {
   return (
     <GridToolbarContainer>
       <GridToolbarFilterButton ref={setFilterButtonEl} />
@@ -39,12 +36,12 @@ export default function CustomFilterPanelPosition() {
     <div style={{ height: 400, width: '100%' }}>
       <DataGrid
         {...data}
-        slots={{ toolbar: CustomToolbar }}
+        slots={{ toolbar: CustomToolbar as unknown as GridSlots['toolbar'] }}
         slotProps={{
           panel: {
             anchorEl: filterButtonEl,
           },
-          toolbar: { setFilterButtonEl },
+          toolbar: { setFilterButtonEl } as GridSlotProps['toolbar'],
         }}
       />
     </div>
diff --git a/docs/data/data-grid/list-view/ListViewAdvanced.tsx b/docs/data/data-grid/list-view/ListViewAdvanced.tsx
index c53a525c0a..a5263f36dc 100644
--- a/docs/data/data-grid/list-view/ListViewAdvanced.tsx
+++ b/docs/data/data-grid/list-view/ListViewAdvanced.tsx
@@ -8,6 +8,7 @@ import {
   GridRowId,
   gridClasses,
   GridRowModel,
+  GridSlotProps,
 } from '@mui/x-data-grid-premium';
 import EditIcon from '@mui/icons-material/Edit';
 import DeleteIcon from '@mui/icons-material/Delete';
@@ -28,15 +29,6 @@ import { formatDate, formatSize, stringAvatar } from './utils';
 import { ActionDrawer } from './components/ActionDrawer';
 import { RenameDialog } from './components/RenameDialog';
 
-declare module '@mui/x-data-grid' {
-  interface ToolbarPropsOverrides {
-    listView: boolean;
-    container: () => HTMLElement;
-    handleDelete: (ids: GridRowId[]) => void;
-    handleUpload: (event: React.ChangeEvent<HTMLInputElement>) => void;
-  }
-}
-
 export default function ListViewAdvanced() {
   // This is used only for the example - renders the drawer inside the container
   const containerRef = React.useRef<HTMLDivElement>(null);
@@ -298,7 +290,7 @@ export default function ListViewAdvanced() {
               container,
               handleDelete,
               handleUpload,
-            },
+            } as GridSlotProps['toolbar'],
             loadingOverlay: {
               variant: 'linear-progress',
             },
diff --git a/docs/data/data-grid/state/RestoreStateInitialState.tsx b/docs/data/data-grid/state/RestoreStateInitialState.tsx
index 5e50289ac8..359750d904 100644
--- a/docs/data/data-grid/state/RestoreStateInitialState.tsx
+++ b/docs/data/data-grid/state/RestoreStateInitialState.tsx
@@ -6,6 +6,7 @@ import {
   DataGridPro,
   GridInitialState,
   GridSlotProps,
+  GridSlots,
   GridToolbarContainer,
   GridToolbarDensitySelector,
   GridToolbarFilterButton,
@@ -14,13 +15,11 @@ import {
 } from '@mui/x-data-grid-pro';
 import { useDemoData } from '@mui/x-data-grid-generator';
 
-declare module '@mui/x-data-grid' {
-  interface ToolbarPropsOverrides {
-    syncState: (stateToSave: GridInitialState) => void;
-  }
+interface CustomToolbarProps {
+  syncState: (stateToSave: GridInitialState) => void;
 }
 
-function GridCustomToolbar({ syncState }: GridSlotProps['toolbar']) {
+function GridCustomToolbar({ syncState }: CustomToolbarProps) {
   const rootProps = useGridRootProps();
   const apiRef = useGridApiContext();
 
@@ -65,8 +64,8 @@ export default function RestoreStateInitialState() {
         <DataGridPro
           {...data}
           loading={loading}
-          slots={{ toolbar: GridCustomToolbar }}
-          slotProps={{ toolbar: { syncState } }}
+          slots={{ toolbar: GridCustomToolbar as unknown as GridSlots['toolbar'] }}
+          slotProps={{ toolbar: { syncState } as GridSlotProps['toolbar'] }}
         />
       </Box>
       <Box sx={{ height: 300 }}>

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 6, 2024
Copy link

github-actions bot commented Nov 6, 2024

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

@flaviendelangle
Copy link
Member

I don't have a great solution 😬

The more I think about it, the more I like @michaldudak proposal of migrating away from the slots pattern for big pieces of UI in favor of something more like:

<DataGrid rows={rows} columns={columns}>
  <DataGrid.UIParts.Toolbar>
    {toolbarProps => (
      <GridCustomToolbar {...toolbarProps} syncState={syncState} />
    )}
  </DataGrid.Slots.Toolbar>
</DataGrid>

The exact DX can of course vary:

<DataGrid rows={rows} columns={columns}>
  <DataGrid.UIParts.Toolbar
    render={toolbarProps => (
      <GridCustomToolbar {...toolbarProps} syncState={syncState} />
    )}
  />
</DataGrid>

or

<DataGrid rows={rows} columns={columns}>
  <DataGrid.Toolbar
    render={toolbarProps => (
      <GridCustomToolbar {...toolbarProps} syncState={syncState} />
    )}
  />
</DataGrid>

or

<DataGrid
  rows={rows} 
  columns={columns}>
  uiParts={{
    toolbar: toolbarProps => (
      <GridCustomToolbar {...toolbarProps} syncState={syncState} />
    )
  }}
/>

etc...

The idea being that those big UI parts have very different trade-offs than what slots have been designed for.
And the slot / slotProps split is a good example with all the TS problems it causes.

Big pieces of UI, I'm refering to things like filterPanel / toolbar / field (on the picker) / calendarHeader (on the picker) etc... where the current slots approach brings a lot of issue that the core does not really have since for them slots are almost always 1 DOM element with little custom props.

Not saying that we should change it tomorrow and maybe an "ugly fix" is enough for now, but we should spend some time working on https://www.notion.so/mui-org/Element-override-patterns-121cbfe7b66080efa637e41a1063343f#121cbfe7b66080299076de3e6ac03056 at some point to improve the customization experience around those topics.

@LukasTy LukasTy mentioned this pull request Nov 11, 2024
1 task
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 11, 2024
@LukasTy
Copy link
Member Author

LukasTy commented Nov 11, 2024

I don't have a great solution 😬

I fully agree with your comments.
It's pretty clear that the more we try to make slots work for X components, the more we see that this concept is not fit for it. 🤔

@LukasTy LukasTy requested review from cherniavskii and a team November 11, 2024 10:18
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 11, 2024
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 Nov 12, 2024
{rootProps.slots.toolbar && (
<rootProps.slots.toolbar
// Fixes error augmentation issue https://github.com/mui/mui-x/pull/15255#issuecomment-2454721612
{...(rootProps.slotProps?.toolbar as any)}
Copy link
Member

Choose a reason for hiding this comment

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

I did this to make the types work, I think it's an acceptable solution as it's internal only.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm fine if you are happy with this solution. 👌
The component is public, but do you mean that if someone wants to override this slot, they will create their component?

Copy link
Member

Choose a reason for hiding this comment

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

No, I meant that as any above doesn't impact the users passing the slot props

@LukasTy LukasTy merged commit 284d0e5 into mui:master Nov 13, 2024
18 checks passed
@LukasTy LukasTy deleted the renovate/react branch November 13, 2024 07:12
@cherniavskii
Copy link
Member

BTW, shouldn't this PR be cherry-picked to v7.x?

@LukasTy
Copy link
Member Author

LukasTy commented Nov 13, 2024

BTW, shouldn't this PR be cherry-picked to v7.x?

That's a good question. 🤔
Do we intend to bring support for React 19 to v7? 🤔
cc @joserodolfofreitas @DanailH @flaviendelangle

@flaviendelangle
Copy link
Member

@joserodolfofreitas is in favor of supporting it yes

cherniavskii added a commit to cherniavskii/mui-x that referenced this pull request Nov 13, 2024
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Andrew Cherniavskyi <[email protected]>
@cherniavskii cherniavskii added the needs cherry-pick The PR should be cherry-picked to master after merge label Nov 13, 2024
cherniavskii added a commit that referenced this pull request Nov 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Update of dependencies needs cherry-pick The PR should be cherry-picked to master after merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants