Skip to content

Use native ESM for dev scripts#12296

Merged
nicolo-ribaudo merged 5 commits intobabel:mainfrom
karansapolia:refac-12137-esm
Jan 30, 2021
Merged

Use native ESM for dev scripts#12296
nicolo-ribaudo merged 5 commits intobabel:mainfrom
karansapolia:refac-12137-esm

Conversation

@karansapolia
Copy link
Contributor

@karansapolia karansapolia commented Nov 2, 2020 •
edited by gitpod-io bot
Loading

Q                       A
Fixed Issues? Fixes #12137
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass?
Documentation PR Link
Any Dependency Changes?
License MIT

@nicolo-ribaudo nicolo-ribaudo added the PR: Internal 🏠 A type of pull request used for our changelog categories label Nov 2, 2020
nicolo-ribaudo
nicolo-ribaudo reviewed Nov 2, 2020
View reviewed changes
nicolo-ribaudo
nicolo-ribaudo reviewed Nov 2, 2020
View reviewed changes
nicolo-ribaudo
nicolo-ribaudo reviewed Nov 2, 2020
View reviewed changes
nicolo-ribaudo
nicolo-ribaudo reviewed Nov 2, 2020
View reviewed changes
nicolo-ribaudo
nicolo-ribaudo reviewed Nov 2, 2020
View reviewed changes
nicolo-ribaudo
nicolo-ribaudo reviewed Nov 2, 2020
View reviewed changes
nicolo-ribaudo
nicolo-ribaudo reviewed Nov 2, 2020
View reviewed changes
nicolo-ribaudo
nicolo-ribaudo reviewed Nov 2, 2020
View reviewed changes
nicolo-ribaudo
nicolo-ribaudo reviewed Nov 2, 2020
View reviewed changes
AjayPoshak
AjayPoshak reviewed Nov 2, 2020
View reviewed changes
Copy link
Contributor

@AjayPoshak AjayPoshak left a comment

Choose a reason for hiding this comment

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

@karansapolia My bad, I did not notice that this PR is still in draft.

JLHwung
JLHwung reviewed Nov 2, 2020
View reviewed changes
JLHwung
JLHwung reviewed Nov 2, 2020
View reviewed changes
@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented Dec 2, 2020

@karansapolia Do you need any help with this? 😄 If you don't have time/interest, I could finish your commits.

(I'm sorry to ping you, but this PR is super exciting for me)

@karansapolia karansapolia force-pushed the refac-12137-esm branch 4 times, most recently from bc950bd to ecd8dcd Compare December 3, 2020 12:23
nicolo-ribaudo
nicolo-ribaudo reviewed Dec 3, 2020
View reviewed changes
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a comment •
edited
Loading

Choose a reason for hiding this comment

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

  1. Since packages/babel-types/scripts only contain native modules, we can put a package.json file there like this one: 
    {
  "type": "module"
}
    and use the .js extension rather than .mjs.
  2. Gulpfile.js doesn't need to be renamed to Gulpfile.mjs
  3. babel.config.js must be renamed to babel.config.cjs
  4. We can rename the files in the test/esm folder to .js, and update the path in the "test:esm" script of the top-level package.json

nicolo-ribaudo
nicolo-ribaudo reviewed Dec 3, 2020
View reviewed changes
@babel-bot
Copy link
Collaborator

babel-bot commented Dec 19, 2020 •
edited
Loading

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/38935/

@codesandbox-ci
Copy link

codesandbox-ci bot commented Dec 20, 2020 •
edited
Loading

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 58046b4:

Sandbox Source
babel-repl-custom-plugin Configuration
babel-plugin-multi-config Configuration

@nicolo-ribaudo nicolo-ribaudo force-pushed the refac-12137-esm branch 2 times, most recently from c21c558 to b6877c8 Compare December 20, 2020 00:25
nicolo-ribaudo
nicolo-ribaudo reviewed Dec 20, 2020
View reviewed changes
.github/workflows/ci.yml
Comment on lines +68 to +70
# Yarn PnP does not support native ESM yet (https://github.com/yarnpkg/berry/issues/638)
# env:
# YARN_NODE_LINKER: pnp # use pnp linker for better linking performance and stricter checks
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo Dec 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

PnP doesn't support ESM yet, but they are working on it: yarnpkg/berry#2161

(:frowning_face: since we only enabled PnP for this CI Job yesterday)

I personally wouldn't expect them to introduce support for ESM by default until Node.js' loader API becomes stable (currently it's --experimental-loader), but:

  • We could decide to use feat(pnp): experimental esm support yarnpkg/berry#2161 instead of their last release: Yarn is committed into our repo, so it will not "suddently break" anyway.
  • They could release ESM support even if loaders are still experimental, maybe behind an experimentalESM config option (cc @merceyz, just in case you like this suggestion 😛)
  • They already use our repository in their E2E tests for node_modules: also giving them a real-world repository that use Yarn 2 to test their ESM support might help with getting ESM support sooner and/or make it more stable!

Copy link
Contributor

@merceyz merceyz Dec 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

All that PR needs is someone to test it but none of the maintainers actually use ESM in our projects so we can't dogfood it, would be great if a project like Babel could test it 👍

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo Dec 21, 2020

Choose a reason for hiding this comment

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

I'll try using it this week and report feedback!

Copy link
Contributor

@merceyz merceyz Dec 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I've added the config enableExperimentalESMLoader to that PR so it can be turned on without setting the root package.jsons type field to module.

I'll try using it this week and report feedback!

Thanks, if you got any questions or anything I'm usually active on Discord 👍

Copy link
Contributor

@JLHwung JLHwung Jan 27, 2021

Choose a reason for hiding this comment

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

Any updates on ESM support in Yarn 2? IMO the PnP check is important in case we forget declaring dependencies. We could add a new build job which transpiles Gulpfile.mjs and then build with PnP.

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo Jan 27, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Because we only lint src/ files with that rule 😬

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo Jan 27, 2021

Choose a reason for hiding this comment

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

It's less important to lint other things since they will never affect our users, but we can lint everything if you prefer.

Copy link
Contributor

@JLHwung JLHwung Jan 27, 2021

Choose a reason for hiding this comment

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

👍 to lint everything except git-ignored.

Copy link
Contributor

@merceyz merceyz Jan 27, 2021

Choose a reason for hiding this comment

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

Any updates on ESM support in Yarn 2?

@JLHwung It works as far as I can tell, @nicolo-ribaudo was testing it on the Babel repo but I don't know the result of that test. All the issues that were brought up was fixed as far as I can tell

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo Jan 27, 2021

Choose a reason for hiding this comment

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

Yup, in this repo it's working (or at least, it was a few weeks ago).
However, I didn't want to put any pressure on yarn since the ESM loader API is far from being stable.

@nicolo-ribaudo nicolo-ribaudo marked this pull request as ready for review December 20, 2020 00:47
@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented Dec 20, 2020

I rebased and fixed the remaining build errors. Thanks @karansapolia for starting this PR! 🧡

@karansapolia
Copy link
Contributor Author

karansapolia commented Dec 20, 2020

@nicolo-ribaudo Thank you to the babel team for the opportunity. This has stretched a lot and needed a lot of reviews and work from you. Lot to learn from you all. Thank you.

@merceyz merceyz mentioned this pull request Dec 21, 2020
Merged
3 tasks
@nicolo-ribaudo nicolo-ribaudo mentioned this pull request Jan 27, 2021
Merged
JLHwung
JLHwung approved these changes Jan 30, 2021
View reviewed changes
Copy link
Contributor

@JLHwung JLHwung left a comment

Choose a reason for hiding this comment

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

LGTM as long as CI is green.

@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented Jan 30, 2021

I restarted the failing job, it was a network error.

@nicolo-ribaudo nicolo-ribaudo merged commit b63be94 into babel:main Jan 30, 2021
@JLHwung JLHwung mentioned this pull request Feb 8, 2021
Merged
@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label May 2, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@nicolo-ribaudo nicolo-ribaudo nicolo-ribaudo approved these changes

@JLHwung JLHwung JLHwung approved these changes

+2 more reviewers

@merceyz merceyz merceyz left review comments

@AjayPoshak AjayPoshak AjayPoshak left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: Internal 🏠 A type of pull request used for our changelog categories

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Use native ESM for dev scripts

6 participants

@karansapolia @nicolo-ribaudo @babel-bot @JLHwung @merceyz @AjayPoshak