module: do not check circular dependencies for exported proxies

[BridgeAR]

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 ruben@bridgewater.de

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

[ghost]

I don't really like the direction this is going. Even with these changes it still triggers some traps (get for example):

module.exports=newProxy({},{get(){assert.fail();// Throws here}});

But, in general, altering user's objects is really bad practice. I understand the intention is to help users resolve problems regarding circular dependencies, but it brings the cobra effect. Consider the following example:

// Script Amodule.exports=someCondition ? newMyClass() : {};// Script BconstA=require('./A');if(Object.getPrototypeOf(A)!==Object.prototype){// Here we know that `someCondition` is true}else{// Here we know that `someCondition` is false}

It does not work as expected in v14.x. May I suggest removing this entire feature, or at least adding an opt-in flag for that? Most of the users know what they are doing and altering random objects makes a lot of unexpected bugs hard to debug.

[rickyes]

I don't really like the direction this is going. Even with these changes it still triggers some traps (get for example):

module.exports=newProxy({},{get(){assert.fail();// Throws here}});

It does not trigger the get hook to the proxy because there is no access property operation for proxy objects inside internal.

But, in general, altering user's objects is really bad practice. I understand the intention is to help users resolve problems regarding circular dependencies, but it brings the cobra effect. Consider the following example:

// Script Amodule.exports=someCondition ? newMyClass() : {};// Script BconstA=require('./A');if(Object.getPrototypeOf(A)!==Object.prototype){// Here we know that `someCondition` is true}else{// Here we know that `someCondition` is false}

Module A is proxied on loading, but the prototype will point to global.Object.prototype, so it works fine in this case.

[ghost]

It does not trigger the get hook to the proxy because there is no access property operation for proxy objects inside internal.

That is incorrect. It does trigger get trap. Please try it yourself before posting a comment:

// A.jsconstassert=require('assert');module.exports=newProxy({},{get(){assert.fail();}});require('./B');// B.jsrequire('./A');

It triggers the get trap at module.exports.__esModule
(that is internal/modules/cjs/loader.js line 861).

Module A is proxied on loading, but the prototype will point to global.Object.prototype, so it works fine in this case.

That is incorrect. Please try it yourself before posting a comment:

// A.jsconstsomeCondition=false;classMyClass{}module.exports=someCondition ? newMyClass() : {};require('./B');// B.jsconstA=require('./A');if(Object.getPrototypeOf(A)!==Object.prototype){console.log('Oh, something went really bad :/');}else{console.log('OK');}

[addaleax]

LGTM although I’m having mixed feelings, because it seems like we’re working around people’s broken Proxy implementations here.

[addaleax]

I don't really like the direction this is going. Even with these changes it still triggers some traps (get for example):

Yeah, I agree here – there’s no point in doing anything here for Proxy exports 👍

But, in general, altering user's objects is really bad practice.

So is exporting an incomplete Proxy from a module, I would say 🤷‍♀️

May I suggest removing this entire feature, or at least adding an opt-in flag for that?

I understand where you’re coming from, but the example code you posted is contrived, and there’s no reason to expect that it is a common occurrence, whereas this feature has enabled catching real-world bugs multiple times even before it was released. It’s making a trade-off, sure, but I would definitely disagree with the notion that it makes the overall situation worse. What’s more, it tends to improve the situation for people who are less familiar with Node.js, and those who do run into these edge cases with it can be assumed to be familiar enough with JS and Node.js to know how to work with them.

[BridgeAR]

I updated the code to fully work with proxies. It will now just use the target object.

It won't "fix" the prototype example but it does seem contrived as @addaleax pointed out and I don't think we have to fix that.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/31274/

[addaleax]

I updated the code to fully work with proxies.

I would disagree with this – the current state of the PR seems worse than the previous version.

It will now just use the target object.

If the module.exports of a module is a Proxy, there is no point in applying this feature, because there is no way to tell how the Proxy interacts with the target object. Plus, modifying an object that is only reachable as a Proxy target breaks expectations about how Proxy objects` work in general.

[BridgeAR]

I was already pondering if the argument about not telling how the Proxy would interact would be brought up 😄 I am fine with skipping proxies completely as well.

[BridgeAR]

Updated.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/31282/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/31293/

[BridgeAR]

Landed in 24bf1ad

[codebytere]

@BridgeAR should this go back to 12, and if yes, could you please open a manual backport for it? There are a host of conflicts.