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 deleteProperty has no receiver #1198

Closed
WebReflection opened this Issue May 21, 2018 · 12 comments

Comments

Projects
None yet
4 participants
@WebReflection

WebReflection commented May 21, 2018

There is a Chromium bug for this:
https://bugs.chromium.org/p/chromium/issues/detail?id=844554

And a codepen live example that shows the issue:
https://codepen.io/WebReflection/pen/qYLrNx?editors=0010

Summary

While get and set traps exposes the proxy/receiver, the deleteProperty doesn't and this makes it impossible to use proxies with instances.

const wm = new WeakMap;
class Test {
  constructor() {
    const proxy = new Proxy(this, handler);
    wm.set(proxy, []);
    return proxy;
  }
  method() {
    // the context here is the proxy/receiver, not its target
    wm.get(this).push(`invoked method`);
  }
}

const handler = {
  get(target, key, receiver) {
    wm.get(receiver).push(`get ${key}`);
    return target[key];
  },
  set(target, key, value, receiver) {
    wm.get(receiver).push(`set ${key} as ${value}`);
    target[key] = value;
    return true;
  },
  deleteProperty(target, key) {
    // how am I supposed to log/track the remote property
    // since there is no receiver in here ?
    wm.get({what: '???'}).push(`deleted ${key}`);
    delete target[key];
    return true;
  }
};

Reason

Having a proxy around a generic instance could be used to track remotely instance changes/updates/logs but without a receiver in the deleteProperty one need to add to the WeakMap both the proxy and the instance itself so that wm.get(target) inside the deleteProperty trap would bring in the same Array if the constructor also relate wm.set(this, wm.get(proxy)) (or just using same array).

The fact such workaround is needed suggests the deleteTrap could simply add a third argument and expose the proxy like every other method does.

Thanks for considering this fix.

@ljharb

This comment has been minimized.

Show comment
Hide comment
@ljharb
Member

ljharb commented May 21, 2018

@bathos

This comment has been minimized.

Show comment
Hide comment
@bathos

bathos May 21, 2018

Contributor

I’ve also run into this, and wished that all trap handlers received a reference to the proxy as an argument.

Since terminology can be confusing with Proxies I would clarify though that receiver is specific to the [[Get]] and [[Set]] internal operations; unlike [[Delete]], whose "receiver" would always be the proxy by definition, the receiver in [[Get]] and [[Set]] is unique in that it may be a remote object that just inherits from proxy (receiver here is the context accessor functions would be called against). If the proxy were added as an argument for the [[Delete]] trap (as well as any other), this would be a novel thing and would break an existing pattern: right now, the arguments for the handlers mirror those of the internal methods being intercepted, which in turn are reflected by Reflect.

Contributor

bathos commented May 21, 2018

I’ve also run into this, and wished that all trap handlers received a reference to the proxy as an argument.

Since terminology can be confusing with Proxies I would clarify though that receiver is specific to the [[Get]] and [[Set]] internal operations; unlike [[Delete]], whose "receiver" would always be the proxy by definition, the receiver in [[Get]] and [[Set]] is unique in that it may be a remote object that just inherits from proxy (receiver here is the context accessor functions would be called against). If the proxy were added as an argument for the [[Delete]] trap (as well as any other), this would be a novel thing and would break an existing pattern: right now, the arguments for the handlers mirror those of the internal methods being intercepted, which in turn are reflected by Reflect.

@WebReflection

This comment has been minimized.

Show comment
Hide comment
@WebReflection

WebReflection May 21, 2018

@bathos thanks for expanding, but the fact there's no way to know which proxy was asked to delete a property for its target is super annoying and inconsistent, IMO.

Also, from user-land perspective, since there is no third argument, this cannot possibly break anything if/once implemented and be also easily feature detected.

I wish we can make proxies better suitable for classes and wrapped instances in the near future, 'cause having to double relate through the weak map both proxy and instance feels plain wrong.

WebReflection commented May 21, 2018

@bathos thanks for expanding, but the fact there's no way to know which proxy was asked to delete a property for its target is super annoying and inconsistent, IMO.

Also, from user-land perspective, since there is no third argument, this cannot possibly break anything if/once implemented and be also easily feature detected.

I wish we can make proxies better suitable for classes and wrapped instances in the near future, 'cause having to double relate through the weak map both proxy and instance feels plain wrong.

@bathos

This comment has been minimized.

Show comment
Hide comment
@bathos

bathos May 21, 2018

Contributor

@WebReflection Agreed, I’m also in favor of somehow getting a ref to the proxy in there. I mentioned the stuff about the existing pattern because I suspect some might consider this asymmetry a downside. However if proxy were added as a final arg to all the handler functions, I’d think that remains sufficiently predictable.

Contributor

bathos commented May 21, 2018

@WebReflection Agreed, I’m also in favor of somehow getting a ref to the proxy in there. I mentioned the stuff about the existing pattern because I suspect some might consider this asymmetry a downside. However if proxy were added as a final arg to all the handler functions, I’d think that remains sufficiently predictable.

@tvcutsem

This comment has been minimized.

Show comment
Hide comment
@tvcutsem

tvcutsem May 21, 2018

First, as @bathos correctly indicated: receiver is special to get/set only due to inheritance. For all other operations, the receiver is basically always the proxy itself. So it does not make sense to add a receiver parameter to just the deleteProperty trap.

However, it is certainly possible to add a reference to the proxy object itself as an argument to every trap (for the get/set traps, receiver may then be equal to the new proxy argument in certain cases).

While adding this extra proxy argument is certainly possible, there are a few drawbacks. First and foremost, one reason we avoided passing in a reference to the proxy in every trap is that it makes it very easy to get into infinite loops (if the handler touches the proxy, it will cause the handler to fire again, which touches the proxy again, etc. etc.)

Second, usually every proxy corresponds one-to-one with its target object, so if you want to keep state associated with the proxy's identity, you can often just as well associate that state with the target. In your original example, you then always do the lookup in the WeakMap based on target, never on receiver. This only becomes a problem when you want to use multiple proxies for the same target, and each proxy needs different private state, but in that case the recommended pattern is to create a separate handler instance per proxy, and to store the state inside the handler object.

Finally, in the example above, it seems this inside method() will be bound to the proxy object. You should think carefully if this is really what you want. Almost all the time you want this to refer to the target object inside the original object's methods. That does mean you need to return a bound method from the get trap.

tvcutsem commented May 21, 2018

First, as @bathos correctly indicated: receiver is special to get/set only due to inheritance. For all other operations, the receiver is basically always the proxy itself. So it does not make sense to add a receiver parameter to just the deleteProperty trap.

However, it is certainly possible to add a reference to the proxy object itself as an argument to every trap (for the get/set traps, receiver may then be equal to the new proxy argument in certain cases).

While adding this extra proxy argument is certainly possible, there are a few drawbacks. First and foremost, one reason we avoided passing in a reference to the proxy in every trap is that it makes it very easy to get into infinite loops (if the handler touches the proxy, it will cause the handler to fire again, which touches the proxy again, etc. etc.)

Second, usually every proxy corresponds one-to-one with its target object, so if you want to keep state associated with the proxy's identity, you can often just as well associate that state with the target. In your original example, you then always do the lookup in the WeakMap based on target, never on receiver. This only becomes a problem when you want to use multiple proxies for the same target, and each proxy needs different private state, but in that case the recommended pattern is to create a separate handler instance per proxy, and to store the state inside the handler object.

Finally, in the example above, it seems this inside method() will be bound to the proxy object. You should think carefully if this is really what you want. Almost all the time you want this to refer to the target object inside the original object's methods. That does mean you need to return a bound method from the get trap.

@WebReflection

This comment has been minimized.

Show comment
Hide comment
@WebReflection

WebReflection May 21, 2018

you can often just as well associate that state with the target

you cannot, because as shown/explained methods are triggered through the proxy, as context, and not its target, so for classes this is a bummer.

Finally, in the example above, it seems this inside method() will be bound to the proxy object. You should think carefully if this is really what you want.

Yes, that's exactly what I want, relate the proxy I own and create, and handle every case that goes through it.

Almost all the time you want this to refer to the target object inside the original object's methods.

Well, no. I'd like to preserve original JavaScript behavior when you can simply borrow methods around.

I have a very specific use case that is hacky to implement with current specs and requires unnecessary work arounds, while an extra proxy reference would perfectly, and better, fix all my needs.

Returning a bound method will fail expectations from users of the class, theoretically unaware of the fact the instance is a Proxy, not its target, and the returned method is always bound (including the fact I'd need another reference to the bound method so that instance.method === instance.method).

That does mean you need to return a bound method from the get trap.

This is again a workaround, hence a limitation of the current Proxy specification when it comes to classes.

WebReflection commented May 21, 2018

you can often just as well associate that state with the target

you cannot, because as shown/explained methods are triggered through the proxy, as context, and not its target, so for classes this is a bummer.

Finally, in the example above, it seems this inside method() will be bound to the proxy object. You should think carefully if this is really what you want.

Yes, that's exactly what I want, relate the proxy I own and create, and handle every case that goes through it.

Almost all the time you want this to refer to the target object inside the original object's methods.

Well, no. I'd like to preserve original JavaScript behavior when you can simply borrow methods around.

I have a very specific use case that is hacky to implement with current specs and requires unnecessary work arounds, while an extra proxy reference would perfectly, and better, fix all my needs.

Returning a bound method will fail expectations from users of the class, theoretically unaware of the fact the instance is a Proxy, not its target, and the returned method is always bound (including the fact I'd need another reference to the bound method so that instance.method === instance.method).

That does mean you need to return a bound method from the get trap.

This is again a workaround, hence a limitation of the current Proxy specification when it comes to classes.

@WebReflection

This comment has been minimized.

Show comment
Hide comment
@WebReflection

WebReflection May 21, 2018

P.S. I'd be more than OK having an extra proxy argument per every single Proxy callback ... it looks like a no brainer, and whoever wants to play harakiri with infinite loops can do that, same way while(true) exists and it's not specifications fault :-)

WebReflection commented May 21, 2018

P.S. I'd be more than OK having an extra proxy argument per every single Proxy callback ... it looks like a no brainer, and whoever wants to play harakiri with infinite loops can do that, same way while(true) exists and it's not specifications fault :-)

@WebReflection

This comment has been minimized.

Show comment
Hide comment
@WebReflection

WebReflection May 21, 2018

P.S.2 relating the target or binding it to its own methods, also means exposing the target directly in case some method invokes some argument through this which is very undesired if you proxy an instance in the constructor and you want to be sure that outside the class any consumer deals with the proxy only, and not, directly, the instance.

Thanks for thinking this through and considering an improvement over current situation.

edit as proof of concept of current situation and possible side effects on binding methods to the target.

const actions = new WeakMap;
const proxies = new WeakMap;

const handler = {
  get(target, key, receiver) {
    actions.get(receiver)
            .push(`get ${key}`);
    return target[key];
  },
  set(target, key, value, receiver) {
    actions.get(receiver)
            .push(`set ${key} => ${value}`);
    return true;
  },
  deleteProperty(target, key) {
    actions.get(proxies.get(target))
            .push(`delete ${key}`);
    return true;
  }
};

class Trackable {
  constructor() {
    const proxy = new Proxy(this, handler);
    actions.set(proxy, []);
    proxies.set(this, proxy);
    return proxy;
  }
  method() {
    if (actions.has(this))
      actions.get(this).push(`method()`);
  }
}

class Test extends Trackable {
  method(options) {
    super.method();
    // if this was bound, it'd expose the target
    // if this is not bound, everything is fine
    options.invoke.call(this);
  }
}

WebReflection commented May 21, 2018

P.S.2 relating the target or binding it to its own methods, also means exposing the target directly in case some method invokes some argument through this which is very undesired if you proxy an instance in the constructor and you want to be sure that outside the class any consumer deals with the proxy only, and not, directly, the instance.

Thanks for thinking this through and considering an improvement over current situation.

edit as proof of concept of current situation and possible side effects on binding methods to the target.

const actions = new WeakMap;
const proxies = new WeakMap;

const handler = {
  get(target, key, receiver) {
    actions.get(receiver)
            .push(`get ${key}`);
    return target[key];
  },
  set(target, key, value, receiver) {
    actions.get(receiver)
            .push(`set ${key} => ${value}`);
    return true;
  },
  deleteProperty(target, key) {
    actions.get(proxies.get(target))
            .push(`delete ${key}`);
    return true;
  }
};

class Trackable {
  constructor() {
    const proxy = new Proxy(this, handler);
    actions.set(proxy, []);
    proxies.set(this, proxy);
    return proxy;
  }
  method() {
    if (actions.has(this))
      actions.get(this).push(`method()`);
  }
}

class Test extends Trackable {
  method(options) {
    super.method();
    // if this was bound, it'd expose the target
    // if this is not bound, everything is fine
    options.invoke.call(this);
  }
}
@bathos

This comment has been minimized.

Show comment
Hide comment
@bathos

bathos May 21, 2018

Contributor

FWIW @WebReflection I would point out that there are, presently, some alternatives to the double WeakMap solution:

Create the handlers in the constructor

The (potential) disadvantage is the fact that the handler functions are always created anew.

class Foo {
  constructor() {
    const proxy = new Proxy(this, {
      deleteProperty: (target, key) => {
        console.log({ proxy });
        return Reflect.deleteProperty(target, key);
      }
    });

    wm.set(proxy, []);
    return proxy;
  }
}

Make the proxy available to the handlers without creating new functions

This is possible because handlers are called against the handler object. So whatever you make available there is accessible as/via this.

const handlersProto = {
  deleteProperty(target, key) {
    console.log({ proxy: this.proxy });
    return Reflect.deleteProperty(target, key);
  }
};

class Foo {
  constructor() {
    const handlers = Object.create(handlersProto);
    const proxy = handlers.proxy = new Proxy(this, handlers);

    wm.set(proxy, []);
    return proxy;
  }
}
Contributor

bathos commented May 21, 2018

FWIW @WebReflection I would point out that there are, presently, some alternatives to the double WeakMap solution:

Create the handlers in the constructor

The (potential) disadvantage is the fact that the handler functions are always created anew.

class Foo {
  constructor() {
    const proxy = new Proxy(this, {
      deleteProperty: (target, key) => {
        console.log({ proxy });
        return Reflect.deleteProperty(target, key);
      }
    });

    wm.set(proxy, []);
    return proxy;
  }
}

Make the proxy available to the handlers without creating new functions

This is possible because handlers are called against the handler object. So whatever you make available there is accessible as/via this.

const handlersProto = {
  deleteProperty(target, key) {
    console.log({ proxy: this.proxy });
    return Reflect.deleteProperty(target, key);
  }
};

class Foo {
  constructor() {
    const handlers = Object.create(handlersProto);
    const proxy = handlers.proxy = new Proxy(this, handlers);

    wm.set(proxy, []);
    return proxy;
  }
}
@WebReflection

This comment has been minimized.

Show comment
Hide comment
@WebReflection

WebReflection May 21, 2018

@bathos I've edited my previous post to show how articulated could be the current status, but I have to admit the last option you gave me is quite possibly the best workaround.

It still bothers me we are showcasing workarounds for something otherwise straight forward if we had the proxy passed along to proxy handlers we create ourselves, for proxies we own.

Yet I think I'd use that prototypal solution in the future, thanks!

WebReflection commented May 21, 2018

@bathos I've edited my previous post to show how articulated could be the current status, but I have to admit the last option you gave me is quite possibly the best workaround.

It still bothers me we are showcasing workarounds for something otherwise straight forward if we had the proxy passed along to proxy handlers we create ourselves, for proxies we own.

Yet I think I'd use that prototypal solution in the future, thanks!

@tvcutsem

This comment has been minimized.

Show comment
Hide comment
@tvcutsem

tvcutsem May 22, 2018

tvcutsem commented May 22, 2018

@WebReflection

This comment has been minimized.

Show comment
Hide comment
@WebReflection

WebReflection May 22, 2018

I think this should be labelled as wont fix then, so that others passing by might as well know what's best as workaround.

const handlersProto = {
  deleteProperty(target, key) {
    console.log({ proxy: this.proxy });
    return Reflect.deleteProperty(target, key);
  }
};

class Foo {
  constructor() {
    const handlers = Object.create(handlersProto);
    const proxy = (handlers.proxy = new Proxy(this, handlers));
    wm.set(proxy, []);
    return proxy;
  }
}

WebReflection commented May 22, 2018

I think this should be labelled as wont fix then, so that others passing by might as well know what's best as workaround.

const handlersProto = {
  deleteProperty(target, key) {
    console.log({ proxy: this.proxy });
    return Reflect.deleteProperty(target, key);
  }
};

class Foo {
  constructor() {
    const handlers = Object.create(handlersProto);
    const proxy = (handlers.proxy = new Proxy(this, handlers));
    wm.set(proxy, []);
    return proxy;
  }
}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment