-
-
Notifications
You must be signed in to change notification settings - Fork 32.4k
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
[core] Convert scripts to ES modules #35036
Conversation
Logdanger-results://tmp/danger-results-0d77b8cd.json |
91fccee
to
e26b976
Compare
e26b976
to
5eaa297
Compare
@mui/toolpad, @mui/x please verify if your projects are affected by these changes. |
Minor changes required mui/toolpad#1307 edit: pleasantly surprised to see you didn't have to resort to |
@michaldudak I notice the dangerfile runs async code. It's possible to dynamically https://nodejs.org/api/esm.html#import-statements
|
@Janpot thanks for the tip! I'll try it out. |
0f882b6
to
6c8e1e8
Compare
🤦 looks like danger is compiling it down to commonjs before running it edit: danger/danger-js#1180 |
Yep, it won't fly this way.. |
6c8e1e8
to
0851b5b
Compare
I reverted the changes done to the sizeComparison directory. It's a CJS module again. |
0851b5b
to
5eaa297
Compare
I also reverted changes to buildColorTypes. It's not really a proper ES module as it imports @Janpot I assume there's no way to import such a not-ESM-but-also-not-CJS-either file in Node but if you happen to know any tricks, I'll be happy to learn. |
The module
For this to resolve correctly, we first need to make the MUI packages proper ESM (I started that effort here, but it's gone a bit stale by now #30510, basically we need to add It's possible to import CJS in ESM, so if the package has been built, the following works: // @mui/material/build/colors/index.js is a commonjs module
import * as colors from '@mui/material/build/colors/index.js'; Not a great solution though, and eslint won't like it neither |
The way I understand it is it doesn't have file extensions in exports, so it's not exactly a valid ES module. Babel does handle it, but Node needs file extensions. |
Right, good catch 👍. edit: If I'm not mistaken, module resolution is not part of the ecmascript specification. It's valid ESM, node.js just doesn't know how to handle the specifier (Not that it matters in how we handle the problem here 🙂 ) |
Converted applicable scripts from the
/scripts
directory to ESM.Contents of the scripts/sizeSnapshot directory were not converted because they were used by Danger. The dangerfile can't be an ES module.
Also renamed a few scripts for better consistency (kebab-case to camelCase).