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.

Sign up for GitHub

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

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

Closed
wants to merge 2 commits into from
+28 −3
Closed

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

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
10a0650
module: fix for #17130 shared loader cjs dep
guybedford Nov 22, 2017
ffe969a
fix windows cache path lookup
guybedford Nov 23, 2017
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
13 changes: 11 additions & 2 deletions lib/internal/loader/ModuleRequest.js
View file
Original file line number Diff line number Diff line change
@@ -2,6 +2,7 @@

const fs = require('fs');
const internalCJSModule = require('internal/module');
const CJSModule = require('module');
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const internalURLModule = require('internal/url');
const internalFS = require('internal/fs');
const NativeModule = require('native_module');
@@ -35,11 +36,19 @@ loaders.set('esm', async (url) => {
});

// Strategy for loading a node-style CommonJS module
const isWindows = process.platform === 'win32';
const winSepRegEx = /\//g;
loaders.set('cjs', async (url) => {
const pathname = internalURLModule.getPathFromURL(new URL(url));
const module = CJSModule._cache[
isWindows ? pathname.replace(winSepRegEx, '\\') : pathname];
if (module && module.loaded) {
const ctx = createDynamicModule(['default'], url, undefined);
ctx.reflect.exports.default.set(module.exports);
return ctx;
}
return createDynamicModule(['default'], url, (reflect) => {
debug(`Loading CJSModule ${url}`);
Copy link
Member

@jdalton jdalton Nov 23, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

@devsnek devsnek Nov 23, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Member

@devsnek devsnek Nov 23, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

const CJSModule = require('module');
const pathname = internalURLModule.getPathFromURL(new URL(url));
CJSModule._load(pathname);
});
});
4 changes: 3 additions & 1 deletion lib/module.js
View file
Original file line number Diff line number Diff line change
@@ -39,6 +39,9 @@ const experimentalModules = !!process.binding('config').experimentalModules;

const errors = require('internal/errors');

module.exports = Module;

// these are below module.exports for the circular reference
const Loader = require('internal/loader/Loader');
const ModuleJob = require('internal/loader/ModuleJob');
const { createDynamicModule } = require('internal/loader/ModuleWrap');
@@ -72,7 +75,6 @@ function Module(id, parent) {
this.loaded = false;
this.children = [];
}
module.exports = Module;

Module._cache = Object.create(null);
Module._pathCache = Object.create(null);
7 changes: 7 additions & 0 deletions test/es-module/test-esm-shared-loader-dep.mjs
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
// Flags: --experimental-modules --loader ./test/fixtures/es-module-loaders/loader-shared-dep.mjs
/* eslint-disable required-modules */
import assert from 'assert';
import './test-esm-ok.mjs';
import dep from '../fixtures/es-module-loaders/loader-dep.js';

assert.strictEqual(dep.format, 'esm');
7 changes: 7 additions & 0 deletions test/fixtures/es-module-loaders/loader-shared-dep.mjs
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import dep from './loader-dep.js';
import assert from 'assert';

export function resolve(specifier, base, defaultResolve) {
assert.equal(dep.format, 'esm');
return defaultResolve(specifier, base);
}