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

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! 😀

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?

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 .

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;
}

@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)

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.

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).

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}`);
}

flesh out a bit how that api would look like?

observable (fn + decorator) Like now, applied by default to fields.

computed (fn + decorator) Like now, applied by default to getters.

action (fn + decorator) Applied by default to functions. Cannot be called from derivations (computed/reaction(first arg)/autorun) (unless inside effect). Throws: Calling action from computed/reaction/observer is forbidden. If this is an utility function that doesn’t mutate state please use ingore. Mutating state during derivation can lead to inifinite cycles, can break batching and can lead to unexpected behavior. Consider refactoring to computed or use effect, see docs, blah blah.

effect (fn + decorator) Like action, but can be called from derivations, allows mutating observables inside computed/reaction/autorun.

ignore (decorator) For utility methods and fields that shouldn’t be observable for whatever reason. Also makes sure that if you (repeatedly) call extendObservable on already observable object it won’t attempt to make the field observable.

Additionally: Mutating state from anywhere outside of action/effect is not allowed. Reading observables from anywhere outside of reaction/action/effect is not allowed.

Therefore: We don’t need specific view decorator (ignore by itself doesn’t allow mutations). We can ditch enforceAction/requireReaction/requireObservable/etc, these are always on and cover your back. We can ditch _allowSomething as they can be replaced by effect (maybe not always not sure…).

care to share the babel transform?

Sure, but keep in mind it’s the first thing I tried with babel and it’s very unfinished. Personally I don’t need it for anything, just wanted to try something new: https://gist.github.com/urugator/ffc92893fafd70d6e52019f38594ece9

So I am still limping on two different lines of thoughts here,

1. go for decorators, but simplify (see comment) and make a constructor (or class decorator mandatory)
2. drop decorators entirely and be done with that now and forever (a.k.a. until standardized at least) and have wrappers used everywhere. The ‘unboxing’ in the constructor could than even be optional; without unboxing everything still works but .get() has to be called explicitly like in @kubk example. This is nice for purist caring about side-effectfull property reads / writes, but the main benefit is that it reduces the many ways of achieving the same thing in MobX, making learning easier, where ‘unboxing’ is just an incremental convenience step, rather than a totally different way of writing things, like extendObservable vs. decorators vs. decorate.

Edit: Some further realizations: option 2 doesn’t require any transpilation at all in modern JS. option 1 shares methods by default on the prototype (pro!) but 2 binds them by default (a different pro) (non-bound shared methods is still possible, by doing e.g. Class.prototype.method = action(Class.prototype.method) after class definition)

Edit: hypothesis: initializers that refer each other need to make sure to read ‘boxed’. (but at least that would result in tracked reads in contrast to when using decorators, which might be even more confusing. Anyway, lets make sure we cover that in tests in any case)

Edit: created a poll because the collective twitter wisdom can always be trusted: https://twitter.com/mweststrate/status/1235227165072822272

the idea is that initializeobservables(this) could automatically unbox the observable boxes (observable objects already use an observable box per property behind the scenes). So techincally it would be pretty straight forward to have it behave the same: width = observable(20) creates an observable.box, then initializeObservables generates a getter and setter for width that redirect to the box’s getter and setter.

On Wed, Mar 4, 2020 at 12:21 PM Egor Gorbachev notifications@github.com wrote:

@mweststrate https://github.com/mweststrate Am I correct that your sixth example will not work because primitives should be wrapped in observable.box? #2288 (comment) https://github.com/mobxjs/mobx/issues/2288#issuecomment-593008340 This is how your example will look like with observable.box:

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

constructor() { initializeObservables(this) }

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

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

utility() { return 1; } }

Or there are plans to get rid of observable.box?

— 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=AAN4NBGWOADMCRQZFAVAG33RFZBUVA5CNFSM4KV4PIV2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOENXS4QY#issuecomment-594488899, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAN4NBC7ANMFVJ7KOLP4JQ3RFZBUVANCNFSM4KV4PIVQ .

@xaviergonz yeah, it is more babel I am worried about 😃 iirc babel-decorators-legacy is not compatible with modern field initializers, but above mentioned plugin (hopefully) is. So my goal is to have a simple decorator implementation that is compatible with babel, TS and define semantics. But which combination actually works has to be investigated still. Just want to figure out the general direction first

@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

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

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.

@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.

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.

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;
  }

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 😭

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