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

Add missing dev dependencies to base/next #30513

Closed
wants to merge 1 commit into from

Conversation

Janpot
Copy link
Member

@Janpot Janpot commented Jan 5, 2022

Discovered this while trying a few things with rollup while investigating #30510 and #30200
@mui/material is being used in the tests of these packages.

@mui-pr-bot
Copy link

mui-pr-bot commented Jan 5, 2022

No bundle size changes

Generated by 🚫 dangerJS against 2540dec

@@ -60,6 +60,7 @@
"react-is": "^17.0.2"
},
"devDependencies": {
"@mui/material": "^5.2.7",
Copy link
Member

Choose a reason for hiding this comment

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

This looks strange to me because mui-base should not use anything from the design system package (mui-material). cc @michaldudak

Copy link
Member Author

@Janpot Janpot Jan 6, 2022

Choose a reason for hiding this comment

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

The package itself doesn't use it, only some of its tests do. I can imagine there to be legitimate use cases for that. e.g. to verify that both packages integrate correctly. Point is, that if it's used, it's best to declare it in the package.json to avoid confusing any tools that try to statically analyse dependencies.

@michaldudak
Copy link
Member

Actually, the correct solution here would be to refactor the tests not to use @mui/material. The Base package should be standalone as much as possible. The integration testing is done in the material-next package.

I suggest closing this PR without merging. I'll create a follow-up to clean the tests.

@Janpot Janpot closed this Jan 10, 2022
@Janpot Janpot deleted the missing-dev-deps branch January 10, 2022 08:34
@Janpot
Copy link
Member Author

Janpot commented Jan 10, 2022

@michaldudak Thanks for looking into this. One thing to be aware of, in @mui/material-next it's not the tests, it's the typings that import @mui/material.

@zannager zannager added the package: base-ui Specific to @mui/base label Jan 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: base-ui Specific to @mui/base
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants