-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
[code-infra] Remove @mui/material-nextjs
dependency
#15925
Conversation
Deploy preview: https://deploy-preview-15925--material-ui-x.netlify.app/ |
@@ -30,7 +30,6 @@ const defaultAlias = { | |||
'@mui/x-tree-view': resolveAliasPath('./packages/x-tree-view/src'), | |||
'@mui/x-tree-view-pro': resolveAliasPath('./packages/x-tree-view-pro/src'), | |||
'@mui/x-internals': resolveAliasPath('./packages/x-internals/src'), | |||
'@mui/material-nextjs': '@mui/monorepo/packages/mui-material-nextjs/src', |
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.
We had the dependency, but it was not even used as we were aliasing it... 🤷 🤯 🙈
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.
some day we like to remove the alias and just rely on the dependency
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 the point is that it is not even used
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 do you think there is a need for this dependency to exist?
I can find https://github.com/mui/material-ui/blob/1920b80bdf58e6fb1f6bb4394f55fa0a07d0faf0/docs/pages/_document.js#L3 that seems like a downstream usage, but I struggle to understand if anything is impacted by the removal of this dependency. 🤔
A style
element with insertion-point-jss
id is still inserted in the DOM. 🤷
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's one of the implicit dependencies coming from the docs. since the docs dependencies aren't installed automatically but we are using monorepo docs code that uses those dependencies, we need to copy them exactly from the monorepo. hopefully we can move all that usage and dependencies to the @mui/docs package some day so that it happens automatically.
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'm merging this "optimization".
Once we further the progress on the @mui/docs
migration I hope that the package will handle the necessary dependencies installation. 👍
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.
If it's a dependency of the docs workspace in core, it should probably be one of the docs workspace in X/toolpad/... as well. If it's not broken, then that's only by coincidence.
But I'm not going to die on this hill, it'll break when it wants to break 😄
'DataGridOverlays', | ||
'PopularFeaturesDemo', |
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.
These are very flaky on Argos lately, hopefully increasing their timeout helps. 🤞 🙈
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
@LukasTy getting this: https://app.netlify.com/sites/material-ui-x/deploys/676437ad00cc550008c3420b
|
This reverts commit 2700d7e.
I'm unsure why this dependency was added... 🤷