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] Update peer dependencies for v8 #16563

Merged
merged 7 commits into from
Feb 17, 2025
Merged

Conversation

Janpot
Copy link
Member

@Janpot Janpot commented Feb 13, 2025

Add the 7.0.0-alpha range as currently it errors on npm install.

edit: To clarify, it errors on end-users trying to install both next targets of core and X side-by-side. X doesn't accept the new core v7 alpha packages as a peer dependency yet.

Questions:

  • Would it make sense to further tighten the range and remove v5? IIUC the goal is to only support last two majors.

@Janpot Janpot added the scope: code-infra Specific to the core-infra product label Feb 13, 2025
@LukasTy
Copy link
Member

LukasTy commented Feb 13, 2025

Add the 7.0.0-alpha range as currently it errors on npm install.

Are you sure that it errors out because of this? 🤔
I have screwed up the lock file after two dependencies PRs merging. 🙈
#16562

I don't think this change is needed. 🤔

@mui-bot
Copy link

mui-bot commented Feb 13, 2025

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

Generated by 🚫 dangerJS against bcb8864

@Janpot
Copy link
Member Author

Janpot commented Feb 13, 2025

Are you sure that it errors out because of this? 🤔

Running npm install with

// ./package.json
  "dependencies": {
    "@mui/material": "next",
    "@mui/x-charts-pro": "next",

fails for me with

npm error code ERESOLVE
npm error ERESOLVE could not resolve
npm error
npm error While resolving: @mui/x-charts-pro@8.0.0-alpha.11
npm error Found: @mui/material@7.0.0-alpha.1
npm error node_modules/@mui/material
npm error   @mui/material@"next" from the root project
npm error   peer @mui/material@"7.0.0-alpha.1" from @mui/icons-material@7.0.0-alpha.1
npm error   node_modules/@mui/icons-material
npm error     @mui/icons-material@"next" from the root project
npm error
...

The same command with

// ./package.json
  "dependencies": {
    "@mui/material": "next",
    // csbci for this PR
    "@mui/x-charts-pro": "https://pkg.csb.dev/mui/mui-x/commit/4ce90d46/@mui/x-charts-pro",

installs correctly

I don't think this change is needed. 🤔

We'll have to update the peer dependencies for v8 so it can work with core v7. This just sets it for the prerelease so we can test this without having to make npm do legacy peer deps handling. This change shouldn't be cherry-picked

The lock file changes are unrelated, something on master is not quite there

@Janpot Janpot marked this pull request as ready for review February 13, 2025 09:20
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 17, 2025
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 Feb 17, 2025
Copy link
Member

@LukasTy LukasTy left a comment

Choose a reason for hiding this comment

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

LGTM. 👍
Let's allow for easier ESM adoption/testing. 👍

Would it make sense to further tighten the range and remove v5? IIUC the goal is to only support last two majors.

That's a topic for the "product" finalize, but based on brief discussion on Friday, we don't feel like it would be for the better product wise. 🤷

@Janpot Janpot merged commit 24402c2 into mui:master Feb 17, 2025
18 checks passed
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants