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

Run Babel asynchronously in fixtures #14659

Merged
merged 4 commits into from Jun 15, 2022

Conversation

nicolo-ribaudo
Copy link
Member

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

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

This was extracted from #13414: by running our tests asynchronously, they can also run ESM plugins. The second commit verifies that it works by converting an existing test-only plugin from CJS to ESM.

@nicolo-ribaudo nicolo-ribaudo added the PR: Internal 🏠 A type of pull request used for our changelog categories label Jun 11, 2022
@babel-bot
Copy link
Collaborator

babel-bot commented Jun 11, 2022

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

@nicolo-ribaudo nicolo-ribaudo added this to In progress in Move to native ES modules via automation Jun 12, 2022
@nicolo-ribaudo
Copy link
Member Author

The failure is related, it looks like our logic to detect if dynamic import is supported was broken 😬

@liuxingbaoyu
Copy link
Member

I remember having a similar problem when I was investigating jest-runner, you can try eval("import()").
Because it is a keyword.
😄

@nicolo-ribaudo
Copy link
Member Author

Yes, but import is in a different file that we require in a try/catch block, so if it was a parsing error would be suppressed and correctly handled 🤔

@liuxingbaoyu
Copy link
Member

liuxingbaoyu commented Jun 14, 2022

This is related to jest-runner, but not only a jest-runner problem, our supportsESM does not actually call to test whether import() is available, so it will throw an exception when used.

Oh no, node crashes when we actually call it.

@nicolo-ribaudo nicolo-ribaudo force-pushed the run-fixtures-async branch 2 times, most recently from 343cabf to 22bcbfa Compare June 14, 2022 09:10
@nicolo-ribaudo
Copy link
Member Author

I'm so annoyed that we have to hard-code the Node.js version, but there is no other synchronous way to know if dynamic import is supported in the current Node.js version. I used this code to test:

// demo.js
console.log("syntax is ok");
import("./.eslintrc.cjs").then(console.log, console.error);

@liuxingbaoyu
Copy link
Member

Maybe we can use child processes!
😉

@nicolo-ribaudo
Copy link
Member Author

Probably it's not worth it for this simple check 😬

@nicolo-ribaudo nicolo-ribaudo merged commit 0c3371c into babel:main Jun 15, 2022
Move to native ES modules automation moved this from In progress to Done Jun 15, 2022
@nicolo-ribaudo nicolo-ribaudo deleted the run-fixtures-async branch June 15, 2022 02:51
nicolo-ribaudo added a commit to nicolo-ribaudo/babel that referenced this pull request Jun 18, 2022
* Run Babel asynchronously in fixtures

* Move `checkScopeInfo` to test folder, convert to ESM

* Fix Node.js 8

* Avoid dynamic import feature detection
@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 14, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 14, 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 PR: Internal 🏠 A type of pull request used for our changelog categories
Development

Successfully merging this pull request may close these issues.

None yet

4 participants