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

should hooks be called with this set to the object they are pulled from? #15

Closed
bmeck opened this issue Jul 24, 2019 · 10 comments
Closed

Comments

@bmeck
Copy link
Member

bmeck commented Jul 24, 2019

new Set([], {
  allowed: [1,2,3],
  coerceValue(v) {
    if (!this.allowed.contains(v)) throw new Error('not allowed');
  }
})
@ljharb
Copy link
Member

ljharb commented Jul 24, 2019

since you can do:

const self = new Set([], {
  allowed: [1,2,3],
  coerceValue(v) {
    if (!self.allowed.contains(v)) throw new Error('not allowed');
  }
})

already, it doesn't seem to add any capabilities, just avoids some developer pain? (thus, yes)

@rbuckton
Copy link

rbuckton commented Jul 25, 2019

The closest prior-art in the ECMAScript proposal to this are Proxy traps. For proxy traps, this is the handler and the proxy target is provided as an argument. We also pass the collection as an argument for all of the built-in Array, Map, and Set methods that are user-supplied callbacks like map, filter, reduce, forEach, etc.

Having this be the Set/Map and not the options object is inconsistent with the rest of the language.

since you can do [...] already, it doesn't seem to add any capabilities, just avoids some developer pain? (thus, yes)

Your example is incorrect. self doesn't have an allowed property. I think you meant:

const self = {
  allowed: [1,2,3],
  coerceValue(v) {
    if (!self.allowed.contains(v)) throw new Error('not allowed');
  }
};
new Set([], self);

@ljharb
Copy link
Member

ljharb commented Jul 25, 2019

ohhhh you want it to be the options object. that seems super weird to me.

@rbuckton
Copy link

That's what happens for Proxy Handlers:

const handler = {
  get(target, key) {
    console.assert(this === handler);
  }
};
new Proxy({}, handler).x;

@bmeck
Copy link
Member Author

bmeck commented Jul 25, 2019

Doing the same as proxy handlers seems fine.

@bmeck
Copy link
Member Author

bmeck commented Jul 25, 2019

@rbuckton if mimicking Proxy handlers, should we lookup the functions off the options bag every time like proxies do?

see:

let i = 0;
let o = new Proxy({}, {
  get ["get"]() {
    let n = i++;
    return () => n;
  }
});
console.log(o.x, o.x);
// 0 1

@ljharb
Copy link
Member

ljharb commented Jul 25, 2019

That seems like it'd create a performance hit :-/

@bmeck
Copy link
Member Author

bmeck commented Jul 25, 2019

@ljharb i believe recent proxy optimization passes in some engines do optimize it out once you hit the full compiler. Perhaps an implementer with knowledge of proxy optimizations being done like @MayaLekova might be able to comment if the same optimization would be ok here?

@MayaLekova
Copy link

For proxies in V8 we currently do the trap lookup on each call, but we have the idea to eventually use more detailed feedback and speculate that the handler stays the same (and deoptimize if that's not the case). Looks to me that the same optimization can be applied in this collections use-case as well.

Personally I think that being consistent with other similar use-cases (like Proxy handlers) would be the better option.

@bmeck
Copy link
Member Author

bmeck commented Jul 26, 2019

this has been updated to align with how Proxy handlers are called and collection moved from this to a parameter, i could not find a place with optional options bag parameter on a glance in the spec that did a Type() check though and so I only tested for the if the options bag was Type() of Object.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants