[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: microsoft/TypeScript#34787

Possibly related to #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. ***@***.***> 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 <#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 <#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 ***@***.***> 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 <#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;
  }