Support for decorators stage 3 proposal with accessor

[WearyMonkey]

The decorators spec has progressed to stage 3 and has introduced the new accessor keyword and the ability to capture initial field values. On the surface these features seem to resolve the mobx decorator incompatibility with the previous static decorator proposal.

Is there any plan to introduce support for decorating accessor fields without the need for makeObservable in future mobx versions? Or are you currently waiting for further progress of the proposal, e.g. stage 4 or TypeScript support?

Context: we currently have a very large code base on mobx 5. We would like to upgrade to mobx 6 but consider it a risk due to the required makeObservable, which if missing will cause subtle runtime issues.

Ideally, if mobx were to support the accessor keyword in a future version, we would be able to leapfrog mobx 6 to the next version.

https://github.com/tc39/proposal-decorators#class-auto-accessors
https://github.com/tc39/proposal-decorators#class-fields

Links to related issues:
#1928
tc39/proposal-decorators#267
#1732
microsoft/TypeScript#48686 (Some discussion in TS about stage 3 support)

Thank you for all your awesome work!

[mweststrate]

MobX 6 deprecated decorators exactly because we want to be able to switch to the standard proposal in the future. I've been involved designing the proposal, and we'll pick it up further once a transpilers are available.

Context: we currently have a very large code base on mobx 5. We would like to upgrade to mobx 6 but consider it a risk due to the required makeObservable, which if missing will cause subtle runtime issues.

Please note that a codemod is available

[WearyMonkey]

Do you think makeObservable will be required after switching to the new standard? Or is that unknown at this point?

[phiresky]

we'll pick it up further once a transpilers are available.

The new decorators support has finally been merged to TypeScript! It's part of [email protected] and enabled without any config flags (by removing experimentalDecorators: true from the tsconfig).

[mweststrate]

In principle no, it's a stopgap. But please note that proposals move slowly, this one has taken 7 years at least so far.

…

On Sat, 23 Apr 2022, 23:31 Toby Rahilly, ***@***.***> wrote: Do you think makeObservable will be required after switching to the new standard? Or is that unknown at this point? — Reply to this email directly, view it on GitHub <#3373 (reply in thread)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAN4NBDQYX6ISLLDOWL6LR3VGR23ZANCNFSM5T56BUPQ> . You are receiving this because you commented.Message ID: ***@***.***>

[phiresky]

Just wanted to mention that the new (stage 3) decorators support is about to land in TypeScript Nightly with this PR: microsoft/TypeScript#50820 and is slated to be released in TypeScript 5.0 (the next version): microsoft/TypeScript#51362

Has the time come to revisit this? :)

[gilamran]

Typescript 5 will support stage 3 decorators, do you plan supporting it?

[mweststrate]

yes :) Practically not sure yet when.

[Matchlighter]

@mweststrate Can I ask where 2022.3/stage 3 decorators place on your timeline/priorities and whether you've outlined any plans/requirements/test cases? This would be a big feature for me both at home and work, so I want to do a PR if you (or someone else on the MobX team) are available to jump on reviews and bounce with me on iterations.

[mweststrate]

@Matchlighter I was planning to look into that ~end march / april. I didn't outline any plans or test cases, but in principle we have all the test cases as we still have tests that test decorator support, where the important bit is that they should still work even when makeObservable(this) is not called in the constructor (that should be a no-op). But happy to review PRs if you are willing to look into it! Upfront I don't give merge guarantees pending on code quality, implementation alignment etc etc as a disclaimer. That being said, happy to receive the help, and happy to give pointers where needed!

[Matchlighter]

Cool cool.
Digging through the code base, I'm seeing a lot of work around "Annotations" and make_/extend_. The first approach I could see would be to just tie into these and (effectively) use an addInitializer (from the 2022.3 decorator spec) callback to essentially call makeObservable(this, { decoratee: observable }).
However, while not totally incompatible with 2022.3 decorators, it poses some large limitations, particularly when wanting to chain decorators. For example, given:

class Thing {
  @custom_decorator
  @observable
  accessor my_field = 4
}

Using the current annotation system (either with makeObservable or piggy backing off it with addInitializer) will end up re-definePropertying the my_field property when it later make_/extend_/applies the annotations. Due to that, my implementation plan would be to circumvent the annotation system and use the standard decorator API for modifying/wrapping the decoratee, using addInitializer if/where it's necessary. My thought is to inject a if (typeof arg2 == 'object' && typeof arg2.kind == 'string') clause into the api-domain functions (eg above line 98 of

mobx/packages/mobx/src/api/observable.ts

Lines 95 to 100 in faf075c

	* @param v the value which should become observable.
	*/
	function createObservable(v: any, arg2?: any, arg3?: any) {
	// @observable someProp;
	if (isStringish(arg2)) {
	storeAnnotation(v, arg2, observableAnnotation)

) to catch the decorator use case and apply expected get/set/call hooks.

Stated more shortly - I plan on keeping the annotation API for backwards compatibility, but bypassing it when using 2022.3 decorator syntax. (Unless you want to go all out and drop another major version, but I'm rarely able to sell that proposition :P)

I foresee a possible breaking change in that interceptors as they're used in the many if (hasInterceptors(this)) blocks in https://github.com/mobxjs/mobx/blob/main/packages/mobx/src/types/observableobject.ts may not be applicable - is there a time that interceptors would be added to an object before it calls makeObservable()?

This approach does mean that a lot of the existing code (the complete annotation system) will be unused by the decorator code, and thus there may be some (though I think minor) duplication.

What are your general thoughts/👍-get-coding/👎-something's-off given that schload of info?

[Matchlighter]

Well, I got bored/excited and jumped in either way! I've found a way to extend the Annotation interface to handle decorators. Still having to duplicate some small bits of code, but this at least keeps everything neatly in one place.

export type Annotation = {
    annotationType_: string
    make_(
        adm: ObservableObjectAdministration,
        key: PropertyKey,
        descriptor: PropertyDescriptor,
        source: object
    ): MakeResult
    extend_(
        adm: ObservableObjectAdministration,
        key: PropertyKey,
        descriptor: PropertyDescriptor,
        proxyTrap: boolean
    ): boolean | null
    decorate_20223_( // <<-- Added
        value: any,
        context: DecoratorContext
    )
    options_?: any
}

I think I've got most of the primary cases addressed, but am calling it a night. I'm hoping to figure out the test suite tomorrow and ideally have an early PR up as well.

Current complete code at https://github.com/Matchlighter/mobx/tree/decorators2022

[belaczek]

Technical question: Once the stage 3 decorators support is implemented in mobx, is it possible that the requirement for useDefineForClassFields: true will be removed? Or is that expected to stay?

[Matchlighter]

I've got a PR open for Stage 3 Decs. My approach was to go the way ES was going, so I wrote and tested things with useDefineForClassFields: true. That said, I don't think I did anything that would actually require useDefineForClassFields: true, so it's likely up to how TS handles Decorators when useDefineForClassFields is disabled.
My PR also keeps support for the current makeObservable approach which does have an expectation of useDefineForClassFields: true, so my guess is that you could set it to false, but you'd have to entirely use decorators and make sure not to use makeobservable.
But all that's a mildly-educated guess / "in theory", so apply salt liberally.

[belaczek]

Thanks for your answer!

guess is that you could set it to false, but you'd have to entirely use decorators and make sure not to use makeobservable

This would be great if true, since it would basically become backward compatible with how mobx 5/4 works. Looking at download numbers, there still seem to be quite many projects stuck on older mobx versions because they're probably unable to migrate to useDefineForClassFields: true, including us.

[kubk]

@belaczek useDefineForClassFields makes TypeScript's property transpilation match ECMAScript's behavior. If you specify compilerOptions.target as ESNext or ES2022 you already get useDefineForClassFields turned on by default. It is something that people have to migrate eventually:

• https://angular.schule/blog/2022-11-use-define-for-class-fields
• https://www.typescriptlang.org/tsconfig#useDefineForClassFields

[Matchlighter]

Yeah, I'm one of those numbers, more though b/c I couldn't give up decorators! :P

I can't recall exactly what the issue with useDefineForClassFields was and what all it impacts, but from what I can tell, the main issue as it affects MobX is makeAutoObservable() and it would overwrite defineProperty calls made by MobX 4/5 and 3rd-party decorators. (See also #3216).

The accessor keyword seems to fix the latter issue for both legacy and Stage 3 Decs.

Looking at the TS playground, neither Stage 3 nor legacy decorator implementation are directly affected by toggling useDefineForClassFields.
https://www.typescriptlang.org/play?strict=false&noImplicitAny=false&strictNullChecks=false&strictFunctionTypes=false&strictPropertyInitialization=false&strictBindCallApply=false&noImplicitThis=false&alwaysStrict=false&emitDecoratorMetadata=false&useDefineForClassFields=true#code/FAYwNghgzlAEAqBTKAXWBvYtuwAIgHsBbABwFcVEATWAc0RQAoBKDLHDgJwbM4DtYAVgDc7bAF8xeAgCMoiTgDcIMsIlgQQIZFAKdYKABYBLPrVgBeIaI65Z8pSrUatOvQZNmATMCl25Csqq6hBGpuZWIn72gU4hYd6ikkA

As kubk noted, transitioning to useDefineForClassFields is definitely ideal and will likely be the most future proof, but getting there is certainly a pain to say the least.

TL;DR, I think Stage 3 Decorators and my implementation of them in MobX will work without useDefineForClassFields.

[TechQuery]

@Matchlighter This is Proposal issues of Class fields: tc39/proposal-class-fields#329

[dartess]

@Matchlighter Sorry for the naive question, but I don't understand something.
If we have accessors, do we really still need @action? I'm not familiar with the internals of mobx, but when I saw the accessor decorators, I immediately thought that it would be cool to throw out thousands of dumb actions, because now it is possible to do something like runInAction in the accessor decorator setter and just use this.field = newValue. I'm naive and it's impossible?

[Matchlighter]

Put that way, @action was never a requirement. The primary purpose of @action is so MobX can batch changes. For example, if I were to write:

this.x = 5;
this.y = 5;

and I had some component that rendered this.x + this.y, the component would end up rendering twice - once when x changes and once when y changes. But if I instead wrap that in an action (either via runInAction or via @action):

runInAction(() => {
  this.x = 5;
  this.y = 5;
})

the component would only render once - when the action is over. This is true of actions calling other actions as well - the component will only render once the outer-most action closes.

MobX does have a config option that allows you to change observables outside of actions, but such is viewed as bad practice since it increases risk of needless rendering.

[mweststrate]

Just merged :)

[gilamran]

Great, but I must say that I'm a bit confused on how my configuration should look like in order to use decorators on my MobX code

• What's the minimal TypeScript version that I must use?
• Should I use experimentalDecorators: true on my tsconfig.json ?
• Same question regarding useDefineForClassFields
• Do I still have to call makeObservable on my constructors?

Thanks!

[mweststrate]

See https://mobx.js.org/enabling-decorators.html

…

On Sat, 11 Nov 2023, 18:45 Gil Amran, ***@***.***> wrote: Great, but I must say that I'm a bit confused on how my configuration should look like in order to use decorators on my MobX code - What's the minimal TypeScript version that I must use? - Should I use experimentalDecorators: true on my tsconfig.json ? - Same question regarding useDefineForClassFields - Do I still have to call makeObservable on my constructors? Thanks! — Reply to this email directly, view it on GitHub <#3373 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAN4NBG6VYAIJUBK6TX3IFTYD62UJAVCNFSM5T56BUP2U5DIOJSWCZC7NNSXTOKENFZWG5LTONUW63SDN5WW2ZLOOQ5TONJUGE2TSOA> . You are receiving this because you modified the open/close state.Message ID: ***@***.***>

[dartess]

I still don't understand one thing.

First the docs says:

With modern decorators, it is no longer needed to call makeObservable / makeAutoObservable.

Then the docs says:

@action some_field = () => {} was and is valid usage (if makeObservable() is also used). However, @action accessor some_field = () => {} is never valid.

So do I still need makeObservable or not if I have actions in my code? Does this only apply to actions declared as properties? What about actions declared as methods, but decorated with action.bound?

I believe that this piece is completely unclear and requires rewriting.

[clins1994]

Confusing indeed