Skip to content

Commit

Permalink
module: do not check circular dependencies for exported proxies
Browse files Browse the repository at this point in the history
In case the exported module is a proxy that has the `getPrototypeOf`
or `setPrototypeOf` trap, skip the circular dependencies check.
It would otherwise be triggered by the check itself.

Fixes: #33334

Signed-off-by: Ruben Bridgewater <[email protected]>

PR-URL: #33338
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Zeyu Yang <[email protected]>
  • Loading branch information
BridgeAR committed May 14, 2020
1 parent 50ba066 commit 24bf1ad
Show file tree
Hide file tree
Showing 4 changed files with 45 additions and 5 deletions.
18 changes: 13 additions & 5 deletions lib/internal/modules/cjs/loader.js
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,11 @@ const {
CHAR_COLON
} = require('internal/constants');


const {
isProxy
} = require('internal/util/types');

const isWindows = process.platform === 'win32';

const relativeResolveCache = ObjectCreate(null);
Expand Down Expand Up @@ -821,7 +826,7 @@ function emitCircularRequireWarning(prop) {
}

// A Proxy that can be used as the prototype of a module.exports object and
// warns when non-existend properties are accessed.
// warns when non-existent properties are accessed.
const CircularRequirePrototypeWarningProxy = new Proxy({}, {
get(target, prop) {
// Allow __esModule access in any case because it is used in the output
Expand All @@ -840,20 +845,22 @@ const CircularRequirePrototypeWarningProxy = new Proxy({}, {
}
});

// Object.prototype and ObjectProtoype refer to our 'primordials' versions
// Object.prototype and ObjectPrototype 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 &&
!isProxy(module.exports) &&
ObjectGetPrototypeOf(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
// 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.
ObjectSetPrototypeOf(module.exports, CircularRequirePrototypeWarningProxy);
ObjectSetPrototypeOf(
module.exports, CircularRequirePrototypeWarningProxy);
}

return module.exports;
Expand Down Expand Up @@ -943,6 +950,7 @@ Module._load = function(request, parent, isMain) {
}
}
} else if (module.exports &&
!isProxy(module.exports) &&
ObjectGetPrototypeOf(module.exports) ===
CircularRequirePrototypeWarningProxy) {
ObjectSetPrototypeOf(module.exports, PublicObjectPrototype);
Expand Down
17 changes: 17 additions & 0 deletions test/fixtures/cycles/warning-skip-proxy-traps-a.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
module.exports = new Proxy({}, {
get(_target, prop) { throw new Error(`get: ${String(prop)}`); },
getPrototypeOf() { throw new Error('getPrototypeOf'); },
setPrototypeOf() { throw new Error('setPrototypeOf'); },
isExtensible() { throw new Error('isExtensible'); },
preventExtensions() { throw new Error('preventExtensions'); },
getOwnPropertyDescriptor() { throw new Error('getOwnPropertyDescriptor'); },
defineProperty() { throw new Error('defineProperty'); },
has() { throw new Error('has'); },
set() { throw new Error('set'); },
deleteProperty() { throw new Error('deleteProperty'); },
ownKeys() { throw new Error('ownKeys'); },
apply() { throw new Error('apply'); },
construct() { throw new Error('construct'); }
});

require('./warning-skip-proxy-traps-b.js');
10 changes: 10 additions & 0 deletions test/fixtures/cycles/warning-skip-proxy-traps-b.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
const assert = require('assert');

const object = require('./warning-skip-proxy-traps-a.js');

assert.throws(() => {
object.missingPropProxyTrap;
}, {
message: 'get: missingPropProxyTrap',
name: 'Error',
});
5 changes: 5 additions & 0 deletions test/parallel/test-module-circular-dependency-warning.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,3 +38,8 @@ assert.strictEqual(esmTranspiledExport.__esModule, true);
const halfTranspiledExport =
require(fixtures.path('cycles', 'warning-esm-half-transpiled-a.js'));
assert.strictEqual(halfTranspiledExport.__esModule, undefined);

// No circular check is done to prevent triggering proxy traps, if
// module.exports is set to a proxy that contains a `getPrototypeOf` or
// `setPrototypeOf` trap.
require(fixtures.path('cycles', 'warning-skip-proxy-traps-a.js'));

0 comments on commit 24bf1ad

Please sign in to comment.