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

[codemod] Fix optimal-imports to support v4 and v5-alpha, beta #28812

Merged
merged 2 commits into from Oct 5, 2021

Conversation

siriwatknp
Copy link
Member

@siriwatknp siriwatknp commented Oct 4, 2021

Open as draft to discuss about this change. I can reproduce with @material-ui/core latest(4.12.3) that codemod cannot transform import blueGrey from "@material-ui/core/colors/blueGrey";

Got the same error as in #28542

 ERR /Users/siriwatknp/practice-repos/mui-colors-codemod/src/App.js Transformation error (Cannot find module '@material-ui/core/modern/colors' Require stack: - /Users/siriwatknp/Personal-Repos/material-ui/packages/mui-codemod/src/v5.0.0/optimal-imports.js - /Users/siriwatknp/Personal-Repos/material-ui/packages/mui-codemod/src/v5.0.0/preset-safe.js - /Users/siriwatknp/Personal-Repos/material-ui/node_modules/jscodeshift/src/Worker.js)
Error: Cannot find module '@material-ui/core/modern/colors'

but by using /es/ folder, it works. @mnajdova what is the reason to use /modern/ at that time?

Note: in node_modules, I only see folder es and esm but no sign of modern.

@siriwatknp siriwatknp added the package: codemod Specific to @mui/codemod label Oct 4, 2021
@mui-pr-bot
Copy link

mui-pr-bot commented Oct 4, 2021

No bundle size changes

Generated by 🚫 dangerJS against 927f135

@mnajdova
Copy link
Member

mnajdova commented Oct 5, 2021

The modern bundle was introduced in v5 with #22814 this is why when running on v4 it fails. The problematic thing I think is that the codemod could be run at any time during the upgrade (when still using v4, or when using v5).

The implemented changes would not work if someone upgraded to some v5 alpha version and then run the codemod. So maybe, in the end, the best solution would be to use the es by default, but if it does not exists to fallback to modern.

cc @eps1lon for thoughts.

@siriwatknp siriwatknp marked this pull request as ready for review October 5, 2021 08:37
@siriwatknp
Copy link
Member Author

siriwatknp commented Oct 5, 2021

The modern bundle was introduced in v5

@mnajdova Got it. added try catch to support both v4 and v5-alpha, beta. Tested locally, both works! no more error.

@siriwatknp siriwatknp changed the title [codemod] Revert optimal-imports v5 to look for es folder [codemod] Fix optimal-imports to support v4 and v5-alpha, beta Oct 5, 2021
Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

This makes sense to me 👍 Thanks @siriwatknp

@eps1lon
Copy link
Member

eps1lon commented Oct 5, 2021

that codemod cannot transform import blueGrey from "@material-ui/core/colors/blueGrey";

The codemod should not transform these paths. It should only transform supported paths and give up on everything else. To catch unsupported paths we can recommend the way we do it via Eslint.

Trying to guess the correct path from an unsupported path is a constant uphill battle. I would not recommend it though it's no longer my decision.

@siriwatknp
Copy link
Member Author

To catch unsupported paths we can recommend the way we do it via Eslint

Noted on this point to consider in the future. But I think this PR also help people with the migration which also benefit us with less issue (I hope), so I am merging it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: codemod Specific to @mui/codemod
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants