-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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] Fail with an explicit error when API context is missing #1877
Conversation
2e32eb6
to
365f0c1
Compare
export function useGridApiContext() { | ||
const apiRef = React.useContext(GridApiContext); | ||
|
||
if (apiRef === undefined) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about raising a warning instead of throwing and only in dev mode to not have any bundle size impact?
Is it important enough to throw?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is important to throw as users would have issues at a later stage.
We could also consider combining it with useGridApiRef so you have a single point to get or create the apiRef 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about raising a warning instead of throwing and only in dev mode to not have any bundle size impact?
@oliviertassinari I tested with the docs and the warning wasn't sent to the console, but to the terminal that was running the dev server. A throw in this situation facilitates for the user to know where's the problem.
We could also consider combining it with useGridApiRef so you have a single point to get or create the apiRef 🤔
@dtassone Let's imagine that there's another grid inside the grid, like a child row. How will we know if a apiRef has to be created or the one in the context should used? Maybe we need to add an argument to force to create the apiRef even if there's a context in the tree.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but to the terminal that was running the dev server.
@m4theushw True, I was actually just talking about this problem in mui/material-ui#18119 (comment). It's a Next.js thing that they could solve by forwarding the console log to the client too (not only the error). But fair enough.
In the future, we could set up the error modification script that Sebastian built (to save a bit of bundle size)
How will we know if a apiRef has to be created or the one in the context should used?
💯. I would even push it one step further is unbundle useGridApiRef. I think that we need 3 hooks, for 3 different use cases, not 1.
useGridApiContext
to consume the API ref (React.useContext
)const apiRef = React.useRef<GridApi>(createGridApi());
to create the apiRef and store it +React.useImperativeHandle
useGridApiRef
to read the API ref from the child data grid ONLY (React.useRef
)
I did a first step in this direction with #990
Co-authored-by: damien <damien.tassone@gmail.com>
Closes #1401