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

Use native ESM for dev scripts #12296

Merged
merged 5 commits into from Jan 30, 2021
Merged

Conversation

karansapolia
Copy link
Contributor

@karansapolia karansapolia commented Nov 2, 2020

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
Gulpfile.mjs Outdated Show resolved Hide resolved
Gulpfile.mjs Outdated Show resolved Hide resolved
Gulpfile.mjs Outdated Show resolved Hide resolved
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.

packages/babel-types/scripts/generateTypeHelpers.mjs Outdated Show resolved Hide resolved
scripts/generators/readmes.js Outdated Show resolved Hide resolved
scripts/parser-tests/utils/parser-test-runner.js Outdated Show resolved Hide resolved
Gulpfile.mjs Outdated Show resolved Hide resolved
@nicolo-ribaudo
Copy link
Member

@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
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

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

packages/babel-types/scripts/generators/docs.mjs Outdated Show resolved Hide resolved
packages/babel-types/scripts/generators/docs.mjs Outdated Show resolved Hide resolved
scripts/utils/formatCode.js Outdated Show resolved Hide resolved
Gulpfile.mjs Outdated Show resolved Hide resolved
Gulpfile.mjs Outdated Show resolved Hide resolved
Gulpfile.mjs Outdated Show resolved Hide resolved
scripts/generators/readmes.js Outdated Show resolved Hide resolved
scripts/generators/readmes.js Outdated Show resolved Hide resolved
@babel-bot
Copy link
Collaborator

babel-bot commented Dec 19, 2020

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

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
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

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

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

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

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

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

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

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

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

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

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

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

@karansapolia
Copy link
Contributor Author

@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.

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

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

@nicolo-ribaudo nicolo-ribaudo merged commit b63be94 into babel:main Jan 30, 2021
@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.
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
Development

Successfully merging this pull request may close these issues.

Use native ESM for dev scripts
6 participants