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

[docs] Fix missing dependency in the DataGrid demo #27597

Merged
merged 3 commits into from Aug 9, 2021

Conversation

m4theushw
Copy link
Member

@m4theushw m4theushw commented Aug 4, 2021

Fixes #27629

The DataGrid component depends on @material-ui/styles. In v4, this package is a dependency of @material-ui/core. However, in v5 it became optional. When the demo is opened in CodeSandbox, it collects all imports and adds the equivalent dependencies to the package.json. Since @material-ui/styles was not being imported it was not added, so it crashes.

Preview: https://deploy-preview-27597--material-ui.netlify.app/components/tables/#data-table
Open the demo in CodeSandbox

@mui-pr-bot
Copy link

mui-pr-bot commented Aug 4, 2021

No bundle size changes (experimental)

Generated by 🚫 dangerJS against 01147a7

@eps1lon
Copy link
Member

eps1lon commented Aug 5, 2021

The DataGrid component depends on @material-ui/styles.

Direct dependency or peer dependency?

@eps1lon eps1lon added the docs Improvements or additions to the documentation label Aug 5, 2021
@m4theushw
Copy link
Member Author

The DataGrid component depends on @material-ui/styles.

Direct dependency or peer dependency?

@eps1lon It's a transitive dependency.

@eps1lon
Copy link
Member

eps1lon commented Aug 5, 2021

It should either be a direct dependency or peer dependency. It sounds like DataGrid is importing @material-ui/styles, but not declaring it. That was already an issue when using /core v4 that is now more apparent.

When you say "transitive" then what package depends on it that you depend on? If you depend on a package that has a peer dependency on /styles then you need to either declare it as a dependency or hoist it by declaring it as a peerDependency

@m4theushw
Copy link
Member Author

m4theushw commented Aug 5, 2021

It should either be a direct dependency or peer dependency. It sounds like DataGrid is importing @material-ui/styles, but not declaring it. That was already an issue when using /core v4 that is now more apparent.

That's something we need to fix to support v5.

When you say "transitive" then what package depends on it that you depend on?

The @material-ui/data-grid depends on @material-ui/styles which is satisfied by using @material-ui/core v4.

If you depend on a package that has a peer dependency on /styles then you need to either declare it as a dependency or hoist it by declaring it as a peerDependency

Yeah, we gonna need to add @material-ui/styles as a peer dependency and check if its version and the core version match.


I changed this PR to not relay on the import-hack but to add @material-ui/styles as a dependency when opening the demo in CodeSandbox.

Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

I suspect that for the next branch in the core repository, it won't be relevant long as we will likely drop core v4 support or simply degrade its support by depending on @mui/core-material v5 to use styled() with a peer dependence on emotion.
I think that it would help to cherry-pick this on master.

@oliviertassinari oliviertassinari added the component: data grid This is the name of the generic UI component, not the React module! label Aug 6, 2021
@eps1lon
Copy link
Member

eps1lon commented Aug 7, 2021

The @material-ui/data-grid depends on @material-ui/styles which is satisfied by using @material-ui/core v4.

You can't satisfy a dependency on B by declaring a dependency on A that depends on B. That was never supported and mostly worked incidentally.

@eps1lon eps1lon merged commit 3138cdd into mui:next Aug 9, 2021
@m4theushw m4theushw deleted the datagrid-demo branch November 1, 2021 20:02
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! docs Improvements or additions to the documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[DataGrid] @material-ui/styles not found when opening codesandbox demo
4 participants