diff --git a/benchmark/module/module-loader-circular.js b/benchmark/module/module-loader-circular.js new file mode 100644 index 00000000000000..6d392e0e1908db --- /dev/null +++ b/benchmark/module/module-loader-circular.js @@ -0,0 +1,34 @@ +'use strict'; +const fs = require('fs'); +const path = require('path'); +const common = require('../common.js'); + +const tmpdir = require('../../test/common/tmpdir'); +const benchmarkDirectory = + path.resolve(tmpdir.path, 'benchmark-module-circular'); + +const bench = common.createBenchmark(main, { + n: [1e4] +}); + +function main({ n }) { + tmpdir.refresh(); + + const aDotJS = path.join(benchmarkDirectory, 'a.js'); + const bDotJS = path.join(benchmarkDirectory, 'b.js'); + + fs.mkdirSync(benchmarkDirectory); + fs.writeFileSync(aDotJS, 'require("./b.js");'); + fs.writeFileSync(bDotJS, 'require("./a.js");'); + + bench.start(); + for (let i = 0; i < n; i++) { + require(aDotJS); + require(bDotJS); + delete require.cache[aDotJS]; + delete require.cache[bDotJS]; + } + bench.end(n); + + tmpdir.refresh(); +} diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index 3580614481acc2..d7ec3dff92f0c8 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -656,6 +656,52 @@ Module._resolveLookupPaths = function(request, parent) { return parentDir; }; +function emitCircularRequireWarning(prop) { + process.emitWarning( + `Accessing non-existent property '${String(prop)}' of module exports ` + + 'inside circular dependency', + 'Warning', + undefined, // code + undefined, // ctor + true); // emit now +} + +// A Proxy that can be used as the prototype of a module.exports object and +// warns when non-existend properties are accessed. +const CircularRequirePrototypeWarningProxy = new Proxy({}, { + get(target, prop) { + if (prop in target) return target[prop]; + emitCircularRequireWarning(prop); + return undefined; + }, + + getOwnPropertyDescriptor(target, prop) { + if (ObjectPrototype.hasOwnProperty(target, prop)) + return Object.getOwnPropertyDescriptor(target, prop); + emitCircularRequireWarning(prop); + return undefined; + } +}); + +// Object.prototype and ObjectProtoype refer to our 'primordials' versions +// and are not identical to the versions on the global object. +const PublicObjectPrototype = global.Object.prototype; + +function getExportsForCircularRequire(module) { + if (module.exports && + Object.getPrototypeOf(module.exports) === PublicObjectPrototype && + // Exclude transpiled ES6 modules / TypeScript code because those may + // employ unusual patterns for accessing 'module.exports'. That should be + // okay because ES6 modules have a different approach to circular + // dependencies anyway. + !module.exports.__esModule) { + // This is later unset once the module is done loading. + Object.setPrototypeOf(module.exports, CircularRequirePrototypeWarningProxy); + } + + return module.exports; +} + // Check the cache for the requested file. // 1. If a module already exists in the cache: return its exports object. // 2. If the module is native: call @@ -676,6 +722,8 @@ Module._load = function(request, parent, isMain) { const cachedModule = Module._cache[filename]; if (cachedModule !== undefined) { updateChildren(parent, cachedModule, true); + if (!cachedModule.loaded) + return getExportsForCircularRequire(cachedModule); return cachedModule.exports; } delete relativeResolveCache[relResolveCacheIdentifier]; @@ -687,6 +735,8 @@ Module._load = function(request, parent, isMain) { const cachedModule = Module._cache[filename]; if (cachedModule !== undefined) { updateChildren(parent, cachedModule, true); + if (!cachedModule.loaded) + return getExportsForCircularRequire(cachedModule); return cachedModule.exports; } @@ -728,6 +778,10 @@ Module._load = function(request, parent, isMain) { if (parent !== undefined) { delete relativeResolveCache[relResolveCacheIdentifier]; } + } else if (module.exports && + Object.getPrototypeOf(module.exports) === + CircularRequirePrototypeWarningProxy) { + Object.setPrototypeOf(module.exports, PublicObjectPrototype); } } diff --git a/test/fixtures/cycles/warning-a.js b/test/fixtures/cycles/warning-a.js new file mode 100644 index 00000000000000..dea4c53a267af6 --- /dev/null +++ b/test/fixtures/cycles/warning-a.js @@ -0,0 +1 @@ +require('./warning-b.js'); diff --git a/test/fixtures/cycles/warning-b.js b/test/fixtures/cycles/warning-b.js new file mode 100644 index 00000000000000..3be73bfd0af264 --- /dev/null +++ b/test/fixtures/cycles/warning-b.js @@ -0,0 +1,3 @@ +const a = require('./warning-a.js'); +a.missingPropB; +a[Symbol('someSymbol')]; diff --git a/test/fixtures/cycles/warning-esm-transpiled-a.js b/test/fixtures/cycles/warning-esm-transpiled-a.js new file mode 100644 index 00000000000000..fe70e745d38e08 --- /dev/null +++ b/test/fixtures/cycles/warning-esm-transpiled-a.js @@ -0,0 +1,2 @@ +Object.defineProperty(exports, "__esModule", { value: true }); +require('./warning-esm-transpiled-b.js'); diff --git a/test/fixtures/cycles/warning-esm-transpiled-b.js b/test/fixtures/cycles/warning-esm-transpiled-b.js new file mode 100644 index 00000000000000..a44a63ce2c95b3 --- /dev/null +++ b/test/fixtures/cycles/warning-esm-transpiled-b.js @@ -0,0 +1 @@ +require('./warning-esm-transpiled-a.js').missingPropESM; diff --git a/test/fixtures/cycles/warning-moduleexports-a.js b/test/fixtures/cycles/warning-moduleexports-a.js new file mode 100644 index 00000000000000..b37504b1b8ad51 --- /dev/null +++ b/test/fixtures/cycles/warning-moduleexports-a.js @@ -0,0 +1,2 @@ +module.exports = {}; +require('./warning-moduleexports-b.js'); diff --git a/test/fixtures/cycles/warning-moduleexports-b.js b/test/fixtures/cycles/warning-moduleexports-b.js new file mode 100644 index 00000000000000..8d2292934d2d64 --- /dev/null +++ b/test/fixtures/cycles/warning-moduleexports-b.js @@ -0,0 +1 @@ +require('./warning-moduleexports-b.js').missingPropModuleExportsB; diff --git a/test/fixtures/cycles/warning-moduleexports-class-a.js b/test/fixtures/cycles/warning-moduleexports-class-a.js new file mode 100644 index 00000000000000..e23654d7f36c64 --- /dev/null +++ b/test/fixtures/cycles/warning-moduleexports-class-a.js @@ -0,0 +1,11 @@ +const assert = require('assert'); + +class Parent {} +class A extends Parent {} + +module.exports = A; +require('./warning-moduleexports-class-b.js'); +process.nextTick(() => { + assert.strictEqual(module.exports, A); + assert.strictEqual(Object.getPrototypeOf(module.exports), Parent); +}); diff --git a/test/fixtures/cycles/warning-moduleexports-class-b.js b/test/fixtures/cycles/warning-moduleexports-class-b.js new file mode 100644 index 00000000000000..3fe1763eb7256f --- /dev/null +++ b/test/fixtures/cycles/warning-moduleexports-class-b.js @@ -0,0 +1 @@ +require('./warning-moduleexports-class-a.js').missingPropModuleExportsClassB; diff --git a/test/parallel/test-module-circular-dependency-warning.js b/test/parallel/test-module-circular-dependency-warning.js new file mode 100644 index 00000000000000..10893978829ff5 --- /dev/null +++ b/test/parallel/test-module-circular-dependency-warning.js @@ -0,0 +1,33 @@ +'use strict'; + +const common = require('../common'); +const assert = require('assert'); +const fixtures = require('../common/fixtures'); + +common.expectWarning({ + Warning: [ + ["Accessing non-existent property 'missingPropB' " + + 'of module exports inside circular dependency'], + ["Accessing non-existent property 'Symbol(someSymbol)' " + + 'of module exports inside circular dependency'], + ["Accessing non-existent property 'missingPropModuleExportsB' " + + 'of module exports inside circular dependency'] + ] +}); +const required = require(fixtures.path('cycles', 'warning-a.js')); +assert.strictEqual(Object.getPrototypeOf(required), Object.prototype); + +const requiredWithModuleExportsOverridden = + require(fixtures.path('cycles', 'warning-moduleexports-a.js')); +assert.strictEqual(Object.getPrototypeOf(requiredWithModuleExportsOverridden), + Object.prototype); + +// If module.exports is not a regular object, no warning should be emitted. +const classExport = + require(fixtures.path('cycles', 'warning-moduleexports-class-a.js')); +assert.strictEqual(Object.getPrototypeOf(classExport).name, 'Parent'); + +// If module.exports.__esModule is set, no warning should be emitted. +const esmTranspiledExport = + require(fixtures.path('cycles', 'warning-esm-transpiled-a.js')); +assert.strictEqual(esmTranspiledExport.__esModule, true);