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

esm: do not call getSource when format is commonjs #50465

Merged
merged 6 commits into from
Nov 29, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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);
}