tc39 / proposal-decorators

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

Field and Accessor initializers should run after the field/accessor has been defined #513

Closed pzuraq closed 10 months ago

pzuraq commented 1 year ago

Currently, field/accessor initializers added via addInitializer run at the same time as method initializers. This means they run before their respective field/accessor is defined, which means if they intend to modify the field/accessor they cannot do so.

The ordering should be updated so that they run immediately after the field/accessor they were applied to is defined. Conceptually, this should not be a performance concern because it would be immediately after the init code for the field/accessor was run, so it should slot in at the same point in class instance creation, but we should double check that with implementers.

cc @syg

syg commented 1 year ago

Gut reaction is this is probably fine? The "instance shape is statically analyzable" property still holds, it's just the timing of the initializer?

pzuraq commented 1 year ago

Yes, exactly, initializers for fields/accessors would just run immediately after the field/accessor is initialized and defined on the instance, no other changes

rbuckton commented 1 year ago

I thought the point of addInitializer was to run code before any initializers are run? Will changing this affect other use cases?

pzuraq commented 1 year ago

@rbuckton initialization prior to fields was for methods/accessors, which are defined on the prototype. Initialization of fields/accessors via addInitializer was actually not included in the proposal at first because it was seen as redundant, but we decided to include it to simplify the API. There is now a use case for initializers added to fields/accessors, see https://github.com/tc39/proposal-decorators/issues/508#issuecomment-1664411457

mweststrate commented 1 year ago

Sorry, I have been out of the loop for a while, so correct me if wrong, but I think in MobX we did run into the same limitation: https://github.com/mobxjs/mobx/pull/3638/files#diff-6df715307a8821749b3d5d5b0e8a6ad7360ed2fae4b01f7005b6c7b3efb3336bR69-R108

Edit: can't make it to the meeting, but sounds like I'm in favour of the change :)

justinfagnani commented 1 year ago

This change may help us with accessor initialization as well.

We have a case where on construction we want to read and delete instance properties that are shadowing accessors and write the value back though the accessor. (this may seem odd, but the instance exists before the constructor is called, and in this case someone has assigned properties to what will be auto-accessors after construction).

We would like to just add an initializer that does this operation, but we can't write to the accessor in an initializer because the private storage for the accessor hasn't been installed yet, resulting in an error. So we have to write the value asynchronously, which is a bit of a convoluted code path and leaves the object in a weird state for a tick.

So just to confirm that this helps us, an initializer for an accessor added with addInitializer() (what about init()?) will run after the private storage is installed so it can access the accessor, yes?

pzuraq commented 1 year ago

So just to confirm that this helps us, an initializer for an accessor added with addInitializer() will run after the private storage is installed so it can access the accessor, yes?

Yes, correct

what about init()?

init will run during initialization of the accessor, before it is defined, just like it does currently

justinfagnani commented 1 year ago

Was this resolved at yesterday's meeting?

pzuraq commented 1 year ago

Nobody else was available so we didn’t discuss it. My plan is to bring the change to committee at the next plenary I’m able to attend.

trusktr commented 1 year ago

@justinfagnani how would being able to access the auto-accessor storage help with custom element pre-upgrade values? Can you describe that in more detail?

trusktr commented 11 months ago

I have a different case:

There I got runtime errors because my decorator wants to know initial values of getters, but fails when getters read a private field (and will silently fail with public instead of private fields due to fields not being initialized yet). The example there would work nicely if the addInitializer initializer ran after fields.

Judging from conversations, it seems like there are uses cases for running addInitializer initializers before or after fields (f.e. what if a field wants to use a method, and the method decorator should initialize it so that it is ready before fields are initialized?), but it seems most people want them after.

Would it make more sense to change addInitializer in the current spec to be after fields? Or more sense to add an alternative API that lets people choose before/after fields as described in #521?

pzuraq commented 10 months ago

This change has been merged. @trusktr this will not address your issue because addInitializer for getters and setters still runs prior to field instantiation. We can discuss more in #521.