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] Re-mounting DataGrid with apiRef causes error (apiRef.current is null) #6879

Closed
2 tasks done
philer-jambit opened this issue Nov 16, 2022 · 17 comments
Closed
2 tasks done
Assignees
Labels
bug 🐛 Something doesn't work component: data grid This is the name of the generic UI component, not the React module! dx Related to developers' experience status: waiting for author Issue with insufficient information v6.x

Comments

@philer-jambit
Copy link

philer-jambit commented Nov 16, 2022

Duplicates

  • I have searched the existing issues

Latest version

  • I have tested the latest version

Steps to reproduce 🕹

CodeSandbox: https://codesandbox.io/s/amazing-panini-4pyuru?file=/demo.tsx

I'm trying to use useGridApiRef with a <DataGridPro> that gets mounted conditionally. Here's the simplest example I could come up with:

const GridExample = () => {
  const apiRef = useGridApiRef()
  const [show, setShow] = useState(true)
  return (
    <div>
      <Button onClick={() => setShow(show => !show)}>toggle</Button>
      {show && (
        <DataGridPro
          apiRef={apiRef}
          autoHeight
          rows={[{ id: 1 }, { id: 2 }, { id: 3 }]}
          columns={[{ field: "id" }]}
        />
      )}
    </div>
  )
}

Current behavior 😯

After mounting, un-mounting and re-mounting the component, the following error is raised:

Warning: Failed prop type: The prop `apiRef.current` is marked as required in `ForwardRef(DataGridPro)`, but its value is `null`.

The fact that apiRef.current can be null also means that additional checks are necessary when accessing it, e.g. in an effect hook. This is not reflected by the corresponding type React.MutableRefObject<GridApiPro>.

Adding the following workaround solves the runtime issues:

  // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition
  if (apiRef.current === null) {
    // @ts-expect-error {} is the initial value set by useGridApiRef
    apiRef.current = {}
  }

Expected behavior 🤔

Don't throw an error for the above code – it doesn't do anything crazy.
To solve this, I assume one would have to

either:

Don't set apiRef.current to null – Note that setting an empty object, as the useGridApiRef hook does on init, might also cause issues as it does not match the DataGridPro type.

or:

Make null a legal value for apiRef.current and also reflect that in the typing of useGridApiRef.

Context 🔦

We retrieve data from our backend to render it in a table. Said request may fail, in which case we show an error message in place of the table.
The apiRef is used to track density changes so we can persist them in localStorage.

Here's a rough sketch of what our code does:

const { data, isError } = useQuery(() => fetch("/api/data"))

const apiRef = useGridApiRef()
useEffect(() =>
  apiRef.current.subscribeEvent("stateChange", state => {
    localStorage.setItem("density", gridDensityValueSelector(state, apiRef.current.instanceId))
  })
, [apiRef.current])

if (isError) {
  return <p>Something went wrong</p>
}

return <DataGridPro
  apiRef={apiRef}
  density={localStorage.getItem("density") || "standard"}
  rows={data}
  // ...
/>

Your environment 🌎

  System:
    OS: Linux …
  Binaries:
    …
  Browsers:
    Chrome: Not Found
    Firefox: 106.0.5
  npmPackages:
    @emotion/react: ^11.10.5 => 11.10.5 
    @emotion/styled: ^11.10.5 => 11.10.5 
    @mui/base:  5.0.0-alpha.105 
    @mui/core-downloads-tracker:  5.10.13 
    @mui/icons-material: ^5.10.9 => 5.10.9 
    @mui/material: ^5.10.13 => 5.10.13 
    @mui/private-theming:  5.10.9 
    @mui/styled-engine:  5.10.8 
    @mui/system:  5.10.13 
    @mui/types:  7.2.0 
    @mui/utils:  5.10.6 
    @mui/x-data-grid:  5.17.11 
    @mui/x-data-grid-pro: ^5.17.11 => 5.17.11 
    @mui/x-date-pickers:  5.0.8 
    @mui/x-date-pickers-pro: ^5.0.8 => 5.0.8 
    @mui/x-license-pro:  5.17.11 
    @types/react: ^18.0.25 => 18.0.25 
    react: ^18.2.0 => 18.2.0 
    react-dom: ^18.2.0 => 18.2.0 
    typescript: ^4.8.4 => 4.8.4

Order ID 💳 (optional)

We have one but I have to check if I'm allowed to post it.

@philer-jambit philer-jambit added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Nov 16, 2022
@m4theushw m4theushw changed the title Re-mounting DataGrid with apiRef causes error (apiRef.current is null) [data grid] Re-mounting DataGrid with apiRef causes error (apiRef.current is null) Nov 17, 2022
@m4theushw
Copy link
Member

We don't set explicitly apiRef.current = null when unmounting. It's the useImperativeHandle hook that does, as the same happens with a ref during unmount.

React.useImperativeHandle(inputApiRef, () => publicApiRef.current, [publicApiRef]);

We can add null to the accepted values for apiRef.current. It will solve the warning from the prop-types.

diff --git a/packages/grid/x-data-grid-pro/src/models/dataGridProProps.ts b/packages/grid/x-data-grid-pro/src/models/dataGridProProps.ts
index c4a383678..669a9e3c3 100644
--- a/packages/grid/x-data-grid-pro/src/models/dataGridProProps.ts
+++ b/packages/grid/x-data-grid-pro/src/models/dataGridProProps.ts
@@ -132,7 +132,7 @@ export interface DataGridProPropsWithoutDefaultValue<R extends GridValidRowModel
   /**
    * The ref object that allows grid manipulation. Can be instantiated with [[useGridApiRef()]].
    */
-  apiRef?: React.MutableRefObject<GridApiPro>;
+  apiRef?: React.MutableRefObject<GridApiPro | null>;
   /**
    * The initial state of the DataGridPro.
    * The data in it will be set in the state on initialization but will not be controlled.
diff --git a/packages/grid/x-data-grid/src/models/props/DataGridProps.ts b/packages/grid/x-data-grid/src/models/props/DataGridProps.ts
index 79744a8ee..1ac29cd88 100644
--- a/packages/grid/x-data-grid/src/models/props/DataGridProps.ts
+++ b/packages/grid/x-data-grid/src/models/props/DataGridProps.ts
@@ -350,7 +350,7 @@ export interface DataGridPropsWithoutDefaultValue<R extends GridValidRowModel =
    * TODO: Remove `@internal` when opening `apiRef` to Community plan
    * @ignore - do not document.
    */
-  apiRef?: React.MutableRefObject<GridApiCommunity>;
+  apiRef?: React.MutableRefObject<GridApiCommunity | null>;
   /**
    * Signal to the underlying logic what version of the public component API
    * of the data grid is exposed [[GridSignature]].

Then we'll have to update a bunch of other files to also accept null.

As a workaround for now, you can move <DataGridPro> and useGridApiRef to a dedicated component, and render conditionally this component.

@m4theushw m4theushw added bug 🐛 Something doesn't work component: data grid This is the name of the generic UI component, not the React module! typescript dx Related to developers' experience and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Nov 17, 2022
@joespeargresham
Copy link
Contributor

Is this likely to get fixed please? Unfortunately we can't move our useGridApiRef call into a dedicated component because we need the apiRef higher up in the hierarchy.

@artola
Copy link

artola commented Oct 15, 2023

I fall into the same problem. Rendering conditionally might solve partially the problem but leads to others.

Look this construction, we have 2 components and we switch the view among them. We do not want to dismount them, otherwise we lose the state on all the components inside, thus we use "freeze".

  <div>
      <Freeze freeze={viewId !== 1}>
        <FooGrid data={foo_data} />
      </Freeze>
      <Freeze freeze={viewId === 1}>
        <BarGrid data={bar_data} />
      </Freeze>
  <div />

Switching from Foo to Bar works, when we return to Foo the warning is shown.

@jewseppi
Copy link

jewseppi commented Feb 6, 2024

Is there a plan to resolve this? seems like an open issue for a while. I'm having the same problem with the pro version.

@scamden
Copy link

scamden commented Feb 6, 2024

This also happens when the grid is Suspended. https://codesandbox.io/p/sandbox/datagrid-null-ref-on-suspense-72nnlt (edit: wrong link, updated)

Note: in the sandbox this only produces a warning but in our production deploy GridRow still renders even though the component is suspended (not sure how. working on a tighter repro) and causes null reference errors!

Is there any recommendation around using DataGrid with suspense?

cc @m4theushw @romgrk

@romgrk
Copy link
Contributor

romgrk commented Feb 7, 2024

From what I read in the top comment, this isn't throwing an error, it's displaying a warning. If it's the case, the workaround listed there (setting the ref value to an empty object) is also the best way to deal with this issue. This is a prop-types runtime warning that is safe to ignore, and they should never be enabled on a production build.

I don't think that widening the typings to accept null is a good idea, because it would force every use of the ref to assert existence (apiRef.current!.xxxxx).

@scamden If there is a runtime error, provide the reproduction case and we'll take a look.

@scamden
Copy link

scamden commented Feb 7, 2024

I don't think that widening the typings to accept null is a good idea, because it would force every use of the ref to assert existence (apiRef.current!.xxxxx).

@romgrk I would agree accept that the use of useImperativeHandle means that .current can in fact be null at times so making the type include null lines it up with reality. Unfortunately the workaround of setting to an empty object is even worse because then null checks need to be made on every api method. Treating current as null until it truly has all the methods is actually much safer.

I do have a runtime error. I'm having a hard time creating a repro in codesandbox but basically GridRow renders while a parent is suspending (and therefore current is null). Take a look at the code sandbox I provided which shows that the ref becomes null during suspension (which seems a lot more dangerous than during unmount and is the source of the run time error I haven't easily reproduced)

I have also seen apiRef.current be null during isGroupExpandedByDefault in the grid's initial setup phase fwiw.

@romgrk
Copy link
Contributor

romgrk commented Feb 8, 2024

If you need clean conditional rendering, you can wrap the apiRef inside a wrapper component with the datagrid and re-export it with useImperativeHandle, so that on the outside you have a way to access the API, and in the inside the grid always has a clean API ref object: https://stackblitz.com/edit/mui-mui-x-2hvdmv?file=src%2Fdemo.tsx

The runtime errors are really worrying though, if you're able to get us a reproduction it would be greatly appreciated. The logic is supposed to initialized apiRef.current at render-time before any other code has a chance to touch the API, so it's not supposed to be null.

@scamden
Copy link

scamden commented Apr 11, 2024

Here i've finally got a repro for this: click Suspend button and witness the errror:
https://codesandbox.io/p/sandbox/datagrid-null-ref-on-suspense-72nnlt

@romgrk
Copy link
Contributor

romgrk commented Apr 16, 2024

I think we can avoid the issue by avoiding useImperativeHandle and just using the ref'ed object directly and never setting the ref to null.

@mui/xgrid Any thoughts on dropping useImperativeHandle? Or the alternative, adding null to the useGridApiRef typing?

@cherniavskii
Copy link
Member

we can avoid the issue by avoiding useImperativeHandle and just using the ref'ed object directly and never setting the ref to null.

This sounds good to me!
This might break some tests, but I think we can make it work.

@romgrk
Copy link
Contributor

romgrk commented Apr 22, 2024

I've thought about it some more, and not setting it to null might cause API calls to go through even though they shouldn't. Suspense unmounts the DOM elements, so maybe some operations could have weird results? I'm not sure. Maybe adding | null to the type could make more sense. It would probably break some user builds though. Wdyt?

@cherniavskii cherniavskii self-assigned this Apr 23, 2024
@cherniavskii
Copy link
Member

I created a minimal reproduction example using the DataGrid without row grouping.
While the issue is reproducible in Data Grid v6, I can't reproduce it in v7.

v6: https://stackblitz.com/edit/react-1fe1ex?file=Demo.tsx
v7: https://stackblitz.com/edit/react-1fe1ex-lszfms?file=Demo.tsx

I believe the issue was fixed by #11792

@scamden Can you give v7 a try?

@cherniavskii cherniavskii added status: waiting for author Issue with insufficient information v6.x and removed typescript labels Apr 23, 2024
Copy link

github-actions bot commented May 1, 2024

The issue has been inactive for 7 days and has been automatically closed.

@github-actions github-actions bot closed this as completed May 1, 2024
@romgrk romgrk reopened this May 1, 2024
@romgrk
Copy link
Contributor

romgrk commented May 1, 2024

Did that PR fix the issue completely?

Copy link

github-actions bot commented May 1, 2024

The issue has been inactive for 7 days and has been automatically closed.

@github-actions github-actions bot closed this as completed May 1, 2024
@sterlingdcs-damian
Copy link

Is there a good workaround for this?
I would upgrade to the next major but the codemod doesnt work yet

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: data grid This is the name of the generic UI component, not the React module! dx Related to developers' experience status: waiting for author Issue with insufficient information v6.x
Projects
None yet
Development

No branches or pull requests

9 participants