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

Alternative proposal: property read/write trapping decorators #299

Closed
mweststrate opened this issue Nov 22, 2019 · 72 comments
Closed

Alternative proposal: property read/write trapping decorators #299

mweststrate opened this issue Nov 22, 2019 · 72 comments

Comments

@mweststrate
Copy link

mweststrate commented Nov 22, 2019

Trapping decorators

Motivation

This proposal is a proposal to introduce decorators that have fewer concepts, and are hopefully easier to implement by JVM authors.

The fundamental difference in comparison is that this proposal does not attempt to process and modify property descriptors on a target. Rather, it recognises that modifying the property descriptors has always been a means to an end for 95% of the decorators: Namely a way to trap reads and writes. To transform values.

The motivation behind this proposal is that we want to trap reads and writes to properties. Very similar to the functionality that Proxies provide so successfully. The main difference is that decorators trap all interactions with the decorated instance, and that an entire 'type' is trapped, rather than specific instances. Things that cannot be done by Proxies.

The rest of the proposal is a proposal to achieve the "property trapping". Probably any other proposal would work for me personally as well. If engine or library authors want to adjust this proposal for whatever benefit: I think the wiggle room is there, as long as it can achieve the goal of trapping property reads and writes per instance, but being able to declare such traps for an entire type.

A big benefit of traps is that they often avoid the need of introducing additional properties. E.g. if there is a getter / setter to normalize input or make some assertions, currently it is needed to introduce an additional property to store the getted / setted value. With traps. this can collapsed into one property, avoiding the need of patterns like _name = "x"; get name() { return this._name }} etc.

The proposal

Property traps

A property trap is an object that provides two functions, named get and set. The goal of these traps is to be able to trap any read and write to a property. Traps can be used to achieve two effects

  1. Transform values on read / writes
  2. Introduce side effects on reads / writes

The signature of a PropertyTrap object is:

{
    get(target, instance, property, storedValue) => value
    set(target, instance, property, newValue) => valueToStore
}

Field Decorators

A decorator syntactically is a prefix of a class or object member.
Syntactically: @<DECORATOR1> <...DECORATORN> <MEMBERDECLARATION>

Everyone of those expressions is evaluated and should resolve a function that returns a PropertyTrap. The functions get's passed in the target that will receive the property definition, and the property name. In other words, the signature of a field decorator is:

(target: object, propName: string | Symbol) => PropertyTrap | undefined

If a field decorator does not return a trap, the decorator does not install additonal traps. However, the decorator could still be valuable as it can have side effects, such as storing meta data. (example: @serializable, @deprecated etc)

A first example

function logged() {
  return  {
     get(target, instance, property, value) {
        console.log(`GET`, target, instance, property, value)
        return value
    },
    set(target, instance, property, value) {
        console.log(`SET`, target, instance, property, value)
        return value
    }
  }
}

class C {
  @logged x = 3
}
// (1) SET C.propotype undefined 'x' 3

const c = new C()
//

c.x
// (2) GET C.prototype c 'x' 3
c.x = 4
// (3) SET c c 'x' 4
c.c
// (4) GET c c 'x' 4

Semantic notes:

When construction the initial property descriptor, the traps are already applied, so that means that the @logged traps are applied during the construction of member x on class C above, and it is the transformed value that ends up in the initial property descriptor. Which is the reason we can observe side effect (1) above.

In the traps target represents the (to be) owner of the property descriptor that is being created. instance represents the this context that is used during a property access. When constructing C.x above, there is a target (C.prototype) but not a this context, hence no instance argument.

On the first read, the read is handled (or intercepted upon) the C.prototype, and the context is c, which we can observe in the output of (2).

When writing to c.x, this will result in a new property descriptor on the instance c. This is again represented in the arguments passed to the traps. Note that again the traps are applied during the computation of the new descriptor for c (the traps are inherited, similar to other properties of the original property descriptor, such as enumerable and writeable).

Note that the original decorator expression @logger is not re-evaluated again! Rather, the traps in which this originally resulted are reused and inherited by the new property descriptor (see below).

If we read from c.x again, this read is now handled by the prop descriptor on the instance, which is reflected in the arguments passed to the traps: the property owner equals the this now.

Storing traps

Traps are stored as the traps property on a PropertyDescriptor. When copying a property descriptor from one object to another, those traps are preserved. So in the above example the following would hold:

Object.getOwnPropertyDescriptor(C.prototype, "x")
// Results in
{
    writable: true,
    configurable: true,
    enumerable: true,
    value: 3,
    traps: [logger]
}

(Note, could also be stored somewhere else, as long as semantics are similar to here, and traps can be requested through reflection)

Reflection

As shown above traps are detectable trough reflection (this is different from Proxies).

Also, traps can be by-passed at any time, for example:

Object.getOwnPropertyDescriptor(c, "x").value // prints 3, without the side effects of logging

This is intentional: it makes sure that developer tools such as the debugger don't accidentally trigger side effects, makes it easy to inspect the underlying data structures, and provides an escape hatch from the traps when needed (either by libraries or users). it is very well conceivable that traps themselves use this trick to bypass themselves (e.g. a set trap might use this to get the original value, but not trigger the side effect of it's corresponding get trap)

Getters and setters

Traps are just pass-through things, so they don't necessarily operate on just value based property descriptors, but work for get / set based descriptors as well. Except that the last category will not hit the traps when the property is originally declared (since there is no value to be initialized)

Trap initializer

Decorators can be parameterized by creating functions that return decorators. For example:

function logged(enabled) {
    return () => {
        get(target, instance, prop, value) {
            return value
        },
        set(target, instance, prop, value) {
            if (enabled)
                console.log(prop, value)
            return value
        }
    }
}

const obj = {
    @logged(true) x: 4    
}

Note again that in the property descriptor for obj.x the resolved value of the decorator expression is stored, in this case a logger trap object where enabled=true is trapped in its closure.

Chaining

Decorator can be chained, which means that all the traps are applied from inside out (or, right to left):

const obj = {
    @private @logged x: 5
}

In the traps property of the descriptor this results in an array with the order of the traps as they will be applied:

Object.getOwnPropertyDescriptor(obj, "x").traps
// [trapFromLogged, trapFromPrivate]

Classes and function decorators

Class and function decorators can be large kept as they are currently implemented in babel and typescript

For example @a class X is desugared to const X = a(class X) just as currently implemented already by babel-plugin-legacy-decorators and typescript's experimentalDecorators flag.

Two open questions raised by this, on which I don't have a strong opinion:

  1. is a class decorator a side effect, or can it potentially return something different? I prefer the second; it is a little more flexible, and it makes clear that you ought to to be doing export @something class (see next question)
  2. allow decorate-before-export (What would it mean to "decorate an export"? #135)?
  3. what is the impact on hoisting? Will decorating a class / function implicitly kill the hoisting? Should class / function decorators be only side effect full to prevent that?

(if somebody could point me to a more accurate write down of the currently implemented semantics, that would be great)

Engine impact (and summary of the proposal)

This proposal tries to keep the impact on JS engines very limited:

  1. Something something parsing
  2. Whenever a property descriptor is being created for an object literal or class member, the following happens:
    1. the decorator expressions are evaluated. This should result in a function
    2. the function is called with the object that will receive the property, and the property name. The return of that function should be a trap or nothing
    3. (if applicable) the set handlers of the traps this results in are applied to the value the property descriptor would be initialized with
    4. (if applicable) the value this results in is stored in the value slot of the descriptor
    5. the set of traps is stored in the traps slot of the descriptor (only if there were any)
  3. Whenever we write to an object: if there are traps stored in the descriptor that receives the write, the set handlers of those traps are applied first, and the resulting value is stored in .value of the descriptor / passed to the set handler of the (potentially new) descriptor
  4. When reading from an object: if there are traps stored in the descriptor that receives the read, the value that is stored in .value of the descriptor (or: the value that is returned from the get handler of the descriptor) is passed trough all get traps of the traps stored in the descriptor
  5. Profit

Reference examples

Not all examples of the current readme are here:

  • @defineElement decorating entire classes is not in this proposal (see below).
  • @metaData didn't work it out, but should be doable
  • @frozen, @callable applies to a class, so skipped
  • @set cannot be done in this proposal. So field initializers in subclasses would need to receive the same decorators as the superclass if needed.

@defineElement

function defineElement(name, options) {
  return klass => {
    customElements.define(name, klass, options)
    return klass
  }
}

@defineElement('my-class')
class MyClass extends HTMLElement { }

@metadata

const allClasses = new Map() // string -> constructor
const allMembers = new Map() // klass -> propName[]

function metaData(target, propName?) {
   if (arguments.length === 1) { // used on class
      allClasses.set(target.constructor.name, target)
      return target
   } else {
      allMembers.set(target, [...(allMember.get(target) ?? []), propName])
   }
}

@metaData class X {
   @metaData member1
   @metaData function() {
   }
}

@logger

const logged () => ({
    set(t, i, p, f) {
        const name = f.name;
        function wrapped(...args) {
            console.log(`starting ${name} with arguments ${args.join(", ")}`);
            f.call(this, ...args);
            console.log(`ending ${name}`);
        }
        return wrapped
    }
})

class C {
    @logged method(arg) {
        this.#x = arg;
    }
}

@Frozen

function frozen(klass) {
    Object.freeze(klass);
    for (const key of Reflect.ownKeys(klass)) {
      Object.freeze(klass[key]);
    }
    for (const key of Reflect.ownKeys(klass.prototype)) {
      Object.freeze(klass.prototype[key]);
    }
    return klass
}

@frozen class ICannotBeChangedAnymore {

}

@bound

function bound() { 
  return {
    get(target, instance, property, fn) {
        // happens when calling Foo.method for example in example below
        if (!instance)
            return fn
        // remember that traps get inherited? 
        // if we bound before, this trap doesn't have to do anything anymore 
        // (we could skip storing the bound methods below, but better cache those bound methods)
        if (target === instance)
            return fn
        // target is not the instance, we still need to bind
        const bound = fn.bind(instance)
        instance[property] = bound
        return bound 
    }
  }
}

class Foo {
  x = 1;

  @bound method() { console.log(this.x); }

  queueMethod() { setTimeout(this.method, 1000); }
}

new Foo().queueMethod();  // will log 1, rather than undefined

@Tracked

function tracked() {
  return {
    set(t, i, p, v) {
        // Note that we can't render synchronously, 
        // as the new value would otherwise not be visible yet!
        setImmediate(() => this.render())
        
        // In practice, what would happen in MobX etc is that they 
        // would mark this object as 'dirty', and run render
        // at the end of the current event handler, rather than awaiting a next tick

        // alternatively, it would be possible to write this value
        // to _another_ property / backing Map, like done in the Readme, so that the new value is externally visible before this chain of traps ends, as done below
        return v
    }
  }
}

class Element {
  @tracked counter = 0;

  increment() { this.counter++; }

  render() { console.log(counter); }
}

const e = new Element();
e.increment();
e.increment();  
// logs 2
// logs 2

@syncTracked

function syncTracked() {
  return {
    set(t, instance, prop, value) {
        instance["_" + prop] = value // or use a Map
        this.render()
        return undefined // booyah what is stored in *this* prop
    }
    get(t, instance, prop, value) {
        return instance["_" + prop]
    }
  }
}

class Element {
  @syncTracked counter = 0;

  increment() { this.counter++; }

  render() { console.log(counter); }
}

const e = new Element();
e.increment();
e.increment();  
// logs 2
// logs 2

Non goal: modify signature

In previous decorator proposal, it is possible to significantly modify the property descriptor, or even the entire shape of the thing under construction. That is not the case with this proposal; decorators can not influence where the descriptor ends up, it's enumerability, configurability, it's type etc.

The only thing that the decorator can influence is

  1. The initial value of the property descriptor (except for getters / setters)
  2. As side effect it could introduce other members on the target, however, this is considered bad practice and is typically best postponed until we trap a read / write with a known instance.

Non goal: run code upon instance initialization

This proposal doesn't offer a way to run code guaranteed when construction a new instance, although code might run when a trap is hit during a read or write in the constructor

TypeScript

Traps don't modify the type signature at this moment. Technically, they can be used to divert the type of x.a from Object.getOwnProperty(x, "a").value, but that doesn't seem to be an interesting case to analyse statically.

More interestingly, traps can be used to normalize arguments, which means that the type being read could be narrow than the type to which is written. For example:

const asInt = () => ({
    set(t, i, p, value: any): number {
        if (typeof value === "number")
            return value
        return parseInt(value)
    }
})

const person = {
    @asInt age = "20"
}
typeof person.age /// number

In the above example, the "write type" of age property would be any, while the read type would be number. However, TS can at the moment not distinguish between the readable and writeable property, and the same limitation already applies to normal getters and setters.

Edge cases

It is possible to omit set or get from a trap. The absence of such trap represents a verbatim write-trough (just like Proxies). See also the asInt example above

Instead of storing the decorators on the property descriptor, it probably suffices to be at least able to detect them trough reflection, e.g.: Reflect.getOwnDecorators(target, property): PropertyTrap[]

Optimization notes

Like proxy handers, property traps are shared as much as possible, so they are not bound to a specific instance, type or property, but rather receive those as arguments.

An assumption in this proposal is that when writing to a property that is defined in the prototype, the original descriptor is copied onto the instance, and we could just as well copy the traps with it. However, if this assumption is wrong, this proposal doesn't strictly require this copy-traps mechanism; as the trap on the prototype could take care itself of the copying the traps in cases where this is need (for example, in @tracked it is, but in @bound it isn't).

The reason that both the decorator and the traps themselve receive the target is that this makes it much easier to reuse traps among different properties and classes. Probably this will be much better optimizable, just like proxy traps are a lot more efficient as soon as the handlers are reused. So for example, it would be adviseable to hoist traps from the decorator definitions whenever possible:

const loggerTraps = {
     get(target, instance, property, value) {
        console.log(`GET`, target, instance, property, value)
        return value
    },
    set(target, instance, property, value) {
        console.log(`SET`, target, instance, property, value)
        return value
    }
  }

function logger() { 
  return loggerTraps 
}

class X {
   @logger methodA() {

    }
    // both methods now use the same logging trap
    @logger methodB() {

    }
}

Note that this optimization notice applies to most examples above


update 27-nov-2019:

  • clarified / linked to the old class decoration proposal
  • changed the signature to of decorators to be a function, to make it easier to do meta-data-only decorators
  • added examples for @frozen, @defineElement, @metaData
  • added note about trap hoisting optimization
@littledan
Copy link
Member

Thanks for this detailed writeup. It'll take me some time to review this properly and catch up with the rest of the activity in this repo, but @pzuraq @fkleuver @wycats @syg may have thoughts.

@justinfagnani
Copy link

I think this is where we need a central list of use cases to judge variants against. I don't see this proposal solving several of them.

@pabloalmunia
Copy link
Contributor

Thanks @mweststrate for this detailed proposal.

You start with an important assumption:

The fundamental difference in comparison is that this proposal does not attempt to process and modify property descriptors on a target. Rather, it recognises that modifying the property descriptors has always been a means to an end for 95% of the decorators: Namely a way to trap reads and writes. To transform values.

In my humble opinion class decorators are important and solve many problems and user cases that are not contemplated in your proposal, which is exclusively focused on properties.

@littledan

This comment has been minimized.

@littledan
Copy link
Member

littledan commented Nov 25, 2019

@justinfagnani I tried to solicit a list of use cases in #231 . If someone wants to help put that all together to find themes, it'd be really helpful. What I tried to do was list some canonical use cases in the explainer, derived from what I'd generally heard about what was useful.

@mgechev
Copy link

mgechev commented Nov 25, 2019

In Angular we primarily use decorators for attaching metadata to classes, properties, and constructor parameters. The semantics described here should be good enough for properties, although we really don't need to "trap" them.

Regarding classes, we don't have complex runtime requirements, so:

Out of scope: Decorating classes and functions

Decorating classes and functions is in principle out of scope of this proposal. However, using decorators as purely syntactic sugar would suffice imho.

E.g.: @a class X -> const X = a(class X) and similar for functions worked fine for us in the legacy proposal.

should be good enough on our side even for JIT.

I'm curious, for example, how useful this proposal would be for someone like Angular @mgechev , if they ever wanted to migrate off of a custom transform.

I don't think we'll ever move from custom build & runtime time transforms. We compile the templates declared in the component metadata to a format similar to incremental DOM, so we can speed up rendering and change detection.

@fkleuver
Copy link

fkleuver commented Nov 25, 2019

In Angular we primarily use decorators for attaching metadata to classes, properties, and constructor parameters.

In Aurelia 2 we also use them for this purpose, but I see that as the opposite of "they cover our use cases".
It's a compatibility / safeguard mechanism. More along the lines of "they are far too limited, so we'll just do it our own way".

Do we really want to force every feature-rich framework down that path? Using decorators purely for static metadata to do runtime rendering (or build time codegen) means you won't get any native performance benefits. You're still relying on the browser's JIT for all the behavior produced by the metadata. What's the point then?

So let's talk about a use case that stands out to me the most: @tracked. We need some configurability here, for example the ability to specify a callback method on an individual property basis, because forcing everything through a single render() method is going to be a performance killer for sure.

Here's what we do in Aurelia with @bindable:

export class NameTag {
  // short-hand for: @bindable({ attribute: 'first-name', callback: 'firstNameChanged', mode: BindingMode.toView })
  @bindable firstName;
  @bindable lastName;

  firstNameChanged(newValue, oldValue) {
    console.log(`firstName changed from ${oldValue} to ${newValue}`);
  }
  lastNameChanged(newValue, oldValue) {
    console.log(`lastNamechanged from ${oldValue} to ${newValue}`);
  }
}

What this translates to at runtime, looks something like this:

export class NameTag {
  set firstName(value) { this.$observers.firstName.setValue(value); }
  get firstName() { return this.$observers.firstName.getValue(); }

  set lastName(value) { this.$observers.lastName.setValue(value); }
  get lastName() { return this.$observers.lastName.getValue(); }

  firstNameChanged(newValue, oldValue) {
    console.log(`firstName changed from ${oldValue} to ${newValue}`);
  }
  lastNameChanged(newValue, oldValue) {
    console.log(`lastNamechanged from ${oldValue} to ${newValue}`);
  }
}

In summary, the runtime renderer wires things up in such a way that incoming changes are sent directly (and only) to listeners that need those changes. In the case of a custom element, that would be for example a text node (which is wrapped by a light-weight accessor object to avoid unnecessary DOM reads).

This is what incremental dom mentioned by @mgechev more or less boils down to and from a performance point of view this is vastly superior to any VDom approach.

There are more useful configuration properties for tweaking performance and control flow but this is the main thing I wanted to touch base on first. How do other people feel about a per-property callback granularity? Or in general, configurability via object literals?

@mweststrate
Copy link
Author

mweststrate commented Nov 27, 2019

Thanks for the feedback guys! I updated the original proposal above to clarify more stuff.

  1. I confusingly mentioned class decorators to be out of scope, with that I meant out of scope of this issue, not of the entire proposal. So clarified that, but I think in essence we can largely follow the semantics babel legacy / typescript experimental decorators already supports.
  2. There are quite a bunch of cases where a decorator are just used to collect meta data. Although that was not impossible in the original proposal, it wasn't very clean either. To handle that case better I updated the proposal from "a decorator expression should result in a trap" to "a decorator expression should result in a function, that receives the target and propname, and might result in a trap"
  3. I added more examples from the original readme. I think I now covered all except for @set and @callable. I think the @set one is the only one we can't implement in this proposal

@pabloalmunia @justinfagnani let me know if this satisfies all (important) use cases for you. Sorry for the initial confusion, I didn't intend to limit decorators to properties, but just this issue ;-). As my impression the biggest problem with the previous proposal is the possibility to establish properties in to much unpredicatable ways (from VM perspective)

@littledan @mgechev with the above changes, attaching meta data only should no be a first-class supported pattern

@fkleuver I agree that tracked is the most interesting case. With this proposal the runtime trap would be a s simple as the following, without any further compile step:

const bindableTraps = {
   get(_target, instance, prop, _value) {
    return instance.$observers[prop].getValue()
   },
   set(_, instance, prop, value) {
     instance.$observers[prop].setValue(value)
     return undefined
   }
}

const bindable = () => bindableTraps

@fkleuver
Copy link

fkleuver commented Nov 27, 2019

@mweststrate Thanks for the example.

So if I understand correctly, interacting with those properties would give you the power and flexibility as if you were talking to a proxy, but without actually talking to a proxy.

Is it also possible to let users pass in arbitrary config into the decorators with your idea? I'm sorry if that's already detailed in your proposal or elsewhere, but I couldn't find it:

e.g.

bindable.js

// Module-scoped lookup
const lookups = new WeakMap();

function getObserver(instance, prop, config) {
  if (!lookups.has(instance)) {
    lookups.set(instance, {});
  }
  const lookup = lookups.get(instance);
  return lookup[prop] || (lookup[prop] = new Observer(instance, prop, config));
}

// The decorator
export function bindable(config) {
  return function () {
    return {
      get(_target, instance, prop, _value) {
        return getObserver(instance, prop, config).getValue();
      },
      set(_, instance, prop, value) {
        getObserver(instance, prop, config).setValue(value);
      }
    };
  };
}

date-picker.js

export class DatePicker {
  @bindable({ mode: BindingMode.twoWay, callback: 'dateChanged' }) day;
  @bindable({ mode: BindingMode.twoWay, callback: 'dateChanged' }) month;
  @bindable({ mode: BindingMode.twoWay, callback: 'dateChanged' }) year;
  

  dateChanged() {
    // ...
  }
}

If the traps idea is this flexible, then from a property decorator standpoint I'm pretty confident this would address all our needs.

EDIT I missed the @logged(true) example you added, so it seems the above would be possible. Correct me if I'm wrong though.

@mweststrate
Copy link
Author

mweststrate commented Nov 27, 2019

You're correct :).

@fkleuver
Copy link

Then this has my vote :)

@littledan From a framework author's point of view (one that does a lot of fancy stuff with decorators), this proposal looks perfect as far as property decorators is concerned.

@benjamingr
Copy link

This looks like a really nice proposal and I would love to play with it if there is a babel/typescript plugin.

@blikblum
Copy link

I tried to port some decorators i use currently to this proposal and found is not clear what parameters are expected to be received in the decorator. In metadata example target, propName are received as parameters but other examples are not explicit.

Below are some of my cases that (to be used in custom elements):

state decorator tracks the changes to it, rendering the element. But also needs to set some metadata to be used in connect/disconnectedCallback lifecycle methods.

// usage:
class MyView extends MyBaseElement {
  @state({copy: true}) model
}

This is how i would implement with this proposal where registerStateProperty sets some meta data

function state(options) {
  return (target, name) => {
    registerStateProperty(target.constructor, name, options)
    return {
      set(t, i, p, v) {
        // if (!initializingField) -> possible to do that check?
          instance.queueUpdate()        
        return v
      }
    }
  }
}

Is this right?
One drawback over static proposal is that i don't know if the value is being set because of a class property initialization (to not call instance.queueUpdate())

Static implementation:

decorator @state(options) {
  @initialize((instance, name, value) => instance[`__internal_${name}`] = value)
  @register((target, name) => {
    registerStateProperty(target.constructor, name, `__internal_${name}`, options);
  })
}

@blikblum
Copy link

Other use case i have with decorators is delegating some events, done at class constructor.

In static proposal i can do:

decorator @event(eventName, selector) {
  @initialize((instance, name, value) => delegate(selector ? this.renderRoot || this : this, eventName, selector, value, this);)
}

Seems is explicitly not possible with this:

This proposal doesn't offer a way to run code guaranteed when construction a new instance

I could implement by setting some metadata and in constructor of a BaseElement loop over the events to be delegated (just like i do in the decorator version implemented by Babel)

@mweststrate
Copy link
Author

mweststrate commented Nov 28, 2019

During field installation instance will be undefined.


I could implement by setting some metadata and in constructor of a BaseElement loop over the events to be delegated (just like i do in the decorator version implemented by Babel)

Correct, this would require the same workaround as in Babel. Unless you use a class decorator, because than you could do something like to wire up logic after the constructor. This approach might or might not be too obtrusive

function classDecorator(klass) { 
  return class extends klass { 
    constructor() { 
       super.apply(this, arguments); 
       fireTheEvents() 
    } 
  }
}

@classDecorator class someClass {

}

@Lodin
Copy link
Contributor

Lodin commented Nov 28, 2019

Hello. I'm the author of Corpuscule, the web components framework built with decorators. When I created this project, I used the obsolete stage 2 decorators proposal; when it was deprecated, I rewrote it with the emulation of static decorators using the legacy proposal + custom babel plugin.

Unfortunately, I cannot say that the trap proposal would work for my project. It is too small; actually, I cannot distinguish its functionality from the legacy proposal for sure. As I can see, all that can be done with the trap proposal can also be done with the legacy proposal (correct me if I'm wrong).

For me, the essential part of the obsolete and static decorator proposals is the ability to inject an action to the class constructor. You cannot do the same without Babel plugin because we don't have any constructor reflection in JavaScript. So missing this feature re-creates the same issues we had with the legacy proposal.

For example, let's imagine the following example (this is a simplified example of Corpuscule Form, which uses the final-form internally).

@form()
class Form extends HTMLElement {
  @gear('formApi') form;
}

The class decorator defines that the Form creates the final-form instance internally and puts it to the form property. The form property could actually have any name, including symbolic or private name; the role the property plays in the decorator system is sent as a decorator parameter.

With the static proposal I can do the following:

const sharedProps = new WeakMap();

export decorator @gear(role) {
  @wrap((klass, name) => {
    sharedProps.set(klass, {[role]: name});
  })
}

export decorator @form() {
  let $formApi;

  // necessary to get proper timing because the order of execution is following:
  // class @wrap -> prop @wrap -> class @register -> prop @register
  @register(klass => {
     ({formApi: $formApi} = sharedProps.get(klass));
  })

  @initialize(self => {
   // $formApi is known only during the instance initialization!
    self[$formApi] = createForm();
  })
}

This pattern is widely used in Corpuscule; furthermore, it is quite useful and allows connecting all the decorators in a single flexible system. And I'm not sure how to implement it using the trap proposal.

I would also like to avoid any unnecessary class extending during the work of the class decorator. Repeated in multiple decorators, it would pollute the prototype system with unnecessary instances of anonymous classes; that is not convenient and, probably, would lead to a performance penalty.

The initializers can also be used in property decorators. Here is an example of my implementation of ob

const token = createComputingToken();

class Foo {
  @observer(token) bar;
  @observe(token) baz;

  @computer(token) get calculated() {
    return this.bar + this.baz
  }
}

On the first call to the calculated the result of computation is memoized. Whenever the bar or baz is changed, the calculated result resets, and the next call to the calculated will run the computation again.

Here is the possible implementation via the static proposal:

const registry = new WeakMap()

export function createComputerToken() {
  const token = {};
  registry.set(token, new WeakMap());
  return token;
}

export decorator @observer(token) {
  const container = Symbol();
  let corrects;

  @register(klass => {
    corrects = registry.get(token).get(klass);
  });

  @initialize((self, _, value) => {
    self[container] = value;
  });

  @wrap((klass, name) => {
    Object.defineProperty(klass.prototype, name, {
      configurable: true,
      get() {
        return this[container]
      },
      set(value) {
        this[container] = value;

        for (const correct of corrects) {
          this[correct] = false;
        }
      }
    })
  })
}

export decorator @computer(token) {
  const correct = Symbol();
  const memoized = Symbol();

  @initialize(self => {
    self[correct] = false;
    self[memoized] = null;
  });

  @wrap((klass, name) => {
    tokenRegistry.get(token).set(klass, correct);

    const {get} = Object.getOwnPropertyDescriptor(klass.prototype, name);

    Object.defineProperty(klass, name, {
      configurable: true,
      get() {
        if (!this[correct]) {
          this[memoized] = get.call(this);
          this[correct] = true;
        }

        return this[memoized];
      }
    });
  });
}

As you can see, it relies on initializer heavily. I'm not sure if it can be implemented via the trap proposal.

Also I would like to mention that the code samples I'm using here is not the current version of the static proposal. I added some things I have talked about in #276.

@robbiespeed
Copy link
Contributor

robbiespeed commented Dec 5, 2019

I really like the idea of property traps, but I have 2 concerns with this proposal.

First is composition, all versions of decorators so far have offered really clean composition. With this proposal you'd have to call another decorator and then use it's traps inside the new one:

function foo (target, property) {
  const barTrap = bar(target, property);
  return {
    get (target, instance, property, value) {
      const _value = barTrap.get(target, instance, property, value);
      console.log('GET:', target, instance, property, _value);
      return _value;
    },
    set (target, instance, property, value) {
      const _value = barTrap.set(target, instance, property, value);
      console.log('SET:', target, instance, property, _value);
      return _value;
    }
  };
}

My second concern is making sure traps don't suffer similar performance pitfalls as proxy. With a proxy if you only define a handler for set, but are leaving get to be handled natively, there is still a penalty when getting a property. It would be great if we could avoid this for property traps, so that if all I have are set traps, it has no impact on the performance of get operations. If this could be achieved then performance speaking it could be superior to all the other decorator versions so far (which modify the property descriptor into a getter and setter). One potential option would be to store get and set traps separately, so if one were to look at them using some kind of reflection it might look like this:

Object.getOwnPropertyTraps(A.prototype, 'propName');
// { get: [], set: [setLogger] }

Separating property traps from decorators could lead to a very versatile set of tools, what I mean by this is that if we treat decorators in a way that allow us to perform any operation on a target, propName pair then not only could we adjust a property descriptor but we could also add traps, or store meta information and not adjust the property in any way. This would look something like:

function setLogger (target, propName) {
  Object.addPropertySetTrap(target, propName, (instance, _propName, value) => {
    console.log('Set:', instance, _propName, value);
    return value;
  });
}

const tagged = new WeakMap();
function tag (target, propName) {
  let taggedProps = WeakMap.get(target);
  if (!taggedProps) {
    WeakMap.set(target, taggedProps = new Set());
  }
  taggedProps.add(propName);
}

function forbid (target, propName) {
  Object.defineProperty(target, propName, {
    get () { throw new Error(`property "${propName}" is forbidden`); },
    set () { throw new Error(`property "${propName}" is forbidden`); }
  });
}

When decorating a property no return value would be expected.

This would drastically simplify the API of decorators, and not favour any specific means of property adjustment. That would open up the possibility of future ideas like property traps.

@xlaywan
Copy link

xlaywan commented Dec 5, 2019

I like Object.addPropertySetTrap. Good way to dynamically add an observer to mutable data. For now, we need to use Object.defineProperty with a redundant getter and possibly a performance penalty.

@mweststrate
Copy link
Author

@robbiespeed

First is composition, all versions of decorators so far have offered really clean composition. With this proposal you'd have to call another decorator and then use it's traps inside the new one:

No you don't, they are chained automatically, so the value received by a trap is already transformed by any earlier traps; decorators don't need to know about each other. See 'chaining' in the original proposal for more details

My second concern is making sure traps don't suffer similar performance pitfalls as proxy. With a proxy if you only define a handler for set, but are leaving get to be handled natively

This was addressed in the proposal as well; the hooks can be left out in which cause the default to just passing the value along

Separating property traps from decorators could lead to a very versatile set of tools

That is the original problem that this proposal tries to address.. The first version of the proposal-decorators heavily relied on calling Object.defineProperty from library / decorator code, which is impossible for engines to optimize. The introduction of traps tries to exactly avoid that, eehh... trap by making the object shape predictable and (mostly) avoiding changing object shapes

@mweststrate
Copy link
Author

@Lodin indeed, with this proposal the way to create initializer logic remains either extending a class (or providing a base class). I'm personally not convinced that having a longer prototype chain is worse than having an initialization mechanism which is hard to optimize by compilers (this was one of the objections against the original proposal)

@robbiespeed
Copy link
Contributor

robbiespeed commented Dec 13, 2019

@mweststrate

No you don't, they are chained automatically, so the value received by a trap is already transformed by any earlier traps; decorators don't need to know about each other. See 'chaining' in the original proposal for more details

Chaining decorators is not equivalent to composing them. A better example would be that you have both a foo and bar decorator, and you want to compose them into a single foobar decorator. In classic that would look something like:

function foobar (target, propertyKey, descriptor) {
  return foo(target, propertyKey, bar(target, propertyKey, descriptor));
}

Static:

decorator foobar {
  @foo @bar
}

Your proposal:

function foobar (...args) {
  const fooTrap = foo(...args);
  const barTrap = bar(...args);

  return {
    get (target, instance, propertyKey, value) {
      const fooValue = fooTrap.get(target, instance, property, value);
      const barValue = barTrap.get(target, instance, property, fooValue);
      return barValue;
    },
    set (target, instance, propertyKey, value) {
      const fooValue = fooTrap.set(target, instance, property, value);
      const barValue = barTrap.set(target, instance, property, fooValue);
      return barValue;
    }
  };
} 

My suggestion of simple decorators:

function foobar (target, propertyKey) {
  foo(target, propertyKey);
  bar(target, propertyKey);
}

(Inside foo and bar either traps or modifed descriptors could be used)

Re proxy concerns:

This was addressed in the proposal as well; the hooks can be left out in which cause the default to just passing the value along

The implementation matters, if set and get traps are being stored in a shared object, then that object could be modified to change an existing trap or add one that was omitted. I don't think this is an easy thing for engines to work out, that's why I used proxies as an example. Browsers have been attempting to optimize proxies since their release, but still if you have no traps your get / set calls are much much slower (https://jsperf.com/proxy-vs-obj/).

Sorry I forgot that property descriptors returned from Object.getOwnPropertyDescriptor are copies, so mutating them has no effect on the actual property. Unlike proxies where the handlers can be mutated.

@robbiespeed
Copy link
Contributor

The first version of the proposal-decorators heavily relied on calling Object.defineProperty from library / decorator code, which is impossible for engines to optimize. The introduction of traps tries to exactly avoid that, eehh... trap by making the object shape predictable and (mostly) avoiding changing object shapes

I don't think engines are going to significantly optimize the application of decorators. The exception maybe being static decorators, if the proposal chooses not to allow dynamic code execution inside the decorator definition, which is a useful feature many are hoping for. I also don't think perf matters that much the instant decorators are applied, what matters is the ongoing operating performance of the classes and instances after. The operating performance is where traps would come in most handy.

I really want property traps, that's a major reason why I think they should have their own proposal. It would also be really nice to have the ability to apply them without the use of classes / decorators.

@ljharb
Copy link
Member

ljharb commented Dec 13, 2019

@robbiespeed my understanding is that the performance hit of dynamic stage 2 decorators was on class and instance creation time, not thereafter - once created, it's data and accessor properties same as anything else. A "traps" proposal (using Proxy's definition of "trap"), however, may run the risk of affecting ongoing interaction with a created object.

@benjamingr
Copy link

@robbiespeed hey, thanks for your comments. regarding:

function foobar (...args) {
 ...
} 

I have found shorter but not obviously correct ways to write this - can the language just provide a compose method that composes varargs decorators together?

@robbiespeed
Copy link
Contributor

@ljharb

my understanding is that the performance hit of dynamic stage 2 decorators was on class and instance creation time, not thereafter - once created, it's data and accessor properties same as anything else.

It depends on what the decorator is doing, if you're wrapping a value with a new descriptor (most value decorators), you have to create a WeakMap to then reference the old value, and access it through getter and setters. Even getters and setters have a negative performance impact you can see in https://jsperf.com/proxy-vs-obj/. In Firefox property access performance is reduced to 30% just by using a getter (the gso GET test), it would be even worse if the WeakMap was involved which it would need to be when decorating.

Traps would at the least, remove the need for a WeakMap, but it would also make it possible to only affect the perf of set and leave get with close to original performance (or vice versa). Engines could maybe even optimize further since traps wouldn't result in deeply nested function calls like applying multiple classic decorators does (through property descriptor modification)

A "traps" proposal (using Proxy's definition of "trap"), however, may run the risk of affecting ongoing interaction with a created object.

It would of course still have an impact, but that impact would/could be lower than it is now. Proxy has issues that this proposal could avoid, proxies handlers are mutable, where as we could make property trap handlers immutable and only modifiable using decorators, Object.defineProperty, or some new method like I mentioned previously. This would give the ability to build a cached list of get traps and set traps separately, the engine could use these lists for either property get or set respectively.

Similar to how properties are now either flagged as value, or getter/setter, they could in future have 8 possibilities: value, getter/setter, value+traps(get,set), value+traps(set), value+traps(get), getter/setter+traps(get,set), getter/setter+traps(set), getter/setter+traps(get). Each time property access or assignment is performed you'd look at the flag and determine the fastest path for the operation. Ex: a property flagged as value+traps(set) on access would use the same path as value because we know it has no get traps only set traps, on assignment it would use the value+traps(set) path.

@robbiespeed
Copy link
Contributor

@benjamingr @mweststrate
I think if decorators are going to be tied to traps, then the best option for composing would be if decorators also allowed returning a list of traps.

function foobar (...args) {
  return [ foo(...args), bar(...args) ];
} 

@Lodin
Copy link
Contributor

Lodin commented Dec 28, 2019

@mweststrate

I'm personally not convinced that having a longer prototype chain is worse than having an initialization mechanism which is hard to optimize by compilers (this was one of the objections against the original proposal)

As I understand what @littledan said, the current proposal suggests that all the decorators are executed before any optimizations done by the engine, so there should be no performance penalty. That's the basic idea of the static decorator proposal. It means that we would get the constructor injections for free without polluting the prototype chain.

@mbrowne
Copy link
Contributor

mbrowne commented Jan 19, 2020

I'm just curious as to when TC39 will be meeting next to discuss decorators, and what is the current status?

@littledan
Copy link
Member

I'm sorry I've been so behind on this thread. I'd like to seriously consider this kind of alternative.

I've added a discussion of decorators, including this alternative, to the March 2020 TC39 meeting agenda (though it's a late addition, so I've asked for it to be deprioritized).

If we want to restrict the scope of this first decorator proposal to just field decorators, and just turning fields into getter/setter pairs, then that's an option. I understand that this thread isn't requiring that, but I also am not sure what semantics for those other constructs would be. So let's just talk about fields for now.

Some high-level benefits I see to this proposal:

  • Easier for people to understand what code does when a field is decorated: It'll always act sort of like a field, but might have extra behavior on access
  • Easier for implementations! It's a pretty fixed transform (unless the decorator returns undefined...)
  • There's a built-in notion of "underlying" storage, without encouraging people to use WeakMaps
  • Might meet practical use cases (TBD, we may have to study more)

Some questions for when it comes down to details:

  • I get the idea that you could have more introspection, but it adds a lot of complexity to the JS object model. Would it be OK to just desugar into accessors on the prototype? Instead of having an own data property, the underling storage could be a generated, hidden private field.
  • The decorators proposal has long had something or other making "initializers" distinguished from future sets. Should we maintain that?
  • Could we make this work for decorating private fields? (I have some ideas, but they depend on the answers to the other questions.)
  • Why is the target passed to each get/set call? I can't figure out when you'd use this.
  • The part where this becomes not-a-fixed-transform is where it might be used just for metadata, not installing a trap. It's really hard for me to square this with the rest of the proposal. Could we omit it, and say that they all must return a PropertyTrap, not undefined? (If so, then it'd really help for implementability/predictability.)

@mgechev
Copy link

mgechev commented Mar 24, 2020

From Angular stand point a getter/setter proposal with runtime semantics would not be useful. Form our perspective, even only standardizing syntax for class, property, method, and parameter decorators would have much higher value.

@littledan
Copy link
Member

@mgechev Thanks for explaining. Overall, on the "completeness" point, it seems to me like the participants of this thread are in agreement that restricting the scope to only field decorators wouldn't be sufficient for use cases that we all care about. Is that accurate?

@pabloalmunia
Copy link
Contributor

@littledan Yes. Restricting decorators to only fields is a huge loss of functionality. IMO, the Javascript community has been waiting for a prowerful decorator definition that allows transform classes, methos and fields.

@pzuraq
Copy link
Collaborator

pzuraq commented Mar 25, 2020

@littledan I also think that limiting scope here wouldn't really make sense. One of the most commonly agreed on use cases across frameworks and the ecosystem, @bound, is a method decorator.

My main concern is that we would advance a solution for class fields, only to have the caveats of that solution block method decorators from advancing. For instance, the hooks proposal as outlined implies that methods would also become accessors, despite many method decorators not requiring access to this. If it were determined that turning all decorated methods into accessors was too expensive, and it blocked @bound and other method decorators, I think it would be pretty bad for usability and cohesiveness in general.


On a different note, this got me thinking about whether on not we could have different semantics per type of class element. One of the downsides of this proposal is that methods will also be transformed into accessors, which likely has some performance implications. I also think that, in general, this isn't actually really necessary, and may actually result in decorator antipatterns. For example:

class Foo {
  @decorate method() {
    // ...
  }
}

let foo = new Foo();

// It would be very a very strange, counterintuitive API
// if `method` were not callable like so
foo.method();

In general I think the expectation is that decorators will somewhat respect the shape of the output. That is, methods will remain callable, and fields and accessors will not be (unless their original value was, in fact, a function). If that's true, then the only time methods really need to be transformed into accessors is if they would result in bound method, like @bound.

So, we could potentially divide it up like this:

Original element type ...when decorated, becomes a(n)
Field Accessor
Accessor Accessor
Method Method OR Bound Method

So I wonder if we could have an alternative API for methods, something like:

function logged() {
  return {
    bound: false,

    // highly bikesheddable name/API
    wrappedMethod(target, instance, prop, value, ...args) {
      console.log(`starting ${prop} with arguments ${args.join(', ')}`);
      let ret = value.call(this, ...args) + 1;
      console.log(`ending ${prop}`);
      return ret;
    },
  };
}

Where wrappedMethod would replace the method itself, and bound would determine whether or not that method was bound to the instance.

The main issue I can see with this is, if decorators ultimately desugar to standard properties + accessors, I think decorated methods would still need to become accessors in general to support the bound option (imagining someone doing Object.getOwnPropertyDescriptor(Class.prototype, 'boundMethod'), that needs to produce the accessor), so engines would still have to assume that the method would be replaced with an accessor. But, it would be a restricted accessor - it would either A. return the underlying value or B. return a bound version of the underlying value. So, it could be easier to optimize.

I also think it might be confusing if we wanted to support decorators that can operate on both methods and fields/accessors. You could imagine it looking like:

function logged() {
  return {
    // Field/Accessor implementation
    get(target, instance, prop, value) {
      console.log(`getting property ${prop}`);
      return value;
    },

    set(target, instance, prop, value) {
      console.log(`getting property ${prop}`);
      return value;
    },

    // Method implementation
    bound: false,

    wrappedMethod(target, instance, prop, value, ...args) {
      console.log(`starting ${prop} with arguments ${args.join(', ')}`);
      let ret = value.call(this, ...args) + 1;
      console.log(`ending ${prop}`);
      return ret;
    },
  };
}

I'm not sure this is too concerning though, since usage is much more common than definition. This would also give us a way to let users know if they're using a decorator on the wrong type of class element, which could result in better DX.

@justinfagnani
Copy link

I have to chime in, and possibly be a contrarian, and say that I don't think that @bound is a very good use case for decorators, because it's very easy to do already:

class A {
  boundMethod = (a) => {
    console.log(this);
  };
}

Meanwhile, meta-programming over class fields is literally impossible. It's the critical case to solve with something, and decorators seem like the only remotely concrete proposal to address that.

Regardless, implementing @bound or any other method decorator like for that matter, as an accessor seems fine, in that a bound method isn't really different from an instance property, and a getter on the prototype that returns the bound method for the instance should have perfectly fine performance compared to a field containing the bound method reference. As long as private storage is induced that the accessors can use, for both the field and method cases, this should be fine.

@mweststrate
Copy link
Author

One of the most commonly agreed on use cases across frameworks and the ecosystem, @bound, is a method decorator.

A solution is to make sure that decorators also work on methods, but that they trap on the prototype instead of on the instance (since methods are prototype members, not instance members like fields). So bound would be implemented as

class X {
  @bound method() {
  }
}

function bound() {
   return {
     get(_t, instance, property, value) {
       return instance[property] = value.bind(instance)
     },
     set(_t, _i, _p, value) {
        return value
     }
  }
} 

This trap is established on the prototype (since it is decorating a prototype member), so once we read from it, we bind the original value (the unbound method) to the instance for which we are reading, and the bound method will from then on hide the accessor on the prototype.

So I don't see the performance trap, as this creates only one accessor, on the prototype, rather than an accessor on all instances. Like with any existing implementation of bound, the most expensive part of it is most probably the creation of all closures (+ additional fields on the instance).

Also I wouldn't a priori assume getters are slow, that is definitely not my experience, as long as you make sure to recycle the handlers.

@pzuraq The rest of your post I couldn't really follow, as accessors and call-ability are unrelated things. One describes how things are read / stored, and the other respresents a value type.

I think the bound field you are proposing is a bit of an awkward one-off solution for a very specific problem. The wrapping you propose could be done in the set if the set is triggered for the initial value assignment.

@ljharb
Copy link
Member

ljharb commented Mar 25, 2020

@justinfagnani using an arrow function as a class field inhibits testing and reusability, because you can't spy on (or borrow) a prototype method prior to instantiating an instance. @bound is an absolutely critical decorator for react testing, and without that I'd be hard to convince decorators would be worth it at all.

@pzuraq
Copy link
Collaborator

pzuraq commented Mar 25, 2020

@justinfagnani I think the main reason @bound is better than a class field for method binding is that it allows the preservation of super:

class A {
  boundMethod = (a) => {
    console.log(this);
  };
}

class B extends A {
  boundMethod = (a) => {
    // this fails, since the parent method has overwritten the child
	super.boundMethod();
    console.log(this);
  };
}

Edit: Also the same reasons @ljharb mentioned, which are pretty solid as well.

Regardless, implementing @bound or any other method decorator like for that matter, as an accessor seems fine, in that a bound method isn't really different from an instance property, and a getter on the prototype that returns the bound method for the instance should have perfectly fine performance compared to a field containing the bound method reference. As long as private storage is induced that the accessors can use, for both the field and method cases, this should be fine.

I agree here in general, it's one of the reasons I liked the traps proposal. It seems like it would cover most common use cases this way.

The only downside is, like I pointed out, the majority of method decorators really don't need to be accessors. I don't know of a case currently where you would want to return, say, a class instance or object in place of a function. So, we could in theory try to do something different there.

I'm not sure of what the performance implications are for having all decorated methods become (simple) accessors, if any, but if it ends up being problematic we could explore having different semantics for decorating a class field/accessor and method. If not, a simpler mental model and spec is probably better 👍

@justinfagnani
Copy link

@justinfagnani using an arrow function as a class field inhibits testing and reusability, because you can't spy on (or borrow) a prototype method prior to instantiating an instance. @bound is an absolutely critical decorator for react testing, and without that I'd be hard to convince decorators would be worth it at all.

I wish this criteria were applied to class fields. We're already in a situation where you can't do anything to them at all.

What would the prototype representation be for @bound methods that would allow them to be replaced? I'm not sure I've understood that requirement before, or how it would work in practice. Are you saying that any replacement is automatically bound also? How does that work?

class A {
  @bound methodOne() {
    console.log(this);
  }
}

A.prototype.methodOne = function() {
  console.log(this);
};

const a = new A();
const m = a.methodOne;
m(); // what does this print?

@pzuraq
Copy link
Collaborator

pzuraq commented Mar 25, 2020

@mweststrate

This trap is established on the prototype (since it is decorating a prototype member), so once we read from it, we bind the original value (the unbound method) to the instance for which we are reading, and the bound method will from then on hide the accessor on the prototype.

I think that would cause a problem in general with updating the shape of the object at dynamically/arbitrarily after construction, no? That's, as I understand it, much worse for performance than any kind of dynamism during initialization 😬 Maybe decorators could also establish a slot on the instance to ensure that they don't mutate the shape arbitrarily, but that seems not ideal.

So I don't see the performance trap, as this creates only one accessor, on the prototype, rather than an accessor on all instances. Like with any existing implementation of bound, the most expensive part of it is most probably the creation of all closures (+ additional fields on the instance).

So, the majority of common use cases for method decorators actually don't bind, in my experience. A good example would be something like @memoized:

class Foo() {
  @memoized method() {
    console.log('ran!');
  } 
}

let foo = new Foo();

foo.memoized(123); // ran!
foo.memoized(123); // 
foo.memoized(456); // ran!

So while you may not be creating an accessor per-instance, you would be creating an accessor per decorated method when it really isn't necessary. Again, I'm not sure what the actual performance implications are there, but I expect these types of use cases for method decorators to be pretty common and think that in a large app, with 100s-1000s of decorated methods, it could have an impact.

@pzuraq The rest of your post I couldn't really follow, as accessors and call-ability are unrelated things. One describes how things are read / stored, and the other respresents a value type.

I think the bound field you are proposing is a bit of an awkward one-off solution for a very specific problem. The wrapping you propose could be done in the set if the set is triggered for the initial value assignment.

My point was mainly that if we limit the accessor functionality specifically for decorated methods, then we might be able to optimize it. Otherwise, it's arbitrary accessor code, which may or may not be optimizable. Assuming that performance isn't an issue though, I agree, the simpler solution is probably better.

@mweststrate
Copy link
Author

I think that would cause a problem in general with updating the shape of the object at runtime, no? That's, as I understand it, much worse for performance than any kind of dynamism during initialization

As @littledan noted (if I understood correctly), doing that when establishing the prototype, is fine. That is no different than doing a bunch of (which is already well optimized). If the decoration process is done only once per class, and doesn't change the predictability of the shape (which this proposal doesn't if it always introduces a property under the same name).

X.prototype.a = b
X.prototype.b = c

with 100s-1000s of decorated methods

I did this with numbers way bigger than that, and that accessor is going to be the least of your perf problems in such cases :) A property descriptor is a property descriptor, regardless whether it is getter or value oriented, the first is just two function allocations more expensive for the entire app (if I simplify things a bit). As long as the getter itself is efficient (like in the bound example) this will work efficiently. Remember people where worried about the same thing with Proxies, and they are way more complex than this proposal, yet they still perform very will (ok granted, that took a lot of effort and time of the engine implementors, but I hope that can be avoided with this proposal)

@justinfagnani
Copy link

I think that would cause a problem in general with updating the shape of the object at dynamically/arbitrarily after construction, no? That's, as I understand it, much worse for performance than any kind of dynamism during initialization 😬 Maybe decorators could also establish a slot on the instance to ensure that they don't mutate the shape arbitrarily, but that seems not ideal.

I thought this was the gist of the proposal in the first place. The accessors should be installed on the prototype, not the instance, and the induced backing storage should be installed on the instance. This ensures a consistent shape effect for any field defining decorator.

@pzuraq
Copy link
Collaborator

pzuraq commented Mar 25, 2020

@mweststrate

As @littledan noted (if I understood correctly), doing that when establishing the prototype, is fine. That is no different than doing a bunch of (which is already well optimized). If the decoration process is done only once per class, and doesn't change the predictability of the shape (which this proposal doesn't if it always introduces a property under the same name).

Ah, I misunderstood your example. If I'm understanding you and @justinfagnani correctly, then in this example, instance[property] = value.bind(instance) calls the setter, setting to the internal storage slot, and everything "just works" without changing the shape of the instance:

function bound() {
   return {
     get(_t, instance, property, value) {
       return instance[property] = value.bind(instance)
     },
     set(_t, _i, _p, value) {
        return value
     }
  }
} 

But you also say:

the bound method will from then on hide the accessor on the prototype.

Which implied to me that it would shadow this accessor and the getter would never fire again. That was what made me think it would modify the shape of the object (I wrote a similar implementation for our version of @bound before realizing this, using legacy/TS decorators).

So I think in this case, the accessor on the prototype would still be called every time the method was looked up, correct?

Remember people where worried about the same thing with Proxies, and they are way more complex than this proposal, yet they still perform very will (ok granted, that took a lot of effort and time of the engine implementors, but I hope that can be avoided with this proposal)

They certainly perform well, much better than originally expected, but I've found they still incur a cost and at scale that can have a significant impact. Maybe this is simple enough it wouldn't be an issue though, but I had the idea and thought it might be worth mentioning.

@ljharb
Copy link
Member

ljharb commented Mar 25, 2020

@justinfagnani

class C {
  @bound
  foo() {}
}

would be equivalent to:

class C {
  foo = this.foo.bind(this);
  foo() {}
}

As for why it doesn't apply to class fields for me, that's because fields are for data - functions aren't data. For React components, I don't need to borrow or mock "state" for all instances of a component - so state = {} is totally reasonable on a single component. However, I might want to mock all onClicks or makeNetworkRequests for all instances of a component, so a class field for those functions isn't reasonable to me.

@NullVoxPopuli
Copy link

I would like to point out that:

class C {
  foo = () => {}
}

does not, in any way, define a method, foo.
it is a property, foo that has a value of an anonymous function, (which, I believe is) duplicated per instance of C.

just so we're all on the same page :D

we mustn't lose sight of the prototype and why it exists.

@justinfagnani
Copy link

@NullVoxPopuli bound methods necessarily have to have a different instance per class instance. They can't be stored on the prototype.

@mweststrate
Copy link
Author

mweststrate commented Mar 26, 2020

Lets throw the concept of 'methods' out of the window here, as that is the thing that adds more confusing to the convo here than it adds value, as it is a concept that doesn't exist technically at all in JavaScript (semantically speaking, not syntactically):

class X {
   @decorator1
   field = y

   @decorator2
   method() { }
}
  • field is a property that would be defined on the instance, so @decorator establishes an accessor on the instance.
  • method is a property that is established on the prototype, so that is where the @decorator2 establishes the trap.

But method is just as much a property as field is, just of the prototype. "method" is the conventional naming for "a property on a prototype that holds a value that is a function", but has no implicit magic features like automatic sharing or something, and for that reason I think it doesn't necessarily neat special treatment in the fields proposal. I mean, we could do that for any good reason, but I think at this point it complicates the mental model and implementation, rather than simplifying it.

That is why the proposal distinguishes the target and instance in the trap parameters: the target is the owner of the property, and the instance is just a convenience parameter representing the this context of the trap call. (How those two concepts are actually accessed is more of a design detail, for example the instance param could be omitted, forcing decorator implementations to rely on this instead in the traps)

So binding is in the end just a value transformation, just as much as a decorator that converts celsius to fahrenheit. If you can express the one, you can express the other.
However, the reason that bounds has a side effect to shadow its own definition, rather than using the storage slot, is because the accessor is established on the prototype, but we don't want to update the value stored for the prototype, but store a new value for the instance instead. So you could argue that it kinda alters the shape of the instance on first evaluation, but so do all bound implementation that exists already in the wild, so I am personally not too worried about it, I think it is quite an exceptional case. (And as said, except for rare cases you are probably better off with a local arrow function anyway)

@pzuraq
Copy link
Collaborator

pzuraq commented Mar 26, 2020

Lets throw the concept of 'methods' out of the window here, as that is the thing that adds more confusing to the convo here than it adds value, as it is a concept that doesn't exist technically at all in JavaScript (semantically speaking, not syntactically)

Totally agree about your point with semantics here, but I think that the syntactic distinction could still be valuable, in that it gives us a way to change the semantics of decorators if we choose to. We can have different semantics per class element type, and that's really the only wiggle room we have for semantic differences with a non-static-decorators style approach.

And again, stepping back, I do agree that the mental model you propose is simpler and easier to grasp if you understand the basics of JS and prototypical inheritance. If everything boils down to an accessor, it is much easier to reason about. But we're here in the first place because of performance, so I think it's important to consider all options, think them through, and consider the tradeoffs.

So you could argue that it kinda alters the shape of the instance on first evaluation, but so do all bound implementation that exists already in the wild

Ours does not, we use a WeakMap to associate the function with the instance. For bound functions, I think this is an acceptable cost vs changing the shape dynamically. We've found that dynamic object shapes can cause significant performance issues in general.

@ljharb
Copy link
Member

ljharb commented Mar 26, 2020

The bound method is per-instance, but the method being bound is not and should not be.

@mbrowne
Copy link
Contributor

mbrowne commented Mar 27, 2020

There have already been some long discussions about the bound decorator in other threads in the past. Let's set aside the discussion about whether or not it's a good use case for decorators—the more important question is whether or not the ability to implement things like a bound decorator (i.e. adding a new instance property to an object via a decorator) is something we need decorators to support.

For more details on the various motivations for a bound decorator and the problems with just using arrow functions directly, see https://github.com/mbrowne/bound-decorator/blob/master/MOTIVATION.md. If anyone has questions about that or wants to discuss it more, I could post some links to past discussions and/or we could start a separate thread about that.

@mbrowne
Copy link
Contributor

mbrowne commented Mar 29, 2020

@ljharb

@bound is an absolutely critical decorator for react testing, and without that I'd be hard to convince decorators would be worth it at all.

Really? You don't think any of the other use cases that people have suggested, and which are already being used, justify the existence of decorators? This seems like a strange statement coming from you, especially since decorators were often presented as a way of filling in some gaps left by the class fields proposal (although maybe it was more @littledan doing that). Not trying to bring up old debates, just trying to understand your opinion. Also, in terms of React, writing new components using classes will probably be quite rare in a few years if it isn't already, so I'm not sure that a native @bound decorator is the most important thing in the world...although I'm certainly in favor of it if at all feasible (and of course it can be used outside of React too). Especially considering that Babel and Typescript already have solved that problem for now by implementing earlier decorators proposals.

@ljharb
Copy link
Member

ljharb commented Mar 29, 2020

@mbrowne i'm not saying those aren't also important (including "set" vs "define", etc). I'm saying that the feature is incomplete for me without "bound", and it's not worth adding an incomplete syntactic feature.

As for transpilers "solving" the problem for now; if that's the case, no language feature would be needed at all - I don't consider code transformations to be a solution.

@mbrowne
Copy link
Contributor

mbrowne commented Mar 29, 2020

Ah, I see.

I don't consider code transformations to be a solution.

The only reason I was bringing that up is because by the time native decorators ship in browsers, React classes will likely have mostly fallen out of use anyway (at least on new code).

@ljharb
Copy link
Member

ljharb commented Mar 29, 2020

I don't plan on stopping use of them; I find them simpler and more readable in many cases than hooks. I can't speak for everyone else.

@mbrowne
Copy link
Contributor

mbrowne commented Mar 29, 2020

(Off-topic) I can understand preferring classes. (I think if personal preference were the only factor to consider, I might continue using them too.) They will probably eventually be moved to a separate package though, and I predict that they'll become a legacy feature that almost no one uses at that point (due to lack of ongoing support/updates and the option to reduce bundle size by not using them). I guess this isn't completely off-topic in the sense that TC39 decisions probably shouldn't be driven very much by React classes going forward, as that use case will no longer be a major focus of the community.

@Lodin
Copy link
Contributor

Lodin commented Mar 29, 2020

The only reason I was bringing that up is because by the time native decorators ship in browsers, React classes will likely have mostly fallen out of use anyway (at least on new code).

I guess this isn't completely off-topic in the sense that TC39 decisions probably shouldn't be driven very much by React classes going forward, as that use case will no longer be a major focus of the community.

@mbrowne, but React is not a single framework in the field. Web components are also based on classes, for them the class shape won't go anywhere because it is a standard. And the @bound decorator would be quite useful for them, e.g. for setting event listeners via addEventListener.

@fkleuver
Copy link

fkleuver commented Mar 31, 2020

I always thought the idea behind this proposal was to "merge" it with the static proposal. The static proposal for the class-level use cases and this one for the field-level use cases. Did I misunderstand the intent?

As for pure metaprogramming, why not keep the idea of runtime-accessible metadata? Angular's AOT can optimize them away, and at the same time we at Aurelia can still offer the decorator+class api in "script tag" scenarios. The syntax is important at class and at field level, not just for AOT, but also for tool-free situations. Tool-free situations are not the common use case but they are an important one, so performance usually doesn't matter nearly as much as it at least being possible.

A somewhat contrived example where just metadata (no accessor) on a class field would be great:

@customElement('fancy-text')
export class FancyText {
  @oneTime label;
  @twoWay value;

  valueChanged(newValue) {}
}
<fancy-text label="foo" value.bind="value"></fancy-text>

The twoWay decorator is much like tracked, but oneTime would only add some metadata to the class that tells the runtime to only read the label attribute once and assign it to the label property. No need for an accessor.

In any case, key points for us:

  • Metaprogramming is not just for AOT because runtime-accessible metadata is still a useful escape hatch
  • Metaprogramming is useful on class-level and field-level decorators

@mbrowne
Copy link
Contributor

mbrowne commented Oct 16, 2020

Is this proposal still being considered? And I don't quite understand how this would work...is it something that could be added in a second version of decorators, or does the decision need to be made before the first release? I'm still concerned that the current static decorators proposal is not powerful and customizable enough.

@mweststrate
Copy link
Author

The current proposal in this repo is roughly based on this issue, so for the current state, just check the readme :).

To avoid confusion will close this issue now.

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