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

[code-infra] Remove @mui/material-nextjs dependency #15925

Merged
merged 3 commits into from
Dec 18, 2024

Conversation

LukasTy
Copy link
Member

@LukasTy LukasTy commented Dec 18, 2024

I'm unsure why this dependency was added... 🤷

@LukasTy LukasTy added scope: docs-infra Specific to the docs-infra product scope: code-infra Specific to the core-infra product labels Dec 18, 2024
@LukasTy LukasTy self-assigned this Dec 18, 2024
@mui-bot
Copy link

mui-bot commented Dec 18, 2024

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

Generated by 🚫 dangerJS against 7d03e5e

@LukasTy LukasTy requested a review from a team December 18, 2024 10:04
@@ -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',
Copy link
Member Author

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... 🤷 🤯 🙈

Copy link
Member

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

Copy link
Member

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

Copy link
Member Author

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. 🤷

Copy link
Member

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.

Copy link
Member Author

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. 👍

Copy link
Member

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 😄

Comment on lines +20 to +21
'DataGridOverlays',
'PopularFeaturesDemo',
Copy link
Member Author

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. 🤞 🙈

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Dec 18, 2024
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Dec 18, 2024
@LukasTy LukasTy merged commit 2700d7e into mui:master Dec 18, 2024
18 checks passed
@LukasTy LukasTy deleted the remove-material-nextjs-dep branch December 18, 2024 15:21
lauri865 pushed a commit to lauri865/mui-x that referenced this pull request Dec 19, 2024
LukasTy added a commit to LukasTy/mui-x that referenced this pull request Dec 19, 2024
@JCQuintas
Copy link
Member

JCQuintas commented Dec 19, 2024

@LukasTy getting this: https://app.netlify.com/sites/material-ui-x/deploys/676437ad00cc550008c3420b

3:15:03 PM: ../node_modules/.pnpm/@mui+monorepo@https+++codeload.github.com+mui+material-ui+tar.gz+356d5dffbcbc1c18b5ab8060a173_3wwoea3hjx53yfq2ez76oxomce/node_modules/@mui/monorepo/docs/pages/_document.js
3:15:03 PM: Module not found: Can't resolve '@mui/material-nextjs/v13-pagesRouter'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope: code-infra Specific to the core-infra product scope: docs-infra Specific to the docs-infra product
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants