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

update @mui/icons-material depdency in material-renderers #2218

Conversation

devbydaniel
Copy link
Contributor

change dependency of @mui/icons-material in material-renderers from ~5.11.16 to ^5.11.16

@CLAassistant
Copy link

CLAassistant commented Nov 20, 2023

CLA assistant check
All committers have signed the CLA.

Copy link

netlify bot commented Nov 20, 2023

Deploy Preview for jsonforms-examples ready!

Name Link
🔨 Latest commit 03e5435
🔍 Latest deploy log https://app.netlify.com/sites/jsonforms-examples/deploys/656052d61c797f000845b904
😎 Deploy Preview https://deploy-preview-2218--jsonforms-examples.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@devbydaniel
Copy link
Contributor Author

devbydaniel commented Nov 20, 2023

Sorry if I'm missing something super obvious - I basically just changed the ~ to ^ for the @mui/icons-material dependency in material-renderers, updated the lock file and updated dayjs from 1.10.6 to 1.10.7 because of a peer dependency requirement. The error during deployments happen to me even before applying the changes.

@lucas-koehler
Copy link
Contributor

Hi @devbydaniel ,
thanks for the contribution :)
I can see that even the normal build (not just the deployment) fails due to some react error. I don't know what exactly causes this. However, I think it makes sense to only update directly material related dependencies in this PR:

  1. It's easier to see what could cause errors
  2. Many of the other dependencies (e.g. webpack, rollup, react and its types, etc.) are also used in other packages and I'd prefer keeping the versions consistent.

Thus, my suggestion is to revert all changes that don't update mui, emotion or dayjs. What do you think?

@devbydaniel
Copy link
Contributor Author

Thanks for you reply! I actually tried to change only the packages necessary for updating the version of material-icons. I changed the ~ to ^, did a pnpm up and then bumped dayjs because I received a warning that due to the update of packages, a peer depdency (dayjs) is not respected anymore. I don't have that much experience working on such issues, do you have any pointers on what to do instead?

And just to be sure - I tried to build a test application from master without applying any changes and even there I received the same error as in this PR. So could it be possible that the issue is not related to my change?

Thanks for your patience anyway :)

@lucas-koehler
Copy link
Contributor

lucas-koehler commented Nov 21, 2023

Hi @devbydaniel , I just tried to build on master and the normal build (pnpm run build) as well as the examples app build (pnpm run build:examples-app ) both worked fine. Maybe you reverted back to master but still had the new dependencies in the node_modules folder.

My suggestion would be to revert back to master, then clean the repository with git clean -dxf. This deletes all untracked files in the repository! Afterwards, adapt the dependencies of the material and the dayjs package. Then install dependencies with pnpm install in the repository root. You should not need to call pnpm up because we do not want to generally update all dependencies in this PR.

@devbydaniel
Copy link
Contributor Author

@lucas-koehler Thanks again for your help, I think / hope I got it right this time!

@coveralls
Copy link

coveralls commented Nov 23, 2023

Coverage Status

coverage: 84.469%. remained the same
when pulling 03e5435 on devbydaniel:update-@mui/icons-material-dependency
into 77624c6 on eclipsesource:master.

@lucas-koehler
Copy link
Contributor

Hi @devbydaniel , thanks for the update. It seems correct but I'd like that we also make the @mui/material use ^ instead of ~ while we are at it to keep it consistent. Could you please update the @mui/material dependency in the peer and dev dependencies accordingly?
I think, you'll need to do the git clean -dxf and pnpm install again afterwards to update the lock file

@devbydaniel
Copy link
Contributor Author

@lucas-koehler done!

Copy link
Contributor

@lucas-koehler lucas-koehler left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for your contribution :)

@lucas-koehler lucas-koehler merged commit da3321b into eclipsesource:master Nov 24, 2023
8 checks passed
@sdirix sdirix linked an issue Nov 28, 2023 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dependency Issue with latest MUI
4 participants