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

[DataGrid] Initialize apiRef early #11792

Merged

Conversation

cherniavskii
Copy link
Member

@cherniavskii cherniavskii commented Jan 23, 2024

I need this for #11164, where I use apiRef in the sortComparator for the grouping column + sortingModel in the initialState.
The demo throws an error, because when initial sorting happens the apiRef is not initialized yet (this is also the reason why Netlify deployment fails)

A simpler example to illustrate the issue:
Before: https://codesandbox.io/p/sandbox/gallant-wu-gh2svt
After: https://codesandbox.io/p/sandbox/heuristic-spence-nmkx2w

@cherniavskii cherniavskii added component: data grid This is the name of the generic UI component, not the React module! customization: extend Logic customizability enhancement This is not a bug, nor a new feature labels Jan 23, 2024
@flaviendelangle
Copy link
Member

flaviendelangle commented Jan 23, 2024

React tells us not to do that due to concurrent rendering 😬

https://react.dev/learn/referencing-values-with-refs

With that being said, we are doing it in useLazyRef
I think it's dangerous if we do it and the value depend on the props (since they might be invalid in the 1st real render of the concurrent mode).

@mui-bot
Copy link

mui-bot commented Jan 23, 2024

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

Generated by 🚫 dangerJS against 8aec690

@cherniavskii
Copy link
Member Author

React tells us not to do that due to concurrent rendering 😬
https://react.dev/learn/referencing-values-with-refs

I don't see concurrent rendering mentioned on that page.
It does say this though:

Don’t read or write ref.current during rendering. If some information is needed during rendering, use state instead. Since React doesn’t know when ref.current changes, even reading it while rendering makes your component’s behavior difficult to predict. (The only exception to this is code like if (!ref.current) ref.current = new Thing() which only sets the ref once during the first render.)

https://react.dev/learn/referencing-values-with-refs#best-practices-for-refs

And I think this PR falls under the exception mentioned above - it's only assigned once because of the !inputApiRef.current?.state check.
Does this make sense?

@@ -311,6 +312,7 @@ describe('<DataGrid /> - Rows', () => {
await waitFor(() => {
expect(getRow(0)).not.to.have.class('Mui-selected');
});
await microtasks();
Copy link
Member Author

Choose a reason for hiding this comment

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

It's not related to this PR, it's just a fix for the flaky test that fails from time to time: https://app.circleci.com/pipelines/github/mui/mui-x/49318/workflows/26e5b314-bb45-4771-8095-330e1f96670b/jobs/282851
It failed again in this PR, this is why I added a fix here.

@flaviendelangle
Copy link
Member

From what I understand, the reason behind the fact that we should not mutate the ref in the render is the concurrent mode, at least that's what this Stackoverflow answers thinks.
It links to another doc page: https://react.dev/reference/react/useRef#caveats, which states the following:

Do not write or read ref.current during rendering, except for initialization. This makes your component’s behavior unpredictable.

Once again, in the doc there is no explicit mention of why you should not do it, just some blurry words like "behavior unpredictable".
But in any case, what you are doing looks like an initialization so I guess it is safe.

Copy link
Contributor

@romgrk romgrk left a comment

Choose a reason for hiding this comment

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

LGTM. I don't think those notes in the react docs are applicable to us.

@cherniavskii cherniavskii marked this pull request as ready for review January 23, 2024 17:53
@cherniavskii cherniavskii merged commit 49587c6 into mui:next Jan 23, 2024
17 checks passed
@cherniavskii cherniavskii deleted the apiRef-access-state-before-first-render branch January 23, 2024 17:55
@Rickardo228
Copy link

This error is thrown when trying to use DataGrid (v7.3.0) with react testing library.
image

Specifically, I'm rendering a custom slot for a cell. The cell component calls:
const apiRef = useGridApiContext();

The test itself uses the render method to render the DataGrid import { render, RenderOptions } from '@testing-library/react'

render( <DataGrid ... /> )

@romgrk
Copy link
Contributor

romgrk commented May 8, 2024

Open a new issue with details, make sure to search past issues for a duplicate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: data grid This is the name of the generic UI component, not the React module! customization: extend Logic customizability enhancement This is not a bug, nor a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants