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

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

Closed
pzuraq opened this issue Aug 3, 2023 · 12 comments

Comments

@pzuraq
Copy link
Collaborator

pzuraq commented Aug 3, 2023

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

syg commented Aug 7, 2023

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

pzuraq commented Aug 7, 2023

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

rbuckton commented Aug 7, 2023

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

@pzuraq
Copy link
Collaborator Author

pzuraq commented Aug 8, 2023

@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 #508 (comment)

@mweststrate
Copy link

mweststrate commented Aug 8, 2023

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

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

pzuraq commented Aug 8, 2023

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

Was this resolved at yesterday's meeting?

@pzuraq
Copy link
Collaborator Author

pzuraq commented Aug 9, 2023

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

trusktr commented Nov 20, 2023

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

trusktr commented Dec 22, 2023

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

pzuraq commented Feb 2, 2024

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.

@pzuraq pzuraq closed this as completed Feb 2, 2024
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

6 participants