search

tc39 / proposal-decorators

Decorators for ES6 classes
https://arai-a.github.io/ecma262-compare/?pr=2417
2.74k stars 106 forks source link

confusing that extra initializers for auto accessors and regular accessors do not both run after storage initializers. #548

Open trusktr opened 4 days ago

trusktr commented 4 days ago

[!Note] Keep in mind that this is feedback coming in after using decorators, which was not previously possible until tools like TypeScript and Babel have more recently implemented the Stage 3 spec and we've had time to work with them in the field.

Fundamentally speaking, the new accessor keyword is basically syntax sugar for get/set members with private storage. However, when decorating them, the behavior is not the same, which can result in not being able to achieve the same result as we can do with accessor using set/get, therefore eliminating some of the sugary aspect of accessor.

For example, intuitively these would behave the same (but they don't, so there's a runtime error):

function foo(_, context) {
  const {name, kind} = context

  if (kind === 'accessor' || kind === 'getter' || kind === 'setter') {
    context.addInitializer(function(this: any) {
      console.log('initial value:', this[name])
    })
  }
}

class Test1 {
  @foo accessor a = 1
}

// The above `accessor` form is basically "syntax sugar" for the following:
class Test2 { 
  #foo = 1
  @foo get b() {return this.#foo}
  @foo set b(n) {this.#foo = n}
}

new Test1() // logs "1"

new Test2() // expected to log "1", got runtime error

TypeScript playground, Babel Repl

The fact that we cannot access the getter/setter in an initializer is problematic, requiring the use of a class decorator if we need the initial value (and this eliminates the getter/setter from being reliable in a user's own constructor).

Use case

When implementing an @attribute decorator that maps Custom Element attributes to JS properties, I want to set the initial value of a JS property back to the storage's initial value when the associated HTML attribute is removed.

With accessors, this is easy:

function attribute(_, context) {
  const {name, kind} = context

  // ... store some info in context.metadata to tell a class decorator to define `static observedAttributes` based on decorated fields/accessors/getters/setters ...

  if (kind === 'accessor') {
    // In this example, I'm not using the storage initializer, but the extra initializer
    context.addInitializer(function () {
      // store the initial value for setup of the default value to use on attribute removal:
      context.metadata['...'] = this[name]
    })
  }
}

function element(name) {
  return function(Class, context) {
    // ... define `static observedAttributes`, define `Class.prototype.attributeChangedCallback`, add an initializer to call `customElements.define(Class, name)`, etc ...
  }
}

// Usage sample:

@element('some-element')
class SomeElement extends HTMLElement {
  @attribute accessor foo = "blah"
}

(See @lume/element for a real implementation)

One would think they could simply do the same thing with a get/set member:

function attribute(_, context) {
  const {name, kind} = context

  // ... store some info in context.metadata to tell a class decorator to define `static observedAttributes` based on decorated fields/accessors/getters/setters ...

  if (kind === 'accessor' || kind === 'getter') {
    // In this example, I'm not using the storage initializer, but the extra initializer
    context.addInitializer(function () {
      // store the initial value for setup of the default value to use on attribute removal:
      context.metadata['...'] = this[name]
    })
  }
}

function element(name) {
  return function(Class, context) {
    // ... define `static observedAttributes`, define `Class.prototype.attributeChangedCallback`, add an initializer to call `customElements.define(Class, name)`, etc ...
  }
}

But that won't work because any field (private or not) that the getter is using for storage will not be ready yet! With public fields, this can be worked around by returning this.__notPrivateStorage ?? theDefaultValue from the getter. But with private fields, a runtime error will happen.

Workaround 1

One way to make it work then, is to require the class decorator to handle the initial value:

function attribute(_, context) {
  const {name, kind} = context

  // ... store some info in context.metadata to tell a class decorator to define `static observedAttributes` based on decorated fields/accessors/getters/setters ...

  if (kind === 'accessor') {
    // In this example, I'm not using the storage initializer, but the extra initializer
    context.addInitializer(function () {
      // store the initial value for setup of the default value to use on attribute removal:
      context.metadata['...'] = this[name]
    })
  } else if (kind === 'getter') {
    // ... store some info in context.metadata for the class decorator to know to handle the getter initial value ...
  }
}

function element(name) {
  return function(Class, context) {
    // ... define `static observedAttributes`, define `Class.prototype.attributeChangedCallback`, add an initializer to call `customElements.define(Class, name)`, etc ...

    return class extends Class {
      constructor() {
        super()
        // ... read the initial value here for any getter that was tracked in context.metadata ...
      }
    }
  }
}

Workaround 2

Another way to solve the issue is to provide a special decorator to use on the getter/setter storage in tandem with the attribute decorator:

@element('some-element')
class SomeElement extends HTMLElement {
  @attribute.initialValue #foo = "blah"

  @attribute get foo() {return this.#foo}
  @attribute set foo(v) {this.#foo = v} 
}

The implementation is omitted, but basically it will track the initial value in the user constructor, rather than in a class decorator's subclass.

Workaround 3

Another way to solve it is to provide a finalizer decorator to use on a dummy field that can handle initial values:

@element('some-element')
class SomeElement extends HTMLElement {
  #foo = "meow"

  @attribute get foo() {return this.#foo}
  @attribute set foo(v) {this.#foo = v}

  #bar = "woof"

  @attribute get bar() {return this.#bar}
  @attribute set bar(v) {this.#bar = v} 

  @attribute.finalize #_
}

The implementation is omitted, but the @attribute.finalize decorator will run after all fields, which means it also runs after all get/set extra initializers, and hence it will handle initial values in the user constructor instead of in a class decorator's subclass.

Workaround 4

The least ideal solution imo (because it causes user-specific detail to be duplicated) is to have the user provide the initial value to the decorator:

@element('some-element')
class SomeElement extends HTMLElement {
  #foo = "meow"

  @attribute("meow") get foo() {return this.#foo}
  @attribute set foo(v) {this.#foo = v}

  #bar = "woof"

  @attribute("woof") get bar() {return this.#bar}
  @attribute set bar(v) {this.#bar = v} 
}

As you can see, now the values are duplicated.

TLDR

If the initializers ran at the same time, all of the workarounds could be avoided, complexity reduced.

If running the extra initializers after storage initializers for get/set members adds other complexity to other use cases unlike the one I described above, then I think it is imperative that we allow defining which types of initializers we wish to add (extra initializers that run before storage initializers, or extra initializers that run after storage initializers).

Related conversations:

@rbuckton's "bucket" idea defines an enum of values to pass to addInitializer to specify where we want a initializer to happen:

pzuraq commented 4 days ago

This is admittedly a bit of a confusing ordering. There are a few reasons for the timing differences here altogether, we went through several iterations to get here, so I'll take us through it again to document the reasons.

  1. First, we wanted a way to add extra initialization specifically for class elements that did not have initializers, i.e. @bound method() {}. This was a blocker, as some members did not believe the decorators proposal was worth advancing without this capability. However, other members believed this was too dynamic.

    Basically, at parse time, WITHOUT running the class code itself, we can tell certain things about the class. For instance, we know that for every class field we will need to run some initialization code. This is what gets produced as the final byte code. If decorators had the ability to add extra initialization, then the JIT would have to emit an additional operation for every decorated method defined on the class, because it would have to check at runtime if there was an initializer added by the decorator. This would impact every class instance initialization, and some members thought this was too risky to allow.

    So, the initial solution to this was the @init:decorator syntax that was widely panned. The point of this syntax was that by adding @init:, you were adding a token that the parser and JIT could use to add that extra operation ONLY in that case, thus eliminating the performance hazard.

  2. I spent a lot of time trying to figure out a way around this that could get consensus, because I believed pretty strongly that forcing every usage of a decorator to have @init was a way to create widespread issues and bugs. Eventually, we settled on a solution that got consensus: Instead of having the extra initializers run lexically, interspersed between standard field initializers, they would all run before class fields were assigned.

    The logic here was that these methods needed to be initialized before user code began running in the first place, because otherwise user code would be able to call them in an incomplete, half-initialized state. When we realized this, it all fell into place and everyone was onboard, and we were able to get rid of @init:. This was one of the final updates that got us to advance toward stage 3.

  3. We ran into an issue with class field/accessor initializers at this point. At this point, they run in the same order as decoration, so from inner-most decorator to outer-most decorator. This seemed logical because it was the same ordering as how get and set were applied to decorators.

    However, this issue popped up: https://github.com/tc39/proposal-decorators/issues/508

    Essentially it was impossible to assign the initial value properly for an accessor because decorated initializers would run in the reverse order of decorated setters. Took a while to realize this, but once we saw it it was a glaring issue.

    So we got consensus to reverse the order specifically for field/accessor initializers, but NOT extra initializers - those would remain in the same order.

  4. Lastly, the changes in 3 ended up causing some regressions, see this final comment in the same issue thread: https://github.com/tc39/proposal-decorators/issues/508#issuecomment-1664411457

    So the final change that was made was for auto-accessor and field extra-initializers to run right after their own standard-initializers. This allowed them to decorate the initialized value to be basically the same as if it were decorating a non-initialized prototype value.

So that's why we're here. Now, a few thoughts on where we could go from here:

  1. Your example could be rewritten like so:
function attribute(_, context) {
  const {name, kind, access, metadata} = context

  if (kind === 'accessor' || kind === 'getter') {
    context.addInitializer(function () {
      context.metadata['...'] = () => access.call(this);
    });
  }
}

This would make the access call to the value lazy, which would prevent it from running until after all of the initialization code had completed. But, you wouldn't be able to cache the initial value of that property in this way.

  1. @rbuckton proposed a few times that we should be able to specify the timing of initializers dynamically, e.g. you would be able to say "after class initialization" or "before class initialization" or something like that. I do like that idea, but when it was brought up it was panned as too dynamic and so we weren't able to get consensus on it.

  2. I think maybe the better way to approach this would be to modify the behavior of setters possibly. Consider:

function attribute(value, context) {
  const {name, kind, access, metadata} = context

  if (kind === 'accessor' || kind === 'setter') {
    return { 
      set() {
        context.metadata['...'] = { dirtied: true } 
      }
    }
  }
}