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.

Sign up for GitHub

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

Jump to bottom

[core] Make @mui/system a direct dependency #11128

Merged
merged 2 commits into from
Nov 28, 2023
Merged

[core] Make @mui/system a direct dependency #11128

merged 2 commits into from
Nov 28, 2023

Conversation

LukasTy
Copy link
Member

@LukasTy LukasTy commented Nov 21, 2023 •
edited
Loading

Fixes #10427

WDYT, should we backport this change to master? 🤔

@LukasTy LukasTy added core Infrastructure work going on behind the scenes component: data grid This is the name of the generic UI component, not the React module! component: pickers This is the name of the generic UI component, not the React module! component: tree view TreeView, TreeItem. This is the name of the generic UI component, not the React module! component: charts This is the name of the generic UI component, not the React module! labels Nov 21, 2023
@LukasTy LukasTy self-assigned this Nov 21, 2023
@mui-bot
Copy link

mui-bot commented Nov 21, 2023 •
edited
Loading

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

Generated by 🚫 dangerJS against 5b66d33

@flaviendelangle
Copy link
Member

Can it be breaking in any way?

@flaviendelangle
Copy link
Member

Btw, we should update the README of each package to remove @mui/system from the listed peer deps.

@LukasTy
Copy link
Member Author

LukasTy commented Nov 21, 2023

Can it be breaking in any way?

Good question. You could probably encounter some interesting use case where we could end up with problematic or suboptimal installation. 🤔
For example: if someone already had listened to the peerDep rule and added an exact @mui/system version package dependency, in this case they could end up with duplicate package installations, but I fail to think of any real breaking change that this could introduce. 🤷

Btw, we should update the README of each package to remove @mui/system from the listed peer deps.

Didn't I already do it? 🤔 Or is there something that I've missed?

@flaviendelangle
Copy link
Member

Didn't I already do it? 🤔 Or is there something that I've missed?

oO I thought I checked the diff correctly sorry

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 23, 2023
@github-actions GitHub Actions
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 Nov 28, 2023
@LukasTy
Copy link
Member Author

LukasTy commented Nov 28, 2023

Going to proceed with merging this change for now as suggested in: #10427 (comment)

@LukasTy LukasTy enabled auto-merge (squash) November 28, 2023 15:03
@LukasTy LukasTy merged commit c60f875 into mui:next Nov 28, 2023
2 checks passed
@LukasTy LukasTy deleted the make-system-direct-dep branch November 28, 2023 15:24
@kris7t kris7t mentioned this pull request Mar 26, 2024
Closed
@kris7t
Copy link

kris7t commented Mar 26, 2024

Unfortunately, this is breaking for package managers using dependency isolation (e.g., Yarn) due to missing optional peer dependencies: #12566

@cherniavskii cherniavskii mentioned this pull request Aug 22, 2024
Merged
8 tasks
@markedwards markedwards mentioned this pull request Oct 7, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexfauquette alexfauquette alexfauquette approved these changes

@flaviendelangle flaviendelangle flaviendelangle approved these changes

@oliviertassinari oliviertassinari Awaiting requested review from oliviertassinari

@cherniavskii cherniavskii Awaiting requested review from cherniavskii

Assignees

@LukasTy LukasTy

Labels
component: charts This is the name of the generic UI component, not the React module! component: data grid This is the name of the generic UI component, not the React module! component: pickers This is the name of the generic UI component, not the React module! component: tree view TreeView, TreeItem. This is the name of the generic UI component, not the React module! core Infrastructure work going on behind the scenes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[core] @mui/system has no singleton, it should a dependency
5 participants
@LukasTy @mui-bot @flaviendelangle @kris7t @alexfauquette