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

Don't load plugins synchronously in tests #14679

Merged
merged 2 commits into from
Jun 22, 2022

Conversation

nicolo-ribaudo
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo commented Jun 20, 2022

Q                       A
Tests Added + Pass? Yes
License MIT

Ref #13414. When migrating to ESM, plugins must either be pre-loaded before calling transformSync or you must use transformAsync so that Babel can internally use dynamic import.

In most cases this is fine because Babel already runs asynchronously, but there were a few places in our tests that needed to be updated. Note that this is the biggest breaking change of the ESM migration, but:

  • most Babel users don't use Babel directly, and all the popular integrations already call it asynchronously
  • users can either choose to migrate to ESM and hoist plugins to normal import declarations, or they can call Babel asynchronously: since Babel is used to transform files, usually it happens already where there is other i/o (which is often async)
  • if we do Move stable ECMAScript plugins to @babel/core rfcs#10, we could mitigate this change by making sure that at least @babel/preset-env can be used synchronously if necessary

@nicolo-ribaudo nicolo-ribaudo added this to In progress in Move to native ES modules via automation Jun 20, 2022
@nicolo-ribaudo nicolo-ribaudo force-pushed the preload-plugins-sync branch 2 times, most recently from b5eaeec to e76e707 Compare June 20, 2022 21:35
@babel-bot
Copy link
Collaborator

babel-bot commented Jun 20, 2022

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

var innerScope = true;
var res = transform(code, {
return transformAsync(code, {
Copy link
Contributor

Choose a reason for hiding this comment

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

Q: why do we need returning the promise?

Copy link
Member Author

Choose a reason for hiding this comment

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

Otherwise the test runner cannot know if it was resolved successfully or rejected.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we move the runCodeInThisContext wrapper

const src = `(function(exports, require, module, __filename, __dirname, opts) {\n${code}\n});`;

before the Babel transform?

babel.transformAsync(execCode, execOpts),

So that we don't have to change test options and return in exec.js is automatically valid.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a good idea, but there are manylre tests that return a promise and thus I'd prefer to do it as a separate PR.

@nicolo-ribaudo nicolo-ribaudo merged commit 87f26d5 into babel:main Jun 22, 2022
Move to native ES modules automation moved this from In progress to Done Jun 22, 2022
@nicolo-ribaudo nicolo-ribaudo deleted the preload-plugins-sync branch June 22, 2022 19:33
@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 Sep 22, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 22, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: tests outdated A closed issue/PR that is archived due to age. Recommended to make a new issue
Development

Successfully merging this pull request may close these issues.

None yet

4 participants