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

+import dep from './loader-dep.js';
+import assert from 'assert';
+
+export function resolve (specifier, base, defaultResolve) {


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) { 
        
               const url = getURLFromFilePath(filename); 
        
               const urlString = `${url}`; 
        
               if (ESMLoader.moduleMap.has(urlString) !== true) { 
        
                 const ctx = createDynamicModule(['default'], url); 
        
                 ctx.reflect.exports.default.set(this.exports); 
        
                 ESMLoader.moduleMap.set(urlString, 
        
                                         new ModuleJob(ESMLoader, url, async () => ctx)); 
        
               } else { 
        
                 const job = 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

@@ -2,6 +2,7 @@

 const fs = require('fs');
 const internalCJSModule = require('internal/module');
+const CJSModule = 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

+    const ctx = createDynamicModule(['default'], url, undefined);
+    ctx.reflect.exports.default.set(module.exports);
+    return ctx;
+  }
  return createDynamicModule(['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 {

const job = 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 <targos@protonmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Bradley Farias <bradley.meck@gmail.com>

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

Landed in 1f5ee33.

PR-URL: #17131 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Bradley Farias <bradley.meck@gmail.com>

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 loader Nov 19, 2017

refack changed the title ~~module: Fix for #1730 CJS dependency shared with loader~~ module: Fix for #17130 CJS dependency shared with loader Nov 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 e480ba7 Compare 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 6462cbe Compare November 22, 2017 09:51

module: fix for nodejs#17130 shared loader cjs dep

10a0650

guybedford force-pushed the loader-shared-dep branch from 6462cbe to 10a0650 Compare 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

guybedford commented Nov 19, 2017

TimothyGu Nov 20, 2017

guybedford Nov 21, 2017

joyeecheung commented Nov 21, 2017

guybedford commented Nov 22, 2017 •

edited

Loading

guybedford Nov 22, 2017

jdalton Nov 23, 2017 •

edited

Loading

devsnek Nov 23, 2017 •

edited

Loading

jdalton Nov 23, 2017

devsnek Nov 23, 2017 •

edited

Loading

targos commented Nov 23, 2017

guybedford commented Nov 23, 2017

targos commented Nov 23, 2017

guybedford commented Nov 23, 2017

guybedford commented Nov 23, 2017

guybedford commented Nov 23, 2017

jdalton commented Nov 23, 2017 •

edited

Loading

guybedford commented Nov 24, 2017

guybedford commented Nov 26, 2017

bmeck commented Nov 27, 2017

guybedford commented Nov 27, 2017

guybedford commented Nov 28, 2017

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

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

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

Conversation

guybedford commented Nov 19, 2017

Checklist

Affected core subsystem(s)

TimothyGu Nov 20, 2017

Choose a reason for hiding this comment

guybedford Nov 21, 2017

Choose a reason for hiding this comment

joyeecheung commented Nov 21, 2017

guybedford commented Nov 22, 2017 • edited Loading

guybedford Nov 22, 2017

Choose a reason for hiding this comment

jdalton Nov 23, 2017 • edited Loading

Choose a reason for hiding this comment

devsnek Nov 23, 2017 • edited Loading

Choose a reason for hiding this comment

jdalton Nov 23, 2017

Choose a reason for hiding this comment

devsnek Nov 23, 2017 • edited Loading

Choose a reason for hiding this comment

targos commented Nov 23, 2017

guybedford commented Nov 23, 2017

targos commented Nov 23, 2017

guybedford commented Nov 23, 2017

guybedford commented Nov 23, 2017

guybedford commented Nov 23, 2017

jdalton commented Nov 23, 2017 • edited Loading

guybedford commented Nov 24, 2017

guybedford commented Nov 26, 2017

bmeck commented Nov 27, 2017

guybedford commented Nov 27, 2017

guybedford commented Nov 28, 2017

guybedford commented Nov 22, 2017 •

edited

Loading

jdalton Nov 23, 2017 •

edited

Loading

devsnek Nov 23, 2017 •

edited

Loading

devsnek Nov 23, 2017 •

edited

Loading

jdalton commented Nov 23, 2017 •

edited

Loading