Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Commit7625dc4

Browse files
joyeecheungmarco-ippolito
authored andcommitted
module: fix submodules loaded by require() and import()
Previously there is an edge case where submodules loaded by require()may not be loaded by import() again from different intermediateedges in the graph. This patch fixes that, added tests, and addeddebug logs.Drive-by: make loader a private field so it doesn't show up in logs.PR-URL:#52487Backport-PR-URL:#53500Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
1 parent6c4f477 commit7625dc4

File tree

8 files changed

+67
-12
lines changed

8 files changed

+67
-12
lines changed

‎lib/internal/modules/esm/loader.js

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -316,7 +316,7 @@ class ModuleLoader {
316316
*@param {string} specifier Specifier of the the imported module.
317317
*@param {string} parentURL Where the import comes from.
318318
*@param {object} importAttributes import attributes from the import statement.
319-
*@returns {ModuleWrap}
319+
*@returns {ModuleJobBase}
320320
*/
321321
getModuleWrapForRequire(specifier,parentURL,importAttributes){
322322
assert(getOptionValue('--experimental-require-module'));
@@ -355,7 +355,7 @@ class ModuleLoader {
355355
// completed (e.g. the require call is lazy) so it's okay. We will return the
356356
// module now and check asynchronicity of the entire graph later, after the
357357
// graph is instantiated.
358-
returnjob.module;
358+
returnjob;
359359
}
360360

361361
defaultLoadSync??=require('internal/modules/esm/load').defaultLoadSync;
@@ -403,7 +403,7 @@ class ModuleLoader {
403403
job=newModuleJobSync(this,url,importAttributes,wrap,isMain,inspectBrk);
404404

405405
this.loadCache.set(url,importAttributes.type,job);
406-
returnjob.module;
406+
returnjob;
407407
}
408408

409409
/**

‎lib/internal/modules/esm/module_job.js

Lines changed: 25 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,9 @@ const {
1818
StringPrototypeSplit,
1919
StringPrototypeStartsWith,
2020
}=primordials;
21+
letdebug=require('internal/util/debuglog').debuglog('esm',(fn)=>{
22+
debug=fn;
23+
});
2124

2225
const{ ModuleWrap, kEvaluated}=internalBinding('module_wrap');
2326

@@ -48,8 +51,7 @@ const isCommonJSGlobalLikeNotDefinedError = (errorMessage) =>
4851
);
4952

5053
classModuleJobBase{
51-
constructor(loader,url,importAttributes,moduleWrapMaybePromise,isMain,inspectBrk){
52-
this.loader=loader;
54+
constructor(url,importAttributes,moduleWrapMaybePromise,isMain,inspectBrk){
5355
this.importAttributes=importAttributes;
5456
this.isMain=isMain;
5557
this.inspectBrk=inspectBrk;
@@ -62,11 +64,13 @@ class ModuleJobBase {
6264
/* A ModuleJob tracks the loading of a single Module, and the ModuleJobs of
6365
* its dependencies, over time. */
6466
classModuleJobextendsModuleJobBase{
67+
#loader=null;
6568
// `loader` is the Loader instance used for loading dependencies.
6669
constructor(loader,url,importAttributes={__proto__:null},
6770
moduleProvider,isMain,inspectBrk,sync=false){
6871
constmodulePromise=ReflectApply(moduleProvider,loader,[url,isMain]);
69-
super(loader,url,importAttributes,modulePromise,isMain,inspectBrk);
72+
super(url,importAttributes,modulePromise,isMain,inspectBrk);
73+
this.#loader=loader;
7074
// Expose the promise to the ModuleWrap directly for linking below.
7175
// `this.module` is also filled in below.
7276
this.modulePromise=modulePromise;
@@ -89,7 +93,8 @@ class ModuleJob extends ModuleJobBase {
8993
// these `link` callbacks depending on each other.
9094
constdependencyJobs=[];
9195
constpromises=this.module.link(async(specifier,attributes)=>{
92-
constjob=awaitthis.loader.getModuleJob(specifier,url,attributes);
96+
constjob=awaitthis.#loader.getModuleJob(specifier,url,attributes);
97+
debug(`async link()${this.url} ->${specifier}`,job);
9398
ArrayPrototypePush(dependencyJobs,job);
9499
returnjob.modulePromise;
95100
});
@@ -121,6 +126,8 @@ class ModuleJob extends ModuleJobBase {
121126
async_instantiate(){
122127
constjobsInGraph=newSafeSet();
123128
constaddJobsToDependencyGraph=async(moduleJob)=>{
129+
debug(`async addJobsToDependencyGraph()${this.url}`,moduleJob);
130+
124131
if(jobsInGraph.has(moduleJob)){
125132
return;
126133
}
@@ -156,7 +163,7 @@ class ModuleJob extends ModuleJobBase {
156163
const{1:childSpecifier,2:name}=RegExpPrototypeExec(
157164
/module'(.*)'doesnotprovideanexportnamed'(.+)'/,
158165
e.message);
159-
const{url:childFileURL}=awaitthis.loader.resolve(
166+
const{url:childFileURL}=awaitthis.#loader.resolve(
160167
childSpecifier,
161168
parentFileUrl,
162169
kEmptyObject,
@@ -167,7 +174,7 @@ class ModuleJob extends ModuleJobBase {
167174
// in the import attributes and some formats require them; but we only
168175
// care about CommonJS for the purposes of this error message.
169176
({ format}=
170-
awaitthis.loader.load(childFileURL));
177+
awaitthis.#loader.load(childFileURL));
171178
}catch{
172179
// Continue regardless of error.
173180
}
@@ -257,18 +264,27 @@ class ModuleJob extends ModuleJobBase {
257264
// All the steps are ensured to be synchronous and it throws on instantiating
258265
// an asynchronous graph.
259266
classModuleJobSyncextendsModuleJobBase{
267+
#loader=null;
260268
constructor(loader,url,importAttributes,moduleWrap,isMain,inspectBrk){
261-
super(loader,url,importAttributes,moduleWrap,isMain,inspectBrk,true);
269+
super(url,importAttributes,moduleWrap,isMain,inspectBrk,true);
262270
assert(this.moduleinstanceofModuleWrap);
271+
this.#loader=loader;
263272
constmoduleRequests=this.module.getModuleRequestsSync();
273+
constlinked=[];
264274
for(leti=0;i<moduleRequests.length;++i){
265275
const{0:specifier,1:attributes}=moduleRequests[i];
266-
constwrap=this.loader.getModuleWrapForRequire(specifier,url,attributes);
276+
constjob=this.#loader.getModuleWrapForRequire(specifier,url,attributes);
267277
constisLast=(i===moduleRequests.length-1);
268278
// TODO(joyeecheung): make the resolution callback deal with both promisified
269279
// an raw module wraps, then we don't need to wrap it with a promise here.
270-
this.module.cacheResolvedWrapsSync(specifier,PromiseResolve(wrap),isLast);
280+
this.module.cacheResolvedWrapsSync(specifier,PromiseResolve(job.module),isLast);
281+
ArrayPrototypePush(linked,job);
271282
}
283+
this.linked=linked;
284+
}
285+
286+
getmodulePromise(){
287+
returnPromiseResolve(this.module);
272288
}
273289

274290
asyncrun(){
Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
// Flags: --experimental-require-module
2+
'use strict';
3+
4+
// This tests that previously synchronously loaded submodule can still
5+
// be loaded by dynamic import().
6+
7+
constcommon=require('../common');
8+
constassert=require('assert');
9+
10+
(async()=>{
11+
constrequired=require('../fixtures/es-modules/require-and-import/load.cjs');
12+
constimported=awaitimport('../fixtures/es-modules/require-and-import/load.mjs');
13+
assert.deepStrictEqual({ ...required},{ ...imported});
14+
})().then(common.mustCall());
Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
// Flags: --experimental-require-module
2+
'use strict';
3+
4+
// This tests that previously asynchronously loaded submodule can still
5+
// be loaded by require().
6+
7+
constcommon=require('../common');
8+
constassert=require('assert');
9+
10+
(async()=>{
11+
constimported=awaitimport('../fixtures/es-modules/require-and-import/load.mjs');
12+
constrequired=require('../fixtures/es-modules/require-and-import/load.cjs');
13+
assert.deepStrictEqual({ ...required},{ ...imported});
14+
})().then(common.mustCall());
Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
module.exports=require('dep');
2+
Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
export*from'dep';
2+

‎test/fixtures/es-modules/require-and-import/node_modules/dep/mod.js

Lines changed: 2 additions & 0 deletions
Some generated files are not rendered by default. Learn more aboutcustomizing how changed files appear on GitHub.

‎test/fixtures/es-modules/require-and-import/node_modules/dep/package.json

Lines changed: 5 additions & 0 deletions
Some generated files are not rendered by default. Learn more aboutcustomizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)
[8]ページ先頭
©2009-2025 Movatter.jp