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

[data grid] Improve typing for useGridApiRef #16000

Open
pcorpet opened this issue Dec 25, 2024 · 15 comments
Open

[data grid] Improve typing for useGridApiRef #16000

pcorpet opened this issue Dec 25, 2024 · 15 comments
Labels
breaking change bug 🐛 Something doesn't work component: data grid This is the name of the generic UI component, not the React module! support: premium standard Support request from a Premium standard plan user. https://mui.com/legal/technical-support-sla/ typescript

Comments

@pcorpet
Copy link
Contributor

pcorpet commented Dec 25, 2024

Steps to reproduce

Steps:

  1. Create a component with useGridApiRef
  2. use the created apiRef in a <DataGrid in the same component
  3. use apiRef in a useEffect in the same component
  4. update the component so that you can unmount the DataGrid with a state.

Current behavior

in 3. the useEffect receives a apiRef.current which is actually {} so does not have all the functions. However the type says that it has them.

In 4. the useEffect receives a apiRef.current which is null.

Expected behavior

I suggest we fix the type of useGridApiRef to mention those possibilities and avoid breaks at runtime. So either

  • change the default value setup in useGridApiRef to also have functions like the real api, but without any effect (as the DataGrid is not mounted yet)
  • also change the unmounted state so that api is still an object that supports the functions.
    OR
  • update the type of useDataGridRef (and the property of apiRef in DataGrid) to support something closer to the current reality.

Context

Note that we're trying to use some eslint rules that raise whenever we check for nullability of a variable that types say cannot be null.

Your environment

npx @mui/envinfo

Using Chrome, although the typing error is happening with TypeScript, not the browser.

  System:
    OS: macOS 15.2
  Binaries:
    Node: 22.9.0 - /opt/homebrew/bin/node
    npm: 10.8.3 - /opt/homebrew/bin/npm
    pnpm: 9.3.0 - /opt/homebrew/bin/pnpm
  Browsers:
    Chrome: 131.0.6778.205
    Edge: Not Found
    Safari: 18.2

Search keywords: datagrid types useGridApiRef hook

Order ID: 102731

@pcorpet pcorpet added bug 🐛 Something doesn't work status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Dec 25, 2024
@github-actions github-actions bot changed the title [Data Grid][Typing] Better typing for useGridApiRef [data grid][Typing] Better typing for useGridApiRef Dec 25, 2024
@github-actions github-actions bot added component: data grid This is the name of the generic UI component, not the React module! support: premium standard Support request from a Premium standard plan user. https://mui.com/legal/technical-support-sla/ labels Dec 25, 2024
@michelengelen
Copy link
Member

Thanks for raising this @pcorpet ... I'll add this to the board for the team to pick it up.
I went ahead and created a minimal code example:

import * as React from 'react';
import { DataGrid, useGridApiRef } from '@mui/x-data-grid';
import { useDemoData } from '@mui/x-data-grid-generator';
import { Button } from '@mui/material';

export default function PaginationCommunityNoSnap() {
  const [mounted, setMounted] = React.useState(true);
  const apiRef = useGridApiRef();
  const { data, loading } = useDemoData({
    dataSet: 'Commodity',
    rowLength: 500,
    maxColumns: 6,
  });

  React.useEffect(() => {
    console.log(apiRef.current);
  }, [apiRef]);

  return (
    <div style={{ height: 300, width: '100%' }}>
      <Button onClick={() => setMounted(!mounted)} sx={{ my: 3 }} fullWidth>
        Toggle Data Grid Mount
      </Button>
      {mounted ? <DataGrid {...data} apiRef={apiRef} loading={loading} /> : null}
    </div>
  );
}

This did raise another issue when remounting. apiRef is null for a brief moment during remount and throws an error in the ForwardRef(DataGrid) component.

cc @arminmeh

@michelengelen michelengelen added typescript and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Dec 27, 2024
@michelengelen michelengelen changed the title [data grid][Typing] Better typing for useGridApiRef [data grid] Improve typing for useGridApiRef Dec 27, 2024
@github-project-automation github-project-automation bot moved this to 🆕 Needs refinement in MUI X Data Grid Dec 27, 2024
@MBilalShafi
Copy link
Member

MBilalShafi commented Dec 27, 2024

@pcorpet Which version of the Data Grid are you using. It seems the useEffect one isn't there anymore since v7. Could you confirm?

Regarding the one with apiRef.current being null on unmount, we have another, pretty old issue (#6879) talking about the same thing in even more detail.
I'd side with the @romgrk's idea of adding null to the typing of the apiRef returned by useGridApiRef as it's closer to the actual situation.
The con about having to specifically checking it being not-null seems justified in this case.

Wdyt @mui/xgrid ?

P.S. Ideally, we'd continue the discussion in #6879 and close this one.

@MBilalShafi MBilalShafi added the status: waiting for author Issue with insufficient information label Dec 27, 2024
@pcorpet
Copy link
Contributor Author

pcorpet commented Dec 27, 2024

I'm using v7, however I have setup a fix in our types before switching to v6 so I might be wrong about it. Just FYI, this is what I've added:

// TODO(pascal): Fix the types upstream in https://github.com/mui/mui-x
declare module '@mui/x-data-grid-premium' {
	// On first render, the grid api ref is using a DEFAULT_VALUE  = {} object. Here we consider it's equivalent
	// to a Partial<GridApiPremium> to enforce callers to check API's methods exist before calling them.
	// After unmount, the ref is set to null.
	export const useGridApiRef: () => MutableRefObject<Partial<GridApiPremium> | null>

	export interface DataGridPremiumProps<R extends GridValidRowModel>
		extends Omit<MuiDataGridPremiumProps<R>, 'apiRef'> {
		apiRef?: MutableRefObject<Partial<GridApiPremium> | null>
	}
}

Note that useGridApiRef might actually be in trouble in useEffect if, in the example above, you start with "mounted" to be false. In that case you have the default value ({}).

This is happening in some of our components where the DataGrid is not loaded right away, however we have a useEffect nearby. We can easily check whether the DataGrid is mounted or not before calling it, however the type hints are not helping as much as they should.

I don't think this is about fixing the issue #6879 here, just to have types follow what's happening so that we can handle any inconsistencies reliably. If the other bug gets fixed one way or the other, then the types should follow the fix.

@github-actions github-actions bot added status: waiting for maintainer These issues haven't been looked at yet by a maintainer and removed status: waiting for author Issue with insufficient information labels Dec 27, 2024
@michelengelen
Copy link
Member

michelengelen commented Jan 2, 2025

As @MBilalShafi stated the useEffect problem should not exist anymore in v7, so whenever you do the switch you can change your augmentation to this:

// TODO(pascal): Fix the types upstream in https://github.com/mui/mui-x
declare module '@mui/x-data-grid-premium' {
	// On first render, the grid api ref is using a DEFAULT_VALUE  = {} object. Here we consider it's equivalent
	// to a Partial<GridApiPremium> to enforce callers to check API's methods exist before calling them.
	// After unmount, the ref is set to null.
	export const useGridApiRef: () => MutableRefObject<Partial<GridApiPremium> | null>

	export interface DataGridPremiumProps<R extends GridValidRowModel>
		extends Omit<MuiDataGridPremiumProps<R>, 'apiRef'> {
-		apiRef?: MutableRefObject<Partial<GridApiPremium> | null>
+		apiRef?: MutableRefObject<GridApiPremium | null>
	}
}

Other than that I would agree with @MBilalShafi to close this one and continue the discussion in #6879 ... The types will follow the fix in that regard.

If you disagree @pcorpet please feel free to comment ... the bot will then reopen the issue!
Thanks for taking the time to open it! 🙇🏼

Copy link

github-actions bot commented Jan 2, 2025

This issue has been closed. If you have a similar problem but not exactly the same, please open a new issue.
Now, if you have additional information related to this issue or things that could help future readers, feel free to leave a comment.

Note

@pcorpet How did we do? Your experience with our support team matters to us. If you have a moment, please share your thoughts in this short Support Satisfaction survey.

@github-actions github-actions bot removed the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Jan 2, 2025
@pcorpet
Copy link
Contributor Author

pcorpet commented Jan 2, 2025

I've just tried and made sure to use DataGrid v7 and still have the issue.

Here's a code to reproduce:
https://codesandbox.io/p/sandbox/ylmhk7

If you check the box, then uncheck the box, you'll see in the console:
image

So I confirm that the type is still not correct:

  • first it's an empty object
  • then the proper api ref
  • and then null

@pcorpet
Copy link
Contributor Author

pcorpet commented Jan 2, 2025

If you disagree @pcorpet please feel free to comment ... the bot will then reopen the issue!

Apparently it's not working

@michelengelen michelengelen reopened this Jan 2, 2025
@github-project-automation github-project-automation bot moved this from 🆕 Needs refinement to 📋 Backlog in MUI X Data Grid Jan 2, 2025
@michelengelen
Copy link
Member

Ah, now I get it ... the problem is we always consider the grid to be mounted on first render. Interestingly enough we still instantiate the ref with an empty object. So the ref can be in 3 different states:

  1. uninstantiated => {}
  2. instantiated => Api
  3. unmounted => null

We should probably reduce the cases to 2 in total ... did we ever consider using null as the initial value?

diff --git a/packages/x-data-grid/src/DataGrid/useDataGridComponent.tsx b/packages/x-data-grid/src/DataGrid/useDataGridComponent.tsx
index 2c888c05b..3d4cb3cdc 100644
--- a/packages/x-data-grid/src/DataGrid/useDataGridComponent.tsx
+++ b/packages/x-data-grid/src/DataGrid/useDataGridComponent.tsx
@@ -63,7 +63,7 @@ import {
 } from '../hooks/features/listView/useGridListView';

 export const useDataGridComponent = (
-  inputApiRef: React.MutableRefObject<GridApiCommunity> | undefined,
+  inputApiRef: React.MutableRefObject<GridApiCommunity | null> | undefined,
   props: DataGridProcessedProps,
 ) => {
   const apiRef = useGridInitialization<GridPrivateApiCommunity, GridApiCommunity>(
diff --git a/packages/x-data-grid/src/hooks/core/useGridApiInitialization.ts b/packages/x-data-grid/src/hooks/core/useGridApiInitialization.ts
index 3113bc997..7d9f64a1d 100644
--- a/packages/x-data-grid/src/hooks/core/useGridApiInitialization.ts
+++ b/packages/x-data-grid/src/hooks/core/useGridApiInitialization.ts
@@ -96,7 +96,7 @@ export function useGridApiInitialization<
   PrivateApi extends GridPrivateApiCommon,
   Api extends GridApiCommon,
 >(
-  inputApiRef: React.MutableRefObject<Api> | undefined,
+  inputApiRef: React.MutableRefObject<Api | null> | undefined,
   props: Pick<DataGridProcessedProps, 'signature'>,
 ): React.MutableRefObject<PrivateApi> {
   const publicApiRef = React.useRef() as React.MutableRefObject<Api>;
diff --git a/packages/x-data-grid/src/hooks/core/useGridInitialization.ts b/packages/x-data-grid/src/hooks/core/useGridInitialization.ts
index d59852b43..bb818680a 100644
--- a/packages/x-data-grid/src/hooks/core/useGridInitialization.ts
+++ b/packages/x-data-grid/src/hooks/core/useGridInitialization.ts
@@ -17,7 +17,7 @@ export const useGridInitialization = <
   PrivateApi extends GridPrivateApiCommon,
   Api extends GridApiCommon,
 >(
-  inputApiRef: React.MutableRefObject<Api> | undefined,
+  inputApiRef: React.MutableRefObject<Api | null> | undefined,
   props: DataGridProcessedProps,
 ) => {
   const privateApiRef = useGridApiInitialization<PrivateApi, Api>(inputApiRef, props);
diff --git a/packages/x-data-grid/src/hooks/utils/useGridApiRef.ts b/packages/x-data-grid/src/hooks/utils/useGridApiRef.ts
index ae5db730f..7afdeb65a 100644
--- a/packages/x-data-grid/src/hooks/utils/useGridApiRef.ts
+++ b/packages/x-data-grid/src/hooks/utils/useGridApiRef.ts
@@ -6,4 +6,4 @@ import { GridApiCommunity } from '../../models/api/gridApiCommunity';
  * Hook that instantiate a [[GridApiRef]].
  */
 export const useGridApiRef = <Api extends GridApiCommon = GridApiCommunity>() =>
-  React.useRef({}) as React.MutableRefObject<Api>;
+  React.useRef(null) as React.MutableRefObject<Api | null>;
diff --git a/packages/x-data-grid/src/models/props/DataGridProps.ts b/packages/x-data-grid/src/models/props/DataGridProps.ts
index 02c4bc07b..5baa98ecc 100644
--- a/packages/x-data-grid/src/models/props/DataGridProps.ts
+++ b/packages/x-data-grid/src/models/props/DataGridProps.ts
@@ -415,7 +415,7 @@ export interface DataGridPropsWithoutDefaultValue<R extends GridValidRowModel =
   /**
    * The ref object that allows Data Grid manipulation. Can be instantiated with `useGridApiRef()`.
    */
-  apiRef?: React.MutableRefObject<GridApiCommunity>;
+  apiRef?: React.MutableRefObject<GridApiCommunity | null>;
   /**
    * Forwarded props for the Data Grid root element.
    * @ignore - do not document.

Thoughts?

@pcorpet
Copy link
Contributor Author

pcorpet commented Jan 2, 2025

Yes, I would very much like that. I believe that this is how useRef does it

@romgrk
Copy link
Contributor

romgrk commented Jan 3, 2025

@michelengelen's proposition LGTM.

@romgrk romgrk moved this from 📋 Backlog to 🔖 Ready in MUI X Data Grid Jan 3, 2025
@cherniavskii
Copy link
Member

@michelengelen Do you want to open a PR?
It might be worth creating the ApiRef type and using it instead of MutableRefObject all over the place.

@FredSwadlingPelt8
Copy link

Hi, I just did a minor version update to "@mui/x-data-grid-pro": "7.23.6", from "@mui/x-data-grid-pro": "7.23.5", and ive started seeing type errors with all my datagrids that use useGridApiRef() to pass in an apiref. Im getting:

Type 'RefObject' is not assignable to type 'MutableRefObject'.
Types of property 'current' are incompatible.
Type 'GridApiPro | null' is not assignable to type 'GridApiPro'.
Type 'null' is not assignable to type 'GridApiPro'.ts(2322)

Is this related? and if so is there a fix for this?

@arminmeh
Copy link
Contributor

@FredSwadlingPelt8
This PR should solve it

@sternma
Copy link

sternma commented Jan 15, 2025

@arminmeh , I am seeing the same issue upgrading from v7.22.1 to v7.23.6:

error TS2322: Type 'RefObject<GridApiCommunity>' is not assignable to type 'MutableRefObject<GridApiCommunity>'.
  Types of property 'current' are incompatible.
    Type 'GridApiCommunity | null' is not assignable to type 'GridApiCommunity'.
      Type 'null' is not assignable to type 'GridApiCommon<GridStateCommunity, GridInitialStateCommunity>'.

             <MyComponent myProp={value} apiRef={myApiRef} />

Where myApiRef is of type MutableRefObject<GridApiCommunity>.

The weirdest part about this is that my Intellisense doesn't pick up any issue at all. Seems totally fine until build.

Using Typescript 5.7.3 for reference.

Assuming it is fixed by the linked PR, but it's concerning that this is only detected at build time and not by Intellisense. Will wait for the next patch version!

@arminmeh
Copy link
Contributor

arminmeh commented Jan 16, 2025

@arminmeh , I am seeing the same issue upgrading from v7.22.1 to v7.23.6:

error TS2322: Type 'RefObject<GridApiCommunity>' is not assignable to type 'MutableRefObject<GridApiCommunity>'.
  Types of property 'current' are incompatible.
    Type 'GridApiCommunity | null' is not assignable to type 'GridApiCommunity'.
      Type 'null' is not assignable to type 'GridApiCommon<GridStateCommunity, GridInitialStateCommunity>'.

             <MyComponent myProp={value} apiRef={myApiRef} />

Where myApiRef is of type MutableRefObject<GridApiCommunity>.

The weirdest part about this is that my Intellisense doesn't pick up any issue at all. Seems totally fine until build.

Using Typescript 5.7.3 for reference.

Assuming it is fixed by the linked PR, but it's concerning that this is only detected at build time and not by Intellisense. Will wait for the next patch version!

This is actually caused by the PR that you have linked. And it is only an issue for React < 19.
It will be fixed as a part of another issue
You can check this comment

Update
I just realized that we are about to release the linked PR. So, your statement was correct. The fix for the type mismatch will be released by the end of the week.
Other problem from the linked comment (apiRef.current being possibly null) will still be there in the next release but I am working on that as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change bug 🐛 Something doesn't work component: data grid This is the name of the generic UI component, not the React module! support: premium standard Support request from a Premium standard plan user. https://mui.com/legal/technical-support-sla/ typescript
Projects
Status: 🔖 Ready
Development

No branches or pull requests

8 participants