Skip to content

Commit d55c3e7

Browse files
joyeecheungrichardlau
authored andcommitted
esm: link modules synchronously when no async loader hooks are used
When no async loader hooks are registered, perform the linking as synchronously as possible to reduce the chance of races from the the shared module loading cache. PR-URL: #59519 Fixes: #59366 Refs: https://github.com/abejfehr/node-22.18-issue-repro Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Filip Skokan <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]>
1 parent 9e1fbb6 commit d55c3e7

File tree

5 files changed

+25
-15
lines changed

5 files changed

+25
-15
lines changed

lib/internal/modules/esm/loader.js

Lines changed: 13 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,8 @@ let debug = require('internal/util/debuglog').debuglog('esm', (fn) => {
6363
debug = fn;
6464
});
6565

66+
const { isPromise } = require('internal/util/types');
67+
6668
/**
6769
* @typedef {import('./hooks.js').HooksProxy} HooksProxy
6870
* @typedef {import('./module_job.js').ModuleJobBase} ModuleJobBase
@@ -582,15 +584,21 @@ class ModuleLoader {
582584

583585
/**
584586
* Load a module and translate it into a ModuleWrap for ordinary imported ESM.
585-
* This is run asynchronously.
587+
* This may be run asynchronously if there are asynchronous module loader hooks registered.
586588
* @param {string} url URL of the module to be translated.
587589
* @param {object} loadContext See {@link load}
588590
* @param {boolean} isMain Whether the module to be translated is the entry point.
589-
* @returns {Promise<ModuleWrap>}
591+
* @returns {Promise<ModuleWrap>|ModuleWrap}
590592
*/
591-
async loadAndTranslate(url, loadContext, isMain) {
592-
const { format, source } = await this.load(url, loadContext);
593-
return this.#translate(url, format, source, isMain);
593+
loadAndTranslate(url, loadContext, isMain) {
594+
const maybePromise = this.load(url, loadContext);
595+
const afterLoad = ({ format, source }) => {
596+
return this.#translate(url, format, source, isMain);
597+
};
598+
if (isPromise(maybePromise)) {
599+
return maybePromise.then(afterLoad);
600+
}
601+
return afterLoad(maybePromise);
594602
}
595603

596604
/**

lib/internal/modules/esm/module_job.js

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@ const {
3737
},
3838
} = internalBinding('util');
3939
const { decorateErrorStack, kEmptyObject } = require('internal/util');
40+
const { isPromise } = require('internal/util/types');
4041
const {
4142
getSourceMapsSupport,
4243
} = require('internal/source_map/source_map_cache');
@@ -135,12 +136,11 @@ class ModuleJob extends ModuleJobBase {
135136
this.#loader = loader;
136137

137138
// Expose the promise to the ModuleWrap directly for linking below.
138-
if (isForRequireInImportedCJS) {
139-
this.module = moduleOrModulePromise;
140-
assert(this.module instanceof ModuleWrap);
141-
this.modulePromise = PromiseResolve(this.module);
142-
} else {
139+
if (isPromise(moduleOrModulePromise)) {
143140
this.modulePromise = moduleOrModulePromise;
141+
} else {
142+
this.module = moduleOrModulePromise;
143+
this.modulePromise = PromiseResolve(moduleOrModulePromise);
144144
}
145145

146146
// Promise for the list of all dependencyJobs.

test/es-module/test-esm-error-cache.js

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,9 @@ let error;
1919
await assert.rejects(
2020
() => import(file),
2121
(e) => {
22-
assert.strictEqual(error, e);
22+
// The module may be compiled again and a new SyntaxError would be thrown but
23+
// with the same content.
24+
assert.deepStrictEqual(error, e);
2325
return true;
2426
}
2527
);

test/es-module/test-esm-import-attributes-errors.js

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ async function test() {
2828

2929
await rejects(
3030
import(jsModuleDataUrl, { with: { type: 'json', other: 'unsupported' } }),
31-
{ code: 'ERR_IMPORT_ATTRIBUTE_TYPE_INCOMPATIBLE' }
31+
{ code: 'ERR_IMPORT_ATTRIBUTE_UNSUPPORTED' }
3232
);
3333

3434
await rejects(
@@ -48,7 +48,7 @@ async function test() {
4848

4949
await rejects(
5050
import(jsonModuleDataUrl, { with: { foo: 'bar' } }),
51-
{ code: 'ERR_IMPORT_ATTRIBUTE_MISSING' }
51+
{ code: 'ERR_IMPORT_ATTRIBUTE_UNSUPPORTED' }
5252
);
5353

5454
await rejects(

test/es-module/test-esm-import-attributes-errors.mjs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ await rejects(
2323

2424
await rejects(
2525
import(jsModuleDataUrl, { with: { type: 'json', other: 'unsupported' } }),
26-
{ code: 'ERR_IMPORT_ATTRIBUTE_TYPE_INCOMPATIBLE' }
26+
{ code: 'ERR_IMPORT_ATTRIBUTE_UNSUPPORTED' }
2727
);
2828

2929
await rejects(
@@ -43,7 +43,7 @@ await rejects(
4343

4444
await rejects(
4545
import(jsonModuleDataUrl, { with: { foo: 'bar' } }),
46-
{ code: 'ERR_IMPORT_ATTRIBUTE_MISSING' }
46+
{ code: 'ERR_IMPORT_ATTRIBUTE_UNSUPPORTED' }
4747
);
4848

4949
await rejects(

0 commit comments

Comments
 (0)