Skip to content

Commit

Permalink
esm: do not call getSource when format is commonjs
Browse files Browse the repository at this point in the history
Ensure that `defaultLoad` does not uselessly access the file system to
get the source of modules that are known to be in CommonJS format.

This allows CommonJS imports to resolve in the current phase of the
event loop.

Refs: eslint/eslint#17683
PR-URL: #50465
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
  • Loading branch information
fasttime authored and UlisesGascon committed Dec 11, 2023
1 parent fb3a618 commit 8feb2db
Show file tree
Hide file tree
Showing 6 changed files with 69 additions and 9 deletions.
19 changes: 10 additions & 9 deletions lib/internal/modules/esm/load.js
Original file line number Diff line number Diff line change
Expand Up @@ -132,20 +132,21 @@ async function defaultLoad(url, context = kEmptyObject) {
if (urlInstance.protocol === 'node:') {
source = null;
format ??= 'builtin';
} else {
let contextToPass = context;
} else if (format !== 'commonjs' || defaultType === 'module') {
if (source == null) {
({ responseURL, source } = await getSource(urlInstance, context));
contextToPass = { __proto__: context, source };
context = { __proto__: context, source };
}

// Now that we have the source for the module, run `defaultGetFormat` again in case we detect ESM syntax.
format ??= await defaultGetFormat(urlInstance, contextToPass);
if (format == null) {
// Now that we have the source for the module, run `defaultGetFormat` to detect its format.
format = await defaultGetFormat(urlInstance, context);

if (format === 'commonjs' && contextToPass !== context && defaultType !== 'module') {
// For backward compatibility reasons, we need to discard the source in
// order for the CJS loader to re-fetch it.
source = null;
if (format === 'commonjs') {
// For backward compatibility reasons, we need to discard the source in
// order for the CJS loader to re-fetch it.
source = null;
}
}
}

Expand Down
19 changes: 19 additions & 0 deletions test/es-module/test-esm-dynamic-import-commonjs.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
'use strict';

const common = require('../common');
const fixtures = require('../common/fixtures');
const assert = require('node:assert');

(async () => {

// Make sure that the CommonJS module lexer has been initialized.
// See https://github.com/nodejs/node/blob/v21.1.0/lib/internal/modules/esm/translators.js#L61-L81.
await import(fixtures.fileURL('empty.js'));

let tickDuringCJSImport = false;
process.nextTick(() => { tickDuringCJSImport = true; });
await import(fixtures.fileURL('empty.cjs'));

assert(!tickDuringCJSImport);

})().then(common.mustCall());
9 changes: 9 additions & 0 deletions test/es-module/test-esm-dynamic-import-commonjs.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
import '../common/index.mjs';
import fixtures from '../common/fixtures.js';
import assert from 'node:assert';

let tickDuringCJSImport = false;
process.nextTick(() => { tickDuringCJSImport = true; });
await import(fixtures.fileURL('empty.cjs'));

assert(!tickDuringCJSImport);
10 changes: 10 additions & 0 deletions test/es-module/test-esm-loader-with-source.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
// Flags: --experimental-loader ./test/fixtures/es-module-loaders/preset-cjs-source.mjs
import '../common/index.mjs';
import * as fixtures from '../common/fixtures.mjs';
import assert from 'assert';

const { default: existingFileSource } = await import(fixtures.fileURL('es-modules', 'cjs-file.cjs'));
const { default: noSuchFileSource } = await import(new URL('./no-such-file.cjs', import.meta.url));

assert.strictEqual(existingFileSource, 'no .cjs file was read to get this source');
assert.strictEqual(noSuchFileSource, 'no .cjs file was read to get this source');
Empty file added test/fixtures/empty.cjs
Empty file.
21 changes: 21 additions & 0 deletions test/fixtures/es-module-loaders/preset-cjs-source.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
export function resolve(specifier, context, next) {
if (specifier.endsWith('/no-such-file.cjs')) {
// Shortcut to avoid ERR_MODULE_NOT_FOUND for non-existing file, but keep the url for the load hook.
return {
shortCircuit: true,
url: specifier,
};
}
return next(specifier);
}

export function load(href, context, next) {
if (href.endsWith('.cjs')) {
return {
format: 'commonjs',
shortCircuit: true,
source: 'module.exports = "no .cjs file was read to get this source";',
};
}
return next(href);
}

0 comments on commit 8feb2db

Please sign in to comment.