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

nicolo-ribaudo commented Jun 13, 2022

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

@liuxingbaoyu
Copy link
Contributor

liuxingbaoyu commented Jun 13, 2022

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

nicolo-ribaudo commented Jun 13, 2022

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
Contributor

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 Jun 14, 2022
@nicolo-ribaudo
Copy link
Member Author

nicolo-ribaudo commented Jun 14, 2022

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
Contributor

liuxingbaoyu commented Jun 14, 2022

Maybe we can use child processes!
😉

@nicolo-ribaudo
Copy link
Member Author

nicolo-ribaudo commented Jun 14, 2022

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

@nicolo-ribaudo nicolo-ribaudo merged commit 0c3371c into babel:main Jun 15, 2022
35 checks passed
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 Jun 15, 2022
nicolo-ribaudo added a commit to nicolo-ribaudo/babel that referenced this issue 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: tests PR: Internal 🏠
Development

Successfully merging this pull request may close these issues.

None yet

4 participants