Skip to content

Run Babel asynchronously in fixtures#14659

Merged
nicolo-ribaudo merged 4 commits intobabel:mainfrom
nicolo-ribaudo:run-fixtures-async
Jun 15, 2022
Merged

Run Babel asynchronously in fixtures#14659
nicolo-ribaudo merged 4 commits intobabel:mainfrom
nicolo-ribaudo:run-fixtures-async

Conversation

@nicolo-ribaudo
Copy link
Copy Markdown
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
Copy Markdown
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
Copy link
Copy Markdown
Member Author

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

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

Maybe we can use child processes!
😉

@nicolo-ribaudo
Copy link
Copy Markdown
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
@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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants