-
Notifications
You must be signed in to change notification settings - Fork 352
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
Convert scripts to ES modules #10834
Conversation
6c9fed6
to
6028310
Compare
Marking this one as ready to be reviewed, the documentation is failing on CI as it needs the updated workflows, but since it is triggered from the target branch it doesn't pass. I've tested this locally, and it works, so the failure can be ignored. |
@@ -62,7 +62,7 @@ runs: | |||
- name: Run build | |||
if: inputs.skip-build != 'true' && steps.cache-build.outputs.cache-hit != 'true' | |||
shell: bash | |||
run: yarn build && yarn build:umd | |||
run: yarn build && yarn build:umd && yarn clean:exports |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to be a regression that was introduced, this script was previously ran on CI and was lost in refactoring. I've restored it here, and I will create a patch to fix this for v5
as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this removal was actually intentional, as we now lo longer export our demo files in the top level index of react-core, which is what that clean:exports
was fixing before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, it seems it was removed under #10114, but the script was left behind. I've pushed some changes to remove this script entirely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file was orphaned, so I removed it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file already exists as a .mjs
variant, this was likely accidentally restored as part of a merge conflict, so I removed it.
Signed-off-by: Jon Koops <[email protected]>
Your changes have been released in:
Thanks for your contribution! 🎉 |
Converts the various scripts in the repository to use the ES module syntax instead of CommonJS. This helps future proof these scripts and makes it more consistent with how the rest of the code is written.
This work also simplifies landing #10831, as it allows easy tracking of which files can be used with
"type": "module"
. Subsequent work will also remove the.mjs
extension in favor of using a 'plain'.js
extension combined with"type": "module"
. At which point all remaining CommonJS modules will be given the.cjs
extension to identify them, or by adding a package-level"type": "require"
.