GeezerVison

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

module.exports = new Proxy({}, {
  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 A
module.exports = someCondition ? new MyClass() : {};

// Script B
const A = 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.

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

module.exports = new Proxy({}, {
  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 A
module.exports = someCondition ? new MyClass() : {};

// Script B
const A = 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.

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.js
const assert = require('assert');
module.exports = new Proxy({}, {
  get(){
    assert.fail();
  }
});
require('./B');

// B.js
require('./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.js
const someCondition = false;
class MyClass{}
module.exports = someCondition ? new MyClass() : {};
require('./B');

// B.js
const A = require('./A');
if (Object.getPrototypeOf(A) !== Object.prototype) {
  console.log('Oh, something went really bad :/');
} else {
  console.log('OK');
}

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.

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.

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

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.

Updated.

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

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

Landed in 24bf1ad

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