test: add esm loader test #5383
Conversation
|
Hmm, it seems like the new test fails starting with Node.js v22.12.0 (which should be addressed by the revert). |
|
@legendecas Can we add the tests independently of #5381? |
There was a problem hiding this comment.
Pull Request Overview
This PR adds tests for validating the support of ESM module hooks and loader compilers, including tests to cover both standard and top‐level await functionality. Key changes include:
- Adding integration tests for files with .js and .ts extensions.
- Creating new ESM loader fixture files and corresponding compiled test files.
- Updating ESLint configuration to support top-level await in the ESM test files.
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/integration/compiler-esm.spec.js | New integration tests for ESM module hook compilers using .js/.ts test files. |
| test/compiler-fixtures/esm.fixture.js | Fixture that registers the ESM loader via Module.register. |
| test/compiler-fixtures/esm-loader.fixture.mjs | Defines the custom ESM loader for reading compiled fixture files. |
| test/compiler-esm/test.ts.compiled | Compiled TS test file validating ESM behavior. |
| test/compiler-esm/test.ts | Source TS test file for ESM. |
| test/compiler-esm/test.js.compiled | Compiled JS test file validating ESM behavior. |
| test/compiler-esm/test.js | Source JS test file for ESM. |
| test/compiler-esm/test-tla.ts.compiled | Compiled TS test file with top-level await to validate ESM TLA behavior. |
| test/compiler-esm/test-tla.ts | Source TS test file with top-level await feature. |
| test/compiler-esm/test-tla.js.compiled | Compiled JS test file with top-level await. |
| test/compiler-esm/test-tla.js | Source JS test file with top-level await. |
| test/compiler-esm/package.json | Package file setting the module type to ESM for tests. |
| eslint.config.js | ESLint config update to handle top-level await in ESM files. |
Comments suppressed due to low confidence (1)
test/compiler-esm/test-tla.ts.compiled:12
- Add a comment explaining the purpose of 'await undefined' to clarify that it is intentionally used to test top-level await functionality.
await undefined;
There was a problem hiding this comment.
Thanks, this looks solid to me!
I think this PR in conjunction with #5384 is a very good solution to the introduced 11.17.0 issues.
This comment was marked as outdated.
This comment was marked as outdated.
|
We can ignore the compliance bot, it's just reporting that this doesn't fix a linked issue. This is fine. |
|
I added two new test cases to cover CJS format with module loaders.. |
cd7fc27 to
a9dbb2d
Compare
|
Fantastic, even better! I'm ready to merge this whenever you are. 👍 Edit: assuming all test failures are also happening on |
a9dbb2d to
16d24fb
Compare
|
Seems like the new tests fails on v18. I'm going to split them up to a follow-up PR. |
|
This should be ready to be reviewed again. |
|
This test breaks Node 18 compatibility. Module.register doesn't exist until Node 20 and then it's a candidate feature. |
What do you mean? |
Module.register doesn't exist until Node 20 and then it's a candidate feature. You're relying on effectively experimental APIs that don't even exist in the set of Node versions in package.json->engines |
It's in 18: https://nodejs.org/docs/latest-v18.x/api/module.html#moduleregisterspecifier-parenturl-options. We can also tell it's in 18 because the integration tests on this PR, then on
Well, at the time of authoring in 18 and 20, they were. But now that 18 is EOL and 20 is no longer under active development, they're roughly stable. It's quite unlikely that 20 would take in a change that alters |
PR Checklist
status: accepting prsOverview
Add module loader test.