Skip to content

Commit

Permalink
lib: include cached modules in module.children
Browse files Browse the repository at this point in the history
`module.children` is supposed to be the list of modules included by this
module but lib/module.js failed to update the list when the included
module was retrieved from `Module._cache`.

Fixes: nodejs#7131
PR-URL: nodejs#14132
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
  • Loading branch information
bnoordhuis committed Jul 24, 2017
1 parent 355523f commit c83d9bb
Show file tree
Hide file tree
Showing 5 changed files with 31 additions and 4 deletions.
12 changes: 8 additions & 4 deletions lib/module.js
Original file line number Diff line number Diff line change
Expand Up @@ -46,15 +46,17 @@ function stat(filename) {
}
stat.cache = null;

function updateChildren(parent, child, scan) {
var children = parent && parent.children;
if (children && !(scan && children.includes(child)))
children.push(child);
}

function Module(id, parent) {
this.id = id;
this.exports = {};
this.parent = parent;
if (parent && parent.children) {
parent.children.push(this);
}

updateChildren(parent, this, false);
this.filename = null;
this.loaded = false;
this.children = [];
Expand Down Expand Up @@ -438,6 +440,7 @@ Module._load = function(request, parent, isMain) {

var cachedModule = Module._cache[filename];
if (cachedModule) {
updateChildren(parent, cachedModule, true);
return cachedModule.exports;
}

Expand All @@ -446,6 +449,7 @@ Module._load = function(request, parent, isMain) {
return NativeModule.require(filename);
}

// Don't call updateChildren(), Module constructor already does.
var module = new Module(filename, parent);

if (isMain) {
Expand Down
5 changes: 5 additions & 0 deletions test/fixtures/GH-7131/a.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
'use strict';
require('sys'); // Builtin should not show up in module.children array.
require('./b'); // This should.
require('./b'); // This should not.
module.exports = module.children.slice();
2 changes: 2 additions & 0 deletions test/fixtures/GH-7131/b.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
'use strict';
module.exports = module.children.slice();
13 changes: 13 additions & 0 deletions test/parallel/test-module-children.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
// Flags: --no-deprecation
'use strict';
const common = require('../common');
const assert = require('assert');
const path = require('path');

const dir = path.join(common.fixturesDir, 'GH-7131');
const b = require(path.join(dir, 'b'));
const a = require(path.join(dir, 'a'));

assert.strictEqual(a.length, 1);
assert.strictEqual(b.length, 0);
assert.deepStrictEqual(a[0].exports, b);
3 changes: 3 additions & 0 deletions test/sequential/test-module-loading.js
Original file line number Diff line number Diff line change
Expand Up @@ -237,7 +237,10 @@ try {
// modules that we've required, and that all of them contain
// the appropriate children, and so on.

const visited = new Set();
const children = module.children.reduce(function red(set, child) {
if (visited.has(child)) return set;
visited.add(child);
let id = path.relative(path.dirname(__dirname), child.id);
id = id.replace(backslash, '/');
set[id] = child.children.reduce(red, {});
Expand Down

0 comments on commit c83d9bb

Please sign in to comment.