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

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

Open
trusktr opened this issue Dec 21, 2023 · 18 comments
Open

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

trusktr opened this issue Dec 21, 2023 · 18 comments

Comments

@trusktr
Copy link
Contributor

trusktr commented Dec 21, 2023

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
Copy link
Member

ljharb commented Dec 21, 2023

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

@trusktr
Copy link
Contributor Author

trusktr commented Dec 21, 2023

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
Copy link
Contributor Author

trusktr commented Dec 21, 2023

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
Copy link
Member

ljharb commented Dec 21, 2023

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
Copy link
Contributor Author

trusktr commented Dec 21, 2023

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
Copy link
Contributor Author

trusktr commented Dec 21, 2023

(I reported the Babel bug,

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

@ljharb
Copy link
Member

ljharb commented Dec 21, 2023

methods don't initialize tho, only fields do.

@trusktr
Copy link
Contributor Author

trusktr commented Dec 21, 2023

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
Copy link
Contributor Author

trusktr commented Dec 21, 2023

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 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
Copy link
Member

ljharb commented Dec 21, 2023

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
Copy link
Contributor Author

trusktr commented Dec 21, 2023

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
Copy link

Wouldn't this be fixed by #513 ?

@ljharb
Copy link
Member

ljharb commented Dec 22, 2023

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
Copy link
Contributor Author

trusktr commented Dec 22, 2023

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
Copy link
Contributor Author

trusktr commented Dec 22, 2023

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
Copy link
Member

ljharb commented Dec 22, 2023

Gotcha, thanks for clarifying.

@trusktr
Copy link
Contributor Author

trusktr commented Apr 28, 2024

@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
Copy link
Member

ljharb commented Apr 28, 2024

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

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

3 participants