feat: use require to load esm #5366
Conversation
|
|
|
friendly ping @JoshuaKGoldberg |
|
Hey thanks for the ping! I was out for a bit and am catching up on things now. I'm going to try to get to testing this thoroughly this week (hopefully tomorrow or so). But the more folks who can test this out, the better. Note that we're pretty close to releasing Mocha 12 (#5357). I also have a vague memory of someone mentioning somewhere that once Mocha drops support for pre- IMO it might make sense to either...
I'm personally leaning to ⚡ since this change is pretty targeted, and because of how broken things are at the moment for Node.js versions with type stripping. cc @mochajs/committers - WDYT? |
|
Oh and @legendecas in the meantime, could you please try to fix the lint error? It's failing in CI. |
This allows compilers based on `require.extensions` continue to work.
|
Fixed lint issue. "Merge this in as a feature in Mocha 11.x, then take on the drastic simplification in Mocha 12" sounds good to me! |
There was a problem hiding this comment.
💯 Fantastic! Thanks for sending this in, it's really clean and I haven't been able to break it locally when testing on a bunch of real-world repos.
We should definitely get another review from @mochajs/committers if possible just to be safe. But no blockers from me on merging to main in v11.
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for loading ESM syntax via Node’s require(esm) feature while preserving require.extensions hooks, and provides integration tests for both .js and .ts compilers.
- Introduce
tryImportAndRequireandrequireModuleinesm-utils.jsto choose betweenimport()andrequire()based onprocess.features.require_module. - Add integration tests and fixtures under
test/compiler-cjsandtest/compiler-fixturesto validaterequire.extensions-based compilers. - Update ESLint config to include the new test file.
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| lib/nodejs/esm-utils.js | Replace single requireOrImport with two strategies and conditional override |
| test/integration/compiler-cjs.spec.js | Add tests for .js and .ts extensions using fixtures |
| test/compiler-fixtures/js.fixture.js | Fixture to override .js loader and compile files |
| test/compiler-fixtures/ts.fixture.js | Fixture to override .ts loader and compile files |
| test/compiler-cjs/test.js | ESM source file for .js test |
| test/compiler-cjs/test.js.compiled | Compiled CJS output for .js test |
| test/compiler-cjs/test.ts | ESM source file for .ts test |
| test/compiler-cjs/test.ts.compiled | Compiled CJS output for .ts test |
| eslint.config.js | Include the newly added test/compiler-cjs/test.js in linting |
Comments suppressed due to low confidence (4)
lib/nodejs/esm-utils.js:86
- It looks like the closing
};for the originalexports.requireOrImportarrow function is left behind after replacing it withtryImportAndRequire. Consider removing this stray closing brace and semicolon to avoid a syntax error.
};
test/compiler-cjs/test.ts:3
- [nitpick] The test title uses
ctswhich can be confusing—consider renaming tots written in esmfor clarity and consistency with the file extension.
describe('cts written in esm', () => {
test/compiler-cjs/test.ts.compiled:3
- [nitpick] The compiled test also uses
ctsin its description; update this tots written in esmto match the source and avoid confusion.
describe('cts written in esm', () => {
test/integration/compiler-cjs.spec.js:58
- [nitpick] This expectation references
cts written in esm; after renaming the test titles tots written in esm, update this expectation string accordingly.
'cts written in esm should work',
PR Checklist
status: accepting prsOverview
This allows compilers based on
require.extensionscontinue to work, likets-nodewith Node.js built-in TypeScript support enabled.Hopefully unblock nodejs/node#57298.
Fixes: #5314
Fixes: #5317