Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proxy bug? #31989

Closed
tylerlong opened this issue Feb 27, 2020 · 21 comments
Closed

Proxy bug? #31989

tylerlong opened this issue Feb 27, 2020 · 21 comments
Labels
question Issues that look for answers. util Issues and PRs related to the built-in util module.

Comments

@tylerlong
Copy link

const util = require('util')

const a = { b: 1 }

const p = new Proxy(a, { get: (target, prop, receiver) => {
  if(prop === util.inspect.custom) {
    return () => 'Hello world'
  }
  return target[prop]
}})

console.log(p)

Code above will output Hello world in node 10.x

It will output { b: 1 } in node 13.x

I am not sure this is by design a breaking change or a bug.

@tylerlong
Copy link
Author

I've found something: #26241

@devsnek
Copy link
Member

devsnek commented Feb 27, 2020

(wrong button)

I recently ran into this problem too. For people actually using proxies to do useful things, this is really annoying. I ended up putting a custom inspect function on the proxy target, but that is not always possible if you don't own the proxy target.

@devsnek devsnek added feature request Issues that request new features to be added to Node.js. util Issues and PRs related to the built-in util module. labels Feb 27, 2020
@Trott
Copy link
Member

Trott commented Feb 27, 2020

@BridgeAR

@BridgeAR
Copy link
Member

BridgeAR commented Feb 28, 2020

This is working as intended. Inspecting something should be side effect free. It was "by chance" that the specific trap was triggered with that key in Node.js 10.x. Browsers do not trigger any traps either and it caused all kinds of difficulties for people inspecting proxies.

Using the proxy target is exactly how it should be done in such cases. That way it is always explicit and does not rely on a implementation detail.

@BridgeAR BridgeAR added question Issues that look for answers. and removed feature request Issues that request new features to be added to Node.js. labels Feb 28, 2020
@BridgeAR
Copy link
Member

@tylerlong you can achieve what you want by writing:

const util = require('util')

const a = { b: 1 }

const p = new Proxy(a, { get: (target, prop, receiver) => {
  // Do things
  return target[prop]
}})
p[util.inspect.custom] = () => 'Hello world!'

console.log(p)

@BridgeAR
Copy link
Member

@tylerlong @devsnek I hope the example above helps :) if there are any other use cases that you want to look into, please let me know.

@devsnek
Copy link
Member

devsnek commented Feb 28, 2020

I realize this was an intentional design choice, but it's very annoying if your proxy is being used to say, mock language features. I know that my proxies are not doing things that would be considered side effects to the consumers of them, and yet when they log them they see the wrong thing. Perhaps we can have a way to tag proxies or something?

@bnoordhuis
Copy link
Member

Perhaps we can have a way to tag proxies or something?

That reintroduces the problem that buggy proxies cause all kinds of hard to debug issues.

@BridgeAR
Copy link
Member

@devsnek

I know that my proxies are not doing things that would be considered side effects to the consumers of them, and yet when they log them they see the wrong thing.

To me this sounds like it would be ideal to add the custom inspect function on each proxy instance before returning it to the user.

I am going to close this, since it's not clear what traps a user might want to get invoked and which ones not and there does not seem to be a way to always get this right.

@devsnek devsnek reopened this Feb 28, 2020
@devsnek
Copy link
Member

devsnek commented Feb 28, 2020

@BridgeAR proxies don't have own properties, and you don't always own the proxy target, like I said above.

@BridgeAR
Copy link
Member

@devsnek it would be possible to use the proxy as an indirection to the actual target in such cases (by defining a "redirectTarget" with the properties that should be special handled). That would not be ideal, since all traps have to properly redirect but it should work.

@devsnek
Copy link
Member

devsnek commented Feb 28, 2020

@BridgeAR that's actually not possible, proxy traps perform certain verifications on the underlying target as they're called, so you need them to line up properly.

@tylerlong
Copy link
Author

tylerlong commented Mar 2, 2020

@tylerlong you can achieve what you want by writing:

const util = require('util')

const a = { b: 1 }

const p = new Proxy(a, { get: (target, prop, receiver) => {
  // Do things
  return target[prop]
}})
p[util.inspect.custom] = () => 'Hello world!'

console.log(p)

I wrote a library to provide a proxy. I am only in charge of the proxy. The actual object is out of my control so I cannot change it.

@BridgeAR
Copy link
Member

BridgeAR commented Mar 3, 2020

The original behavior was relying on side effects of an implementation detail and it caused many issues that were partially difficult to work around. I do not think this is something we should support.

A hackish way (that will mostly work for most things that can be done with an object) to implement what you want is to have a redirection as I mentioned above:

const foreign = { a: true };
const redirectTarget = {
  [util.inspect.custom]() {
    return [1, 2, 3];
  }
};

const handler = {
  getPrototypeOf() { return Object.getPrototypeOf(foreign); },
  setPrototypeOf(source, target) { return Object.setPrototypeOf(foreign, target); },
  preventExtensions() { return Object.preventExtensions(foreign); },
  getOwnPropertyDescriptor(target, prop) { return Object.getOwnPropertyDescriptor(foreign, prop); },
  defineProperty(target, prop, value) { return Object.defineProperty(foreign, prop, value); },
  has(target, prop) { return prop in foreign; },
  get(target, prop) {
    if (prop === util.inspect.custom) {
      return target[prop]
    }
    return foreign[prop]
  },
  set(target, prop, value) { return foreign[prop] = value; },
  deleteProperty(target, prop) { return delete foreign[prop]; },
  ownKeys() { return Reflect.ownKeys(foreign); }, // This is tricky, since it's not clear what exactly should be done
  // etc...
};

const proxy = new Proxy(redirectTarget, handler)

console.log(proxy)

@tylerlong
Copy link
Author

tylerlong commented Mar 3, 2020

I don't get the point that you said it is "side effect".

"proxy traps being triggered by .inspect()" so that it is side effect?

proxy traps also triggered by simply getting a property, when don't you consider it a side effect?

const p = new Proxy(a, { get: (target, prop, receiver) => {
  //  do some side effect
  return target[prop]
}})

I don't think triggering an event is side effect.

Side effect is in the event handling code written by end developers. And it is by design for business requirements.

@BridgeAR

@BridgeAR
Copy link
Member

@tylerlong using util.inspect() is meant to have no effect on the object that is inspected. It should just plainly print the string, no matter if there's a proxy or not. The proxy should not be aware if specific properties are accessed during the inspection.

I recommend to add a custom inspect function in case you want to know when that is triggered. So far it was "by chance" that accessing the custom inspect function would trigger the proxy trap and that was fixed.

Maybe you could describe what you try to achieve to find out if there's an alternative way to implement the same.

@tylerlong
Copy link
Author

tylerlong commented May 12, 2020

using util.inspect() is meant to have no effect on the object that is inspected. It should just plainly print the string, no matter if there's a proxy or not. The proxy should not be aware if specific properties are accessed during the inspection.

I still don't get the logic. If the logic is correct, the following should also be correct:

Getting a property is meant to have no effect on the object that is being accessed. It should just plainly return the value, no matter if there's a proxy or not. The proxy should not be aware if specific properties are accessed during property getting process.

BUT, if the goal is to make the behavior identical to browsers', it makes sense. I don't want to start a debate on this. If browser behavior is this, then I will just accept it.

@tylerlong
Copy link
Author

tylerlong commented May 12, 2020

Maybe you could describe what you try to achieve to find out if there's an alternative way to implement the same.

I created a library which is "smart". The internal implementation is kind of complicated but let me simplify the use case:

It provides a method called autoRun which monitor an object o and a function f(). If o changes and it will affect the result of f(), f() will be auto invoked, a.k.a autoRun.

In order to implement it, I need to know how f() is accessing o:

  • If the f() doesn't access o at all, no matter how o changes, f() will never be invoked
  • If the f() accesses o.a, f() will be invoked if o.a changes
  • If the f() accesses o.b, f() will be invoked if o.b changes

Now the question is: if f() inspects o, how do I know that? example:

const f = o => {
   console.log(o)
}

In such case, no matter how o changes, f() will not be invoked at all, because the library doesn't know o's change will affect f()'s result.

I recommend to add a custom inspect function in case you want to know when that is triggered.

I cannot do this, because what I created is a library. I have no control over o and how f() is going to access o at all.

With the details explained, I don't believe there is a solution.

@BridgeAR
Copy link
Member

@tylerlong how does inspection change o? That should not be the case at all. It is still possible to track all user actions on the actual object e.g.:

const util = require('util')

const userObject = {
  b: 1,
  [util.inspect.custom]() {
    this.foo = true;
    this.b === 1
  }
}

const proxiedObject = new Proxy(userObject, {
  get(target, prop) {
    console.log('GET', prop)
    return target[prop]
  },
  set() { console.log('SET') }
})

util.inspect(proxiedObject)

This would still trigger the get and set traps.

There is indeed probably very little that could be done to improve the current situation. I am therefore closing this. If anyone has a solution, please open a new issue, reopen or leave a comment.

@tylerlong
Copy link
Author

tylerlong commented Jul 5, 2020

@BridgeAR Thanks

The sample code you posted makes sense. The only disadvantage is: it is changing the target object directly (adding [util.inspect.custom]() to target object).

My original plan was to add the logic to handler object. I need a trap to know that an object is inspected. And the trap now has to be installed onto the target object, sadly. It defeats the purpose of Proxy if a trap need to be installed onto target object directly. I know most of other traps still works as before (get, set...etc). But now we have this exception.

@BridgeAR
Copy link
Member

BridgeAR commented Jul 5, 2020

@tylerlong my example was meant as coming from the user as is. I wanted to indicate that if the user accesses or changes properties from inside a user defined custom inspect function, it would still be noticeable by the proxy while the object is inspected.

Please be aware that the user could also use util.inspect(proxy, { customInspect: false }). That would prevent calling custom inspect functions.

In your example above you note the following:

If o changes and it will affect the result of f(), f() will be auto invoked, a.k.a autoRun.

This still holds with the current inspection: if o is inspected and it has no custom inspect function and no getter/setter o won't ever change during inspection. Proxy traps are triggered, if a custom inspect function or a getter/setter is defined and they are called in a way that o is changed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Issues that look for answers. util Issues and PRs related to the built-in util module.
Projects
None yet
Development

No branches or pull requests

5 participants