module: Fix for #17130 CJS dependency shared with loader#17131

guybedford · 2017-11-19T12:34:30Z

This fixes the issue of a CJS dependency loaded by the loader then not being defined when imported.

The reason for this is that CJS modules define themselves early into the loader on execution, instead of as a result of being imported by the loader. But this defining only happens the first time they are loaded. Because we use a different loader for the loader itself, the modules weren't being redefined, which has been fixed here.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

esmodules

TimothyGu · 2017-11-20T23:49:04Z

test/fixtures/es-module-loaders/loader-shared-dep.mjs

Nit: no space before (

Thanks, fixed up.

joyeecheung · 2017-11-21T20:59:44Z

CI: https://ci.nodejs.org/job/node-test-pull-request/11598/

guybedford · 2017-11-22T10:11:21Z

This failed the snapshot test unfortunately... I've adjusted the fix to deal with the snapshotting edge cases.

The snapshot test case is basically there to ensure the CJS module is its module.exports value at exactly the time of execution, and not after any further module.exports reassignments, as would be given if we just used the value in the require cache normally. That is why there is this injection of the CJS module into the loader at

node/lib/module.js

Lines 538 to 552 in 9c6f6b0

if(ESMLoader){
consturl=getURLFromFilePath(filename);
consturlString=`${url}`;
if(ESMLoader.moduleMap.has(urlString)!==true){
constctx=createDynamicModule(['default'],url);
ctx.reflect.exports.default.set(this.exports);
ESMLoader.moduleMap.set(urlString,
newModuleJob(ESMLoader,url,async()=>ctx));
}else{
constjob=ESMLoader.moduleMap.get(urlString);
if(job.reflect)
job.reflect.exports.default.set(this.exports);
}
}
};

instead of just using the require() value in the loader itself

The fix here is to specifically catch the case of any modules loaded before the loader itself was initialized, based on using the module from the require cache if it is loaded there.

guybedford · 2017-11-22T10:12:49Z

lib/internal/loader/ModuleRequest.js


 constfs=require('fs');
 constinternalCJSModule=require('internal/module');
+constCJSModule=require('module');


I've also hoisted up this require by fixing the circular reference in module.js.

jdalton · 2017-11-23T03:45:17Z

lib/internal/loader/ModuleRequest.js

+returnctx;
+}
 returncreateDynamicModule(['default'],url,(reflect)=>{
 debug(`Loading CJSModule ${url}`);


How come reflect isn't used inside the createDynamicModule callback for 'cjs'...
reflect.exports.default.set(exports);
...like it is for others and the cached case above 👆
(I'm not familiar with how things work with the loader)

using _load calls around a bunch and eventually ends up here
node/lib/module.js
Lines 577 to 581 in 317f596
}else{
constjob=ESMLoader.moduleMap.get(urlString);
if(job.reflect)
job.reflect.exports.default.set(this.exports);
}

@devsnek Ah, any way that could be tidied up so that all the reflect.exports.default.set is done near each other or at least in the same file?

i think this is already as tidy as it will get, but i think a comment above line 49 would go a long way towards explaining what is going on. personally i had no idea how line 49 worked until when you asked because i decided it was time to figure it out. maybe something like we don't assign using reflect here Module.prototype.load handles setting it.

targos · 2017-11-23T14:24:20Z

New CI: https://ci.nodejs.org/job/node-test-pull-request/11668/

guybedford · 2017-11-23T16:43:27Z

Thanks @targos. Hmm, CI seems to be failing on node-test-binary-windows but not node-compile-windows. Does anyone know what the difference is between those environments?

targos · 2017-11-23T17:00:02Z

node-compile-windows compiles node.exe and then node-test-binary-windows takes it to run the tests in parallel on multiple machines.

guybedford · 2017-11-23T17:02:05Z

Ah right, so it's as simple as a Windows failure. Thanks, will get the local test going here.

guybedford · 2017-11-23T19:18:19Z

I've added the windows fix - was pretty much as to be expected.

CI: https://ci.nodejs.org/job/node-test-pull-request/11677/

guybedford · 2017-11-23T19:29:35Z

@jdalton the function at https://github.com/nodejs/node/blob/master/lib/internal/url.js#L1317 could possibly be updated to convert to windows slashes.

jdalton · 2017-11-23T22:30:16Z

@guybedford

the function at https://github.com/nodejs/node/blob/master/lib/internal/url.js#L1317 could possibly be updated to convert to windows slashes.

Ya, it should totally be updated. Node has methods to handle this without the need for one-off hacks.

guybedford · 2017-11-24T11:56:25Z

@jasnell do you think it would make sense to have internalURLModule.getPathFromURL to do / to \ conversion in Windows? The case here would benefit from the consistency, as otherwise we need to do this as a custom step in https://github.com/nodejs/node/pull/17131/files#diff-c5e21b512cde1c24955c7231ebf52254R44.

guybedford · 2017-11-26T17:13:24Z

//cc @bmeck if you have a moment to review.

bmeck · 2017-11-27T13:04:37Z

This looks fine to me. I need to think on things a bit, but see no blockers.

I am wondering if we should move the module map to be based upon v8::Context rather than per loader and notice that #17153 also changes timing of snapshot when this PR is introduced.

It might be more consistent overall to use a separate Module Map during loader initialization but... well that can be done separately to this.

guybedford · 2017-11-27T15:06:35Z

Thanks @bmeck. I'll aim to merge this tomorrow then. I'm sure these edge cases will change over time, but it would be good to get the bug fix in sooner rather than later.

PR-URL: #17131 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Bradley Farias <[email protected]>

guybedford · 2017-11-28T10:30:23Z

Landed in 1f5ee33.

PR-URL: #17131 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Bradley Farias <[email protected]>

guybedford mentioned this pull request Nov 19, 2017
Imported modules are undefined if also used inside loader #17130
Closed

mscdex added the esm Issues and PRs related to the ECMAScript Modules implementation. label Nov 19, 2017

guybedford changed the title ~~Fix for #1730 CJS dependency shared with loader~~module: Fix for #1730 CJS dependency shared with loaderNov 19, 2017

refack changed the title ~~module: Fix for #1730 CJS dependency shared with loader~~module: Fix for #17130 CJS dependency shared with loaderNov 19, 2017

targos approved these changes Nov 19, 2017
View reviewed changes

cjihrig approved these changes Nov 20, 2017
View reviewed changes

jasnell approved these changes Nov 20, 2017
View reviewed changes

TimothyGu approved these changes Nov 20, 2017
View reviewed changes

guybedford force-pushed the loader-shared-dep branch from 18037ac to e480ba7Compare November 21, 2017 16:55

joyeecheung added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 21, 2017

guybedford force-pushed the loader-shared-dep branch 2 times, most recently from 8acb1e2 to 6462cbeCompare November 22, 2017 09:51

module: fix fornodejs#17130shared loader cjs dep
10a0650

guybedford force-pushed the loader-shared-dep branch from 6462cbe to 10a0650Compare November 22, 2017 10:01

guybedford commented Nov 22, 2017
View reviewed changes

jdalton reviewed Nov 23, 2017
View reviewed changes

devsnek approved these changes Nov 23, 2017
View reviewed changes

fix windows cache path lookup
ffe969a

bmeck approved these changes Nov 27, 2017
View reviewed changes

guybedford closed this Nov 28, 2017

TimothyGu removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 29, 2017

MylesBorins mentioned this pull request Dec 12, 2017
v9.3.0 proposal #17631
Merged

gibfahn added dont-land-on-v6.x labels Dec 19, 2017

gibfahn mentioned this pull request Dec 20, 2017
v8.9.4 proposal #17772
Closed

gibfahn mentioned this pull request Dec 20, 2017
v8.9.4 proposal #17774
Merged

	}else{
	constjob=ESMLoader.moduleMap.get(urlString);
	if(job.reflect)
	job.reflect.exports.default.set(this.exports);
	}

Uh oh!

module: Fix for #17130 CJS dependency shared with loader#17131

module: Fix for #17130 CJS dependency shared with loader #17131

Uh oh!

Conversation

guybedford commented Nov 19, 2017

Checklist

Affected core subsystem(s)

Uh oh!

TimothyGuNov 20, 2017

Choose a reason for hiding this comment

Uh oh!

guybedfordNov 21, 2017

Choose a reason for hiding this comment

Uh oh!

joyeecheung commented Nov 21, 2017

Uh oh!

guybedford commented Nov 22, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

guybedfordNov 22, 2017

Choose a reason for hiding this comment

Uh oh!

jdaltonNov 23, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devsnekNov 23, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jdaltonNov 23, 2017

Choose a reason for hiding this comment

Uh oh!

devsnekNov 23, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

targos commented Nov 23, 2017

Uh oh!

guybedford commented Nov 23, 2017

Uh oh!

targos commented Nov 23, 2017

Uh oh!

guybedford commented Nov 23, 2017

Uh oh!

guybedford commented Nov 23, 2017

Uh oh!

guybedford commented Nov 23, 2017

Uh oh!

jdalton commented Nov 23, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

guybedford commented Nov 24, 2017

Uh oh!

guybedford commented Nov 26, 2017

Uh oh!

bmeck commented Nov 27, 2017

Uh oh!

guybedford commented Nov 27, 2017

Uh oh!

guybedford commented Nov 28, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

guybedford commented Nov 22, 2017•
edited
Loading

jdaltonNov 23, 2017•
edited
Loading

devsnekNov 23, 2017•
edited
Loading

devsnekNov 23, 2017•
edited
Loading

jdalton commented Nov 23, 2017•
edited
Loading