tc39 / proposal-decorators

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

feature request (separate proposal?): `context.addPostInitializer()` #521

Open trusktr opened 11 months ago

trusktr commented 11 months ago

Non-field initializers added with context.addInitializer() run before field initializers according to the README, making it too easy to have problems with private fields. The README says:

Class element initializers run during class construction, before class fields are initialized.

Example:

class Example {
  #foo = 123

  @deco
  get foo() { return this.#foo }
}

function deco(_, context) {
  context.addInitializer(function () {
    console.log('initial value:', this[context.name]) // runtime error
  })
}

The error will be something like:

Cannot read private member #foo from an object whose class did not declare it

With TypeScript the error is thrown (correct, based on proposal-decorator's README):

TS playground

Babel does not throw, seems to run the initializer after class fields and the private field value is ready:

Babel repl

Feature Request

Assuming TypeScript and README are correct and Babel is wrong, it would be nice to have context.addPostInitializer(). It would work just like Babel's context.addInitializer() currently does, but officially:

class Example {
  #foo = 123

  @deco
  get foo() { return this.#foo }
}

function deco(_, context) {
  context.addPostInitializer(function () { // hypothetical added API
    console.log('initial value:', this[context.name]) // no more runtime error
  })
}

If it is actually TypeScript and README that are incorrect and Babel is correct, then a converse context.addPreInitializer() API could be added. There probably are use cases for both.

In my case, I want to use private fields, but I also want to grab getter initial values by reading them, and it's just too bad that private fields are getting in the way here. I don't want to regress back to __private naming convention.

ljharb commented 11 months ago

You can do that already with a class initializer that replaces the constructor function.

trusktr commented 11 months ago

Of course, I know I can work around it with a convoluted class decorator + getter decorator setup, but it is definitely not ideal.

I don't want to have to do the following for this case:

@classDeco
class Example {
  #foo = 123

  @deco
  get foo() { return this.#foo }
}

function deco(_, context) {
  context.addInitializer(function () {
    // implement coordination with the class deco
  })
}

function classDeco(Class) {
  class SubClass extends Class {
    constructor() {
      // implement coordination with the getter deco
    }
  }
}

I'm already familiar with this approach, this is how I avoid having to use accessor on all my class fields to make them reactive.

A context.addPostInitializer() (or whatever name it would have) would simply solve the issue, and is fully backwards compatible with the current spec (assuming TS and README are correct).

trusktr commented 11 months ago

Babel's behavior is the ideal default. Having a getter and its decorator break due to a private field in the very same class is a bad experience.

ljharb commented 11 months ago

That seems like it'd allow you to easily write a class constructor that interacts with subclass constructors after they've run, which seems like a very bad idea - or are you just talking about a hook for a specific field that runs immediately after it's been initialized by the user? If so, can't you also already do that with a field decorator that wraps the initializer function's getter and setter? (that might not be on initialization, but it'd be on first use, at least)

trusktr commented 11 months ago

That seems like it'd allow you to easily write a class constructor that interacts with subclass constructors after they've run,

If you mean this,

function getterDeco(_, context) {
  context.addInitializer(function () {
     doSomethingWith(this.constructor)
  })
}

that's already possible with context.addInitializer. The new context.addPostInitializer would simply run immediately after field initializers, instead of immediately before (this does not have anything to do with subclasses).

Or did you mean something else?

can't you also already do that with a field decorator that wraps the initializer function's getter and setter? (that might not be on initialization, but it'd be on first use, at least)

Not sure what you mean. Got an example? I'm decorating a getter, not a field.

trusktr commented 11 months ago

(I reported the Babel bug,

which I wish was not a bug but the default behavior)

ljharb commented 11 months ago

methods don't initialize tho, only fields do.

trusktr commented 11 months ago

I'm not sure what you're getting at, but getter decorators have context.addInitializer to run an initializer before field initializers. I want to run a getter initializer after field initializers.

trusktr commented 11 months ago

The suggestion of using a class decorator @ljharb also means that user code in the user's constructor body will not be operating on the desired state of the instance, only code after their constructor (i.e. the class decorator's subclass constructor) will finalize the state of the instance. This makes the constructor unreliable for the user to write code in except for minimal actions like assigning some variables (this is same problem with avoiding accessor using a class decorator because the fields are patched in the decorator subclass after the user's code).

A context.addPostInitializer() API would be far more ideal for this use case.

ljharb commented 11 months ago

I’m not sure what you mean by “reliable”; can you elaborate on your use case?

(either way any additive changes to decorators must happen in a separate proposal, regardless; this one’s stage 3)

trusktr commented 11 months ago

I’m not sure what you mean by “reliable”; can you elaborate on your use case?

If a user uses a decorator, they might want to write code in their constructor that relies on the feature of the decorator, which may not be established until the decorator subclass after the user constructor.

If the behavior of the class is defined in a decorator subclass instead of the user's class (instead of a member decorator's initializer), then the user's code in the user's own constructor will not behave as expected.

For example:

@isReactive class MyClass {
  @reactive foo = 123

  constructor() {
    createEffect(() => {
      console.log(this.foo)
    })
  }
}

In that example, createEffect is an API that runs a function and track reactive variable accesses, then it re runs the function when any reactive variables used inside the function body change later.

But because the decorator's subclass constructor has not yet run in the user's constructor, this.foo will not be reactive because it has not yet been patched, therefore the function passed to createEffect() will not automatically re-run on future updates to this.foo, because it that example was equivalent to writing this:

@isReactive class MyClass {
  @reactive foo = 123

  constructor() {
    createEffect(() => {
      console.log(123) // no reactive variable is accessed here, just a plain value, therefore it cannot be reactive.
    })
  }
}

Same problem with coordinating between a getter decorator and class decorator, only the decorator subclass will be able to set things up.

(either way any additive changes to decorators must happen in a separate proposal, regardless; this one’s stage 3)

From a discussion could follow a proposal. Do I make one in a repo under my github username?

justinfagnani commented 11 months ago

Wouldn't this be fixed by #513 ?

ljharb commented 11 months ago

Decorators on instance things are available after a base class constructor, and after super in a derived class constructor. That’s pretty intentional and unlikely to change.

trusktr commented 11 months ago

Wouldn't this be fixed by #513 ?

@justinfagnani If that applies for getter/setter initializers, then yeah, otherwise no.

But based on conversations, it seems like we really need more flexibility to choose before vs after. It depends on use case.

A method decorator may want to addInitializer to run before class fields, so that fields can use the result. But a getter/setter decorator like in the above example would want to run after field initializers.

We just need a more flexible API.

But how? Seems like people want either before or after field decorators. And would it be better for all non-field initializers to run after fields by default?

trusktr commented 11 months ago

Decorators on instance things are available after a base class constructor, and after super in a derived class constructor. That’s pretty intentional and unlikely to change.

I don't think anyone proposed otherwise.

The order of getter/setter vs field vs method vs accessor vs etc doesn't change the fact that they all run after super (or at top of constructor in a base class), and currently field accessors run after getter/setter initializers.

ljharb commented 11 months ago

Gotcha, thanks for clarifying.

trusktr commented 7 months ago

@rbuckton's "bucket" idea,

is essentially asking for the same thing in a different way, if I'm not mistaken.

The example in the OP throwing a runtime error seems totally bad. What's the reason we want it to be that way?

ljharb commented 7 months ago

private fields aren’t “names”; you can only access them syntactically.