[breaking change] Get rid of field initializers (and legacy decorators)

[Kukkimonsuta]

Enabling useDefineForClassFields in TypeScript will prevent decorators from working (both transpiled and using mobx.decorate).

This flag will be enabled by default for ESNext once class fields go stage 4: https://github.com/microsoft/TypeScript/issues/34787

Possibly related to https://github.com/mobxjs/mobx/issues/1969

Intended outcome:

autorun using objectState is executed upon clicking on "increment" button. autorun using classState is executed upon clicking on "increment" button. autorun using classStateNoDecorator is executed upon clicking on "increment" button.

Actual outcome:

✅ autorun using objectState is executed upon clicking on "increment" button. ⛔️ autorun using classState is NOT executed upon clicking on "increment" button. ⛔️ autorun using classStateNoDecorator is NOT executed upon clicking on "increment" button.

How to reproduce the issue:

https://codesandbox.io/s/fragrant-frog-x2487

Versions

TypeScript 3.7+ MobX - all tested versions (4.x, 5.x)

[danielkcz]

Thank you for a nice heads up. So basically we can just be explicit and set that flag false to avoid that default change, right? Would you mind creating a PR for that?

[danielkcz]

Oh wait, I got that all wrong (kinda sleepy). This isn't about mobx building, but users running TS 3.8 will have this problem... Well that goes beyond my expertise why is it a problem.

Basically this...

class State {
    constructor() {
        this.value = 0;
    }
}

Becomes...

class State {
    constructor() {
        Object.defineProperty(this, "value", {
            enumerable: true,
            configurable: true,
            writable: true,
            value: 0
        });
    }
}

And MobX cannot handle that for some reason.

TS Playground

/cc @mweststrate @urugator

[mweststrate]

Nope, I protested heavily when TC 39 proposed this, for reasons like this, but to no avail.... Field initializers will be non-interceptible once this finalizes, so the assignments will have to move to the constructor instead

On Sat, Feb 15, 2020 at 10:07 PM Daniel K. notifications@github.com wrote:

Oh wait, I got that all wrong (kinda sleepy). This isn't about mobx building, but users running TS 3.8 will have this problem... Well that goes beyond my expertise why is it a problem.

Basically this...

class State { constructor() { this.value = 0; } }

Becomes...

class State { constructor() { Object.defineProperty(this, "value", { enumerable: true, configurable: true, writable: true, value: 0 }); } }

And MobX cannot handle that for some reason.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/mobxjs/mobx/issues/2288?email_source=notifications&email_token=AAN4NBDQTWQWA7C3GKZV6MDRDBRR7A5CNFSM4KV4PIV2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEL3YB3I#issuecomment-586645741, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAN4NBAATFBXU4AXHN3MGV3RDBRR7ANCNFSM4KV4PIVQ .

[Kukkimonsuta]

So if I understand this correctly to support classes with this we will need to resort to something like:

import { observable, extendObservable } from 'mobx';

class State {
    constructor() {
        extendObservable(this, {
            value: this.value,
        });
    }

    value = 0;
}

or maybe

import { observable, initializeObservables } from 'mobx';

class State {
    constructor() {
        initializeObservables(this);
    }

    @observable value = 0;
}

That's really unfortunate 😭

[danielkcz]

Or ditch classes :) We just had a short conversation yesterday how decorators are annoying and probably never going to get finished. Sure, the problem remains with decorate too, but question is, what benefit have classes in that case?

In mobx-react we have useLocalStore which is semi clever and can convert passed in object to observable where methods are actions. It is opinionated, but we could expand on that idea and decorate could operate on objects, not just classes.

I understand it's not the best solution for existing code, nobody is going to migrate from classes, but it's at least some path forward.

We should probably add a big warning to README here for TS users so they can disable that config option.

@xaviergonz What are your opinions here? I suppose that mobx-keystone is going to be affected by this too.

[Kukkimonsuta]

Ditching classes is not really an option for us, we're using them extensively through our applications. They certainly have their shortcomings, but they also provide us with some limited reflection capabilities which come in handy. I think being forced to trigger initialization in constructor, but keeping decorate/observable is better way to go. You could even make ObservableObject to inherit from keeping the behavior pretty close to what it is now. People don't like inheritance chains in JS, but it surely beats rewriting to not use classes or using extendObservable duplicating field declarations.

import { observable, initializeObservables } from 'mobx';

class ObservableObject {
    constructor() {
        initializeObservables(this);
    }
}

class State extends ObservableObject {
    @observable value = 0;
}

[danielkcz]

I did not mean ditch classes like remove complete support for them. Just to think of them more like legacy patterns and move forward with something with fewer shortcomings.

Can you elaborate on what reflection capabilities they provide? I am not familiar with that.

Your workaround definitely makes the most sense at this point. But why not to disable that TS option instead of modifying the code?

[Kukkimonsuta]

I'm talking mostly about instanceof and being able to use discriminated unions without hassle:

interface TodoApi {
    create(todoCreate: TodoCreate): Promise<Todo | ValidationState>;
}

onCreateClick = async () => {
    // you can recognize what was returned without having extra fields
    // or wrapping the results
    const result = await this.todoApi.create(this.form);
    if (result instanceof ValidationState) {
        this.errors = result;
        return;
    }

    this.router.goTo(`todos/${result.id}`);
}

[mweststrate]

Afaik class { @observable x; constructor () { this.x = 3 } } will still work.

Op zo 16 feb. 2020 12:32 schreef Lukáš Novotný notifications@github.com:

I'm talking mostly about instanceof and being able to use discriminated unions without hassle:

interface TodoApi { create(todoCreate: TodoCreate): Promise<Todo | ValidationState>; } onCreateClick = async () => { // you can recognize what was returned without having extra fields // or wrapping the results const result = await this.todoApi.create(this.form); if (result instanceof ValidationState) { this.errors = result; return; }

this.router.goTo(`todos/${result.id}`);

}

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mobxjs/mobx/issues/2288?email_source=notifications&email_token=AAN4NBHOGX3HOCLKVKMCHW3RDEW4XA5CNFSM4KV4PIV2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEL4FSZA#issuecomment-586701156, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAN4NBA23DXBJIFV2KIKPXTRDEW4XANCNFSM4KV4PIVQ .

[xaviergonz]

I can think of a way to make it work automatically, but it requires "overriding" Object.defineProperty for certain scenarios:

console.clear()

const propOverridesSymbol = Symbol()

const origDefineProperty = Object.defineProperty;
Object.defineProperty = function(o: any, p: string | number | symbol, attributes: PropertyDescriptor & ThisType<any>): any {
  const overriden = o[propOverridesSymbol]?.has(p); // will get it from prototype if available
  if (!overriden) {
    return origDefineProperty(o, p, attributes);
  } else {
    // just set value
    o[p] = attributes.value;
    return o;
  }
}

function markAsOverridenDefineProperty(o: any, p: string | number | symbol) {
  if (!o[propOverridesSymbol]) {
    // should be a hidden prop instead in a final implementation
    o[propOverridesSymbol] = new Set(); 
  }
  o[propOverridesSymbol].add(p);
}

function deco(proto: any, key: any): any {
  Object.defineProperty(proto, key, {
    get: function() {
      console.log("deco get")
      return Reflect.get(this, key + "int");
    },
    set: function(value) {
      console.log("deco set", value)
      return Reflect.set(this, key+ "int", value);
    }
  });

  markAsOverridenDefineProperty(proto, key);
}

class State {
  @deco value = 0;
}
const classState = new State();
// prints deco set 0

classState.value = 10;
// prints deco set 10

class State2 extends State {
  @deco value2 = 1
}
const classState2 = new State2()

Playground Link

[xaviergonz]

The only place where it wouldn't work is when using ESNEXT as target and useDefineForClassFields, since that transpiles to

class State {
    value = 0;
}

which won't use the modified Object.defineProperty but an internal one.

Btw, this won't work:

class ObservableObject {
    constructor() {
        initializeObservables(this);
    }
}

class State extends ObservableObject {
    @observable value = 0;
}

since in that case the base constructor would get called before the final Object.defineProperty is called. To solve that case (and actually also the problem where the class properties are not transpiled) you could use, ironically, a decorator:

@observableClass
// returns class X extends State {}
// with a constructor that calls the State constructor and then does the defineProperty of observable
// values
class State {
    @observable value = 0;
}

but I guess it is fair to use a decorator to solve a decorator problem?

Or alternatively (as mentioned before) calling a function on the constructor:

class State {
    @observable value = 0;
  constructor() {
    initObservables(this); // would restore overridden defineProperties
  }
}

[mweststrate]

yeah, that is the kind of thinking I have now as well, the only way that pops to my mind to fix it, is indeed by having a class level decorator :) At least the migration path of that wouldn't be too awful, only some tricky edge cases I guess around inheritance

On Sun, Feb 16, 2020 at 3:38 PM Javier Gonzalez notifications@github.com wrote:

The only place where it wouldn't work is when using ESNEXT as target and useDefineForClassFields, since that transpiles to

class State { value = 0; }

which won't use the modified Object.defineProperty but an internal one.

Btw, this won't work:

class ObservableObject { constructor() { initializeObservables(this); } }

class State extends ObservableObject { @observable value = 0; }

since in that case the base constructor would get called before the final Object.defineProperty is called. To solve that case (and actually also the problem where the class properties are not transpiled) you could use, ironically, a decorator:

@observableClass// returns class X extends State {}// with a constructor that calls the State constructor and then does the defineProperty of observable// valuesclass State { @observable value = 0; }

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mobxjs/mobx/issues/2288?email_source=notifications&email_token=AAN4NBGGMIK2G642IR6J5X3RDFMXDA5CNFSM4KV4PIV2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEL4KIZA#issuecomment-586720356, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAN4NBBL2JTGZUYSCMAP3Z3RDFMXDANCNFSM4KV4PIVQ .

[urugator]

I was thinking about...

@decorate({
  value: observable,
})
class State {
  value = 0
}
// no @
decorate({
  value: observable,
})(class State {
  value = 0
})

But personally I would most likely prefer to decorate/extend in constructor (not sure):

class State {
  value = 0
  constructor() {
    decorate(this, {
      value: observable,
    })    
  }
}

EDIT: Or eventually plugin to babel, comment driven definitions?

[mweststrate]

For reference, this is how Ember does it (since basically two month, I guess we have been some inspiration :)):

class Person {
  @tracked firstName;
  @tracked lastName;
  @tracked age;
  @tracked country;

  constructor({ firstName, lastName, age, country }) {
    this.firstName = firstName;
    this.lastName = lastName;
    this.age = age;
    this.country = country;
  }

[Kukkimonsuta]

@mweststrate Could you elaborate why your sample would work? The property is defined no matter whether it has default value or not according to spec.

---

By the way - babel situation is similar:

Not working at all (modern decorators, non-loose fields):

• https://codesandbox.io/s/vigilant-paper-brhij

Only decorators (legacy decorators, non-loose fields):

• https://codesandbox.io/s/condescending-snowflake-lxz8f

Fully working (legacy decorators, loose fields):

• https://codesandbox.io/s/confident-ishizaka-86fkn

I think babel modern decorators could work though: Babel repl sample

---

As far as I can tell:

• calling decorate on class cannot work going forward and should be used only within constructor.
• decorators could work for babel transpilation
• decorators won't work for TypeScript transpiler unless they introduce a way to modify descriptor like babel does for both legacy and modern implementation (which I doubt will happen, but maybe they could be convinced since this change does break basically everyone using decorators for modifying fields).

---

I've spent several hours on researching this today and I feel like the only way forward is @urugator suggestion:

class State {
  value = 0;

  constructor() {
    decorate(this, {
      value: observable
    });
  }
}

[xaviergonz]

The only thing I don't like about solutions inside the constructor (besides that the migration path is a bit harder) is that they force you to write constructors for extended classes. e.g.

class A {
  @observable x = 0
  constructor(a, b, c, d, e) {...}
}

class B extends A {
  @observable y = 0
}

vs

class A {
  x = 0
  constructor(a, b, c, d, e) {
    decorate(this, {x: observable})
  }
}

class B extends A {
  y = 0
  constructor(a,b,c,d,e) {
    super(a,b,c,d,e)
    decorate(this, {y: observable})
  }
}

but other than that I guess it is ok (and actually very cross platform!).

decorate({
  value: observable,
})(class State {
  value = 0
})

That'd need to be something like

const State = decorate({
  value: observable,
})(class {
  value = 0
})

which doesn't get too well with generic classes and typings. e.g.

const State = decorate({
  value: observable,
})(class S<T> {
  value: T = 0
})
type State = typeof State

const s: S<number> = new S<number>(...) // : S<number> won't work

(PS: I still think doing a @observableClass should be viable)

[mweststrate]

@Kukkimonsuta you are right, was still assuming some old behavior where x; wouldn't do anything (beyond giving an optimization hint)

I don't feel that decorators are moving anywhere some, so a change in the proposals (if ever some proposal does get at stage 3 again) might bite us, like field initializers do now, where the final spec deviates from all reference implementations. So ideally we'd find a solution that doesn't rely on decorators at all. (A nice benefit is that it would drop a lot of code from the lib!).

So I think @urugator's proposal are the clearest way forward, where probably the in-constructor variation is the simplest one (constructor wrapping is very hard, and without it we would still need to rely on the ugly babel hack that initializes observables on first read).

I think it should be possible to create code-mods that rewrites the decorators (would someone be interested in building one?). (For babel a babel-plugin-macros could work as well if just the syntax is enabled?)

Still, I'm a bit sad that we will probably worse the DX as a result of the language progressing....

But alas, at least it will work out of the box with CRA.

I hope to experiment a bit further in coming weeks to try and feel what works best

TODO: after some initial poccing, open up a fresh issue to discuss into more detal

[Kukkimonsuta]

Related TypeScript issue here: https://github.com/microsoft/TypeScript/issues/35081

Unless I missed something traps should be still doable when leveraging decorators (= just replacing descriptor, no custom defineProperty calls) as follows:

	Fields	Properties	Methods
Babel "modern"	✅	✅	✅
Babel "legacy"	✅	✅	✅
TypeScript (without "useDefineForClassFields")	✅	✅	✅
TypeScript (with "useDefineForClassFields")	⛔️	✅	✅

Considering TypeScript fields are the outlier we might be able to convince TypeScript team to properly address this.

Code here:

Gist

TypeScript playground

Babel repl

[xaviergonz]

I'd love if TS fixed it on their end, but I think that to address it they'd need to enable some sort of transpliation when a field has a decorator and the target is set to "esnext".

Right now when useDefineForClassFields is true and the target is set to esnext there's absolutely no transpliation involved whatsoever, but I see no way to make it work without transpilations since browsers now internally use defineProperty for class properties.

In other words, they'd need to move the decorate call for fields to the constructor rather than work over the prototype.

[urugator]

I've made a simple PoC babel plugin, transforming:

class A {
  // @whathever 
  field = value;  
}
// into
class A {
  // @whathever 
  field = value;  

  constructor() {
    decorate(this, {
      field: whatever,
    })
  }
}

If there would be interest I think I could turn it into something usable.

[danielkcz]

It sounds like an interesting approach for sure to remove the burden for the glue code from the developer and have it as an automatic tool. It might make more sense to do it under babel-macros, adding extra plugins to CRA is annoying.

[urugator]

It would be more interesting if it would transform the class directly (without the need to call decorate at runtime) and also support @action async fn, @action constructor and @observer (in a way that you wouldn't need mobx-react).

[mweststrate]

Can a babel plugin also be run as a code-mod? I think that would be the most future proof? (as it also removes the dependency on decorator syntax etc)

On Wed, Feb 19, 2020 at 9:54 PM urugator notifications@github.com wrote:

It would be more interesting if it would trasform the class directly (without the need to call decorate at runtime) and also support @action async fn, @action constructor and @observer (in a way you that you wouldn't need mobx-react).

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mobxjs/mobx/issues/2288?email_source=notifications&email_token=AAN4NBHWCNQHQIXOELIOQFDRDWTCNA5CNFSM4KV4PIV2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEMJ2UMA#issuecomment-588491312, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAN4NBFSKUH5JOWYJZQ2HIDRDWTCNANCNFSM4KV4PIVQ .

[danielkcz]

Things might get tricky if you want to support JS & TS projects. At the moment you have a project that's not compiled by Babel, but by TSC, the AST would be different (not sure how much). There is probably a reason why ts-codemod exists.

@urugator I wonder how do you want to achieve reactive functional components with Babel plugins :)

[spion]

For DX I think the best way forward would be to add a class-level decorator in addition to the field level ones, with the field ones defining metadata and the class level one executing on it

edit: and now I realize this is harder than I thought, with proxy traps :)

[Bnaya]

Few thoughts:

1. Somekind of Runtime detection if the user have broken observables due to native class/useDefineForClassFields, and warn about it Also reactionRequiresObservable: true will help the user understand that something is wrong with his mobx setup

2. eslint rules, in a similar manner to rules of hooks, plus auto-fixers. That's can be somewhat of a balance between DX and not needing special transpile step.

[spion]

FWIW here is a solution with a proxy trap. Not sure what the perf implications are:

https://codesandbox.io/s/cool-wave-h4is4

[danielkcz]

@spion That would work only for V5, we cannot use Proxies in V4 because of IE11. Probably better to find a common solution than two different ones imo.

[spion]

I made a proxyless implementation at the same link, however I bet it's going to have worse DX and edge cases than the proxy version.

[mweststrate]

Did a bunch of experiments on how MobX could look when not having decorators, in a world where class fields are initialized with define instead of set. With the constraints:

• only use officially supported modern lang features (so, fields with define, no decorators)
• use classes (the non-class case is covered with current api's well already I think, but abolishing classes doesn't give a clear migration path imho, and classes have benefits unrelated to MobX, such as instanceof and sharing things in the prototype)
• strongly typable

0. base example

class Todo {
  @observable width = 20;
  @observable height = 10;

  @computed
  get surface() {
    return this.height * this.width * this.utility();
  }

  @action
  double() {
    this.width *= 2);
  }

  utility() {
    return 1;
  }
}

1. extendObservable in constructor 🚫

class Todo {
  constructor() {
    extendObservable(this, {
      width: 20,
      height: 10,
      get surface() {
        return this.height * this.width * this.utility();
      },
      double() {
        this.width *= 2);
      }
    });
  }

  utility() {
    return 1;
  }
}

Pro

• exists

Con

• trouble with this typing
• weakly typed (class members not know upfront)

2. decorate after class definition 🚫

class Todo {
  width = 20;
  height = 10;

  constructor() {
    initializeObservables(this);
  }

  get surface() {
    return this.height * this.width * this.utility();
  }

  double() {
    this.width *= 2);
  }

  utility() {
    return 1;
  }
}
decorate(Todo, {
  width: observable,
  height: observable,
  surface: computed,
  double: action
});

Pro

• well typed

Con

• doesn't work with field initializers, unless a utility method is called in constructor. Even providing a base class doesn't work, as it's constructor can't see the members of subclasses

3. decorate in constructor ✅

class Todo {
  width = 20;
  height = 10;

  constructor() {
    decorate(this, {
      width: observable,
      height: observable,
      surface: computed,
      double: action
    });
  }

  get surface() {
    return this.height * this.width * this.utility();
  }

  double() {
    this.width *= 2);
  }

  utility() {
    return 1;
  }
}

Pro

• no hacks needed
• strongly typeds

Con

• messy constructor required
• easy to forget members in large classes

4. generate Observable class ✅

const Todo = Observable(
  {
    width: observable,
    height: observable,
    surface: computed,
    double: action
  },
  class {
    width = 20;
    height = 10;

    get surface() {
      return this.height * this.width * this.utility();
    }

    double() {
      this.width *= 2);
    }

    utility() {
      return 1;
    }
  }
)

Observable generates a class on the fly with a constructor that takes care of initialization

Note, the above class could also be written as follows, which benefits from hoisting and probably better debugger names:

class Todo extends Observable(
  {
    width: observable,
    // etc
  },
  class {
    width = 20;
    // etc
  }
) {} // empty class body

Pro

• declarative
• strongly typed
• verified: this works also correctly with statics and super classes

Con

• uncommon syntax

5. fake decorators 😕

class Todo {
  [observable()]
  width = 20;

  [observable()]
  height = 10;

  constructor() {
    initializeObservables(this)
  }

  [computed()]
  get surface() {
    return this.height * this.width * this.utility();
  }

  [action()]
  double() {
    this.width *= 2);
  }

  utility() {
    return 1;
  }
}

This method abuses computed property keys and leverages the predicatable ordering of class members to create temporary fields (like observable__001) to annotate the behavior of the next member. This works fine technically (I tried), but, typescript doesn't like this for two reasons:

1. all those fields have implicitly any types, so in strict mode this will yel
2. JS does, but TS doesn't allow using dynamic exressions as class member names
   1. a work around is to use [decorators.observable] that hits a getter to work around that

Pro

• actual decoration, no repeating member names!

Con

• Doesn't work with common TS compiler settings (see above)
• hacky. Leaves (at least temporarily) ugly non existing members on the class instances, in TS autocompletion etc
• looks awkward if you format with semicolons ;

6. Field initializers all the way ✅

class Todo {
  width = observable(20);
  height = observable(10);

  constructor() {
    initializeObservables(this)
  }

  surface = computed(() => {
    this.height * this.width * this.utility();
  })

  double = action(() => {
    this.width *= 2;
  })

  utility() {
    return 1;
  }
}

Pro

• quite clear and short
• no double declarations
• strongly typed

Con

• if using function, functions need to be typed with this: Todo as first arg
• when not using function, actions cannot be shared on the prototype (and we can't detect which of the two is done). Slight benefit: at least they're always bound :)
• still needs constructor (or Observable class wrapper)
• computed and observable needs some type hacking or alias so that x: () => T is inferred to x: T. For backward compatiblity we might even need to give them different names, but I don't think so (although that will results in observable members for readonly fields being created potentially)

7. utility fields 😕

class Todo {
  $width = observable;
  width = 20;

  $height = observable
  height = 10;

  constructor() {
    initializeObservables(this)
  }

  $surface = computed
  get surface{
    this.height * this.width * this.utility();
  })

  $double = action
  double() {
    this.width *= 2;
  }

  utility() {
    return 1;
  }
}

Pro

• quite clear, decoratings near members
• can share computeds and actions on proto
• strongly typed

Con

• looks ugly
• leaves additional class members, which will show up in autocompletion, and might influence type checks
• still needs constructor (or Observable class wrapper)
• when renaming fields, won't error statically

8. Proxies 😕

Didn't really try didn't initially feel feasible in my hand, @spion please correct me if your experience is different;

• hard to compile down to old ES versions.
• Generally quite messy in the debugger.
• deals badly with bounded context, e.g. handler = () => this.x++ probably bypasses any proxy in front of the instance

---

N.B. in all of the examples above, needing to define a constructor could be replaced by a wrapper function class X extends Observable(anonymousClass) or const X = Observable(anonymousClass) to achieve the same goal. But adding / extending just a constructor with a single function call feels less obtrusive

So far only solutions 3, 4 and 6 appeal to me. I just left them all here, as someone might find a nice mix and match that is better than all of the above.

[xaviergonz]

As con for number 4, typing generics and declaring the type is a bit weird

e.g.

function wrapClass<T>(t: T): T {
  return t
}

const Todo1 = wrapClass(class InnerTodo1<T> {
  x!: T
})
// notice how we need to use type here so we can declare the usage
type Todo1 = InstanceType<typeof Todo1>
// generic problem: declaring const c: Todo<number> won't work :( 
const c: Todo1 = new Todo1<number>()

// to solve the generic problem we need to separate the class from the wrapping
class InnerTodo2<T> {
  x!: T
}
const Todo2 = wrapClass(InnerTodo2)
// even then we still need to do this to be able to declare types
type Todo2<T> = InnerTodo2<T>
const c2: Todo2<number> = new Todo2<number>()

[danielkcz]

Personally, if I would have to choose, I would go with 3. decorate in constructor. A messy constructor is probably subjective, it feels ok to me. And for the con of forgetting something, it could be possible to have DEV checks. In case there would be a member that's not supposed to be MobX related, it could be added to the decorate map with some noop notation.

Btw, when you say it's strongly typed, what does that exactly mean? The resulting instances? Isn't it somehow possible to type check that you have forgotten to decorate something or you have decorated something that doesn't exist in the class? That would be certainly better than a runtime dev check.

[urugator]

I vote for 3 with a little twist ... it's no longer opt-in, but an opt-out, so in 99% cases one will need just decorate(this). So easy to forget members in large classes is no longer an issue. It's simple, easy to follow, easy to implement, no hacks, no compilation required. De facto it's current observable(object, decorators), but mutative with support for non-own/enumerable and function members (automatically a bound action unless specified otherwise). EDIT: It should probably replace extendObservable?

[danielkcz]

it's no longer opt-in, but an opt-out, so in 99% cases one will need just decorate(this)

Might as well be exported as a class to extend from in case you don't have any other constructor logic.

Although I do wonder how it will work with actions. In useLocalStore we couldn't even use true actions. I can imagine it will be a same problem with classes afaik.

[urugator]

@FredyC You couldn't because you didn't have a way to opt-out. https://github.com/mobxjs/mobx-react-lite/issues/259#issuecomment-588110137 makes it possible (but it's still opt-in behavior, which makes sense in relation to current observable behavior) I am proposing basically this https://github.com/mobxjs/mobx-react-lite/issues/259#issuecomment-588588511, but as noted below, it has to be done in constructor (variant 3 allows that).

[mweststrate]

@xaviergonz good catch on the generics. It is even worse when using the class variant: playground

@FredyC yeah decorate has the cleanest implementation and is pretty straightforward anyway. It has really good papers atm :). With typesafe I mean that the members are properly recognized by TS (something that extendObservable doesn't have). Decorate can be typed statically so that it at least can't decorate non-existing members, which is great in case of renames. How do you feel about alternative 6 in comparison? (6 might need different name for observable to get better typing, but that is probably ok)

@urugator the problem with auto-decorating is that annotating functions automatically can lead to bugs. In the example utility would be marked as action, making it untrackable by the computed that uses it. That is the biggest drawback, bugs caused by this would be pretty hard to detect. The issue @FredyC points to nicely points that out. I'm still quite hesistant about class wide opt-in, as having things accidentally be (deeply) observable (e.g. a React instance) or actions is quite risky, if a class is not purely mobx-y stuff. I think that is the interesting thing of solution 6, it is a little juggling type-wise, but at least decorating is co-located so mistakes are easier to spot.

@FredyC providing base class doesn't work, as the properties don't exist yet when the base constructor run. Only class wrapping (like Observable above) can do this, but might suffer from type limitations as pointed out by @xaviergonz

[spion]

@mweststrate the proxy only applies to the constructor function:

function tc39sucks(Klass: any) {
  return new Proxy(Klass, {
    construct(Klass: any, argsArray: any) {
      let decorators = Reflect.getMetadata('observables', Klass.prototype) as Decorators;
      if (decorators == null) {
        // If there are no decorators defined in this class just call the constructor
        return new Klass(...argsArray);
      }
      else {
        // construct the object, then call decorate on it.
        let target = new Klass(...argsArray);
        decorate(target, decorators);
        return target;
      }
    }
  });
}

Generally quite messy in the debugger.

I don't think thats the case since the proxy only exists for the constructors, the objects themselves are not affected by proxies in any way - but let me know what you have in mind. AFAICT only class constructors will have additional proxy lines in their stack trace, should they throw

deals badly with bounded context, e.g. handler = () => this.x++ probably bypasses any proxy in front of the instance

There are no instance proxies. This implementation just enhances the constructor function calling decorate() on its result. I don't see how it would affect handlers at all.

[spion]

Just for the record, I'd like to state that I'm against removing decorators from libraries just because TC39 cannot reach consensus or makes bad choices like field initializers. Continued use of decorators should send a clear message to TC39 that decorators are here to stay regardless of how implementers feel about them: MobX together with AngularJS and NestJS can certainly play a significant role in this.

TC39 used to operate in "pave the cowpaths" mode, where the ecosystem does something first and then they standardize it. This was much easier when Babel was the dominant compiler, as it was extremely easy to write extensions for it. It's not so easy now that TypeScript is the dominant one, not just because the compiler isn't extensible (it kinda is), but because extensions are a lot more difficult once types get involved. The only option we have right now is to continue using already implemented things that we think are useful.

Sorry for the philosophical rant - I hope I didn't upset anyone! :grinning:

[spion]

@urugator I like your idea a lot for the proxyless implementation that still uses decorators :grinning:

Metadata based decorator:

function obs(target: any, prop: string) {
  let obsMap = Reflect.getMetadata('observables', target) as Decorators | null;
  if (obsMap == null) {
    obsMap = {};
    Reflect.defineMetadata('observables', obsMap, target);
  }
  Object.assign(obsMap, { [prop]: observable });
}

Constructor based applicator:

function tc39sucksInConstructor(obj: any) {
  let proto = Object.getPrototypeOf(obj);
  let decorators = Reflect.getMetadata('observables', proto) as Decorators;
  if (decorators != null) {
    decorate(obj, decorators);
  }
}

Example model:

class MyModel {
  constructor() {
    tc39sucksInConstructor(this);
  }
  toString() {
    return '[[MyModel]]';
  }
  @obs v = 1;
  @computed get x() {
    return this.v + 100;
  }
}

Demo at the same link: https://codesandbox.io/s/cool-wave-h4is4

The demo does have useDefineForClassFields activated, and if you switch @obs to @observable it will not work.

I think it's clean and minimal, easy to migrate to - and its also quite possible that having an "opt-out" version would be even nicer.

[urugator]

@mweststrate I think it could be quite the opposite, because with auto-decoration everything is by default under our control, therefore we can intercept the calls with various checks. Eg. calling action from computed or reaction is detecable and both can throw. It doesn't mean that mutations are strictly forbidden, but since these are special cases you have to be explicit about them, lets say by using sideEffect decorator or something. We could provide more of these semantic decorators/wrappers, reflecting the actual intended usage and preventing all sort of errors.

[mweststrate]

@spion sorry, I glanced over your original post too quickly and entirely misread it. Do you have an example on how the proxy version looks like? I think it suffers from the same typing issue with generics that @xaviergonz pointed out?

[Maaartinus]

@mweststrate

the problem with auto-decorating is that annotating functions automatically can lead to bugs. In the example utility would be marked as action, making it untrackable by the computed that uses it.

IMHO for properties, defaulting to computed is fine (all my properties are computed).

Functions can be either actions or not and erring in either direction is a problem. Maybe the default should be to require all functions to be mentioned explicitly. Additionally, there should a way to specify the behavior for all non-listed functions.

@spion

... I'm against removing decorators from libraries just because TC39 cannot reach consensus or makes bad choices like field initializers.

+100. From a beginner's POV, decorators are the only sane way. Anything else is either too error-prone or too cryptic.

[mweststrate]

@maaartinus how'd you feel about proposal 6 in that case?

[spion]

@mweststrate it looks the same as the non-proxy one, except there is a class decorator

class MyModel {
  constructor() { tc39sucksInConstructor(this); }
  @obs v = 1;
  @computed get x() {
    return this.v + 100;
  }
}

vs

@tc39sucks
class MyModel {
  @obs v = 1;
  @computed get x() {
    return this.v + 100;
  }
}

I'm pretty sure there are other ways to do it too, the main idea is that property decorators only provide metadata on which the class constructor acts - we modify the class constructor either via the decorator + proxy, or manually (by calling tc39sucksInConstructor)

[Maaartinus]

@mweststrate

how'd you feel about proposal 6 in that case?

That depends on how bad the listed "Cons" get in reality. Such problems can really take ages for a beginner and are in no way related to the real work. Documentation can help or confuse even more; for me, an example covering all known problems (sorted from simplest) would be best.

I really hope, we can continue decorators, as anything else feels like a huge step backwards.

I could imagine using a naming convention for differentiating between actions and plain functions. I guess, I'd go for it... naming conventions are good per se and once you get used to using them, they make the coding less error-prone. Sort of Hungarian notation in the dark ages when it made sense.

[mweststrate]

I made a proxyless implementation at the same link, however I bet it's going to have worse DX and edge cases than the proxy version.

@spion I'd be interested if there actually are, looks pretty solid to me. Theoretically the inheritance chain is one deeper, but practically I'm not sure that matters. Even statics seem to be inherited correctly.

I think the approach you are proposing is very neat, as it provides both a way to be backward compatible for easy migration, and a way forward in which we easily can opt-out from decorators if needed.

1. Have people enable in TS enable emitDecoratorMetadata option
2. In Babel, drop the legacy decorators plugin, and have them use https://github.com/leonardfactory/babel-plugin-transform-typescript-metadata (?) instead. They should also disable loose mode for class fields.
3. make sure that decorate(Class, decorators) stores meta data on the class in the same manner as decorators do
4. This means that we can probably clean up the whole mess of interpreting decorators for different standards / transpilers :)
5. Make it mandatory to call initializeObservables(this) or something similar in the constructor of a class. Either by manually adding it to the constructor or by adding a decorator on class level

I think this way we can fix the field initializer problem that will hit us soonish. We also decouple a bit from the actual decorator implementation, and there is a clear way to opt out from decorators by using either decorate(Class, decorators), or supporting initializeObservables(this, decorators) as well (in which case we could actually deprecate the first).

Since initializeObservables would be a new api, we could have it auto-configure as well in the way @urugator describes, if no meta data exists for the class and no decorators are passed in. I'm still not 100% about it, but it is definitely worth an experiment.

Creating a code mode that generates / updates a constructor should be doable as well (although passing the correct args to a super() call is probably hard, but hopefully not a common case)

[danielkcz]

@mweststrate Sounds like a show stopper for CRA based apps that cannot (officially) configure Babel plugins. Feels like extra hurdle people might be reluctant to accept.

[mweststrate]

@FredyC for them it will remain the same; they can configure through decorate as they currently already do (or by using using just initializeObservables(this, decorators), I doubt decorate has any value if a constructor needs to be created anyway)

Edit: I think babel macros are now supported, so that might be an escape to using decorators? not sure if macros can handle syntax extensions

[xaviergonz]

Have people enable in TS enable emitDecoratorMetadata option

Do you really need decorator metadata to make it work? I'd guess the only thing needed to make it work is the class prototype and the field/method name, which you both get even without decorator metadata.

The decorator could then do something like

prototype[someSymbol].push({ // someSymbol is a hidden prop
  propertyName,
  decoratorInfo
})

While the class decorator would call the original constructor + then constructor init function and iterate over the prototype symbol info to apply the desired behavior over the instance

[vkrol]

Have people enable in TS enable emitDecoratorMetadata option

Does this mean that the reflect-metadata polyfill will be required?