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

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

Closed
Kukkimonsuta opened this issue Feb 15, 2020 · 65 comments
Closed

Comments

@Kukkimonsuta
Copy link

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

danielkcz commented Feb 15, 2020

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

danielkcz commented Feb 15, 2020

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

mweststrate commented Feb 15, 2020 via email

@Kukkimonsuta
Copy link
Author

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

danielkcz commented Feb 16, 2020

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

Kukkimonsuta commented Feb 16, 2020

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

danielkcz commented Feb 16, 2020

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

Kukkimonsuta commented Feb 16, 2020

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

mweststrate commented Feb 16, 2020 via email

@xaviergonz
Copy link
Contributor

xaviergonz commented Feb 16, 2020

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

xaviergonz commented Feb 16, 2020

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

mweststrate commented Feb 16, 2020 via email

@urugator
Copy link
Collaborator

urugator commented Feb 16, 2020

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

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

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

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

Fully working (legacy decorators, loose fields):

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

xaviergonz commented Feb 17, 2020

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

mweststrate commented Feb 18, 2020

@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

@mweststrate mweststrate changed the title TypeScript useDefineForClassFields breaks decorators [breaking change] Get rid of field initializers (and decorators) Feb 19, 2020
@Kukkimonsuta
Copy link
Author

Kukkimonsuta commented Feb 19, 2020

Related TypeScript issue here: microsoft/TypeScript#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
Copy link
Contributor

xaviergonz commented Feb 19, 2020

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

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

danielkcz commented Feb 19, 2020

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

urugator commented Feb 19, 2020

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

mweststrate commented Feb 19, 2020 via email

@danielkcz
Copy link
Contributor

danielkcz commented Feb 19, 2020

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

@mweststrate
Copy link
Member

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

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

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

@spion
Copy link

spion commented Mar 2, 2020

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

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

mweststrate commented Mar 3, 2020

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

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

mweststrate commented Mar 3, 2020

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

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

vkrol commented Mar 3, 2020

Have people enable in TS enable emitDecoratorMetadata option

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

@mweststrate
Copy link
Member

@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

@mweststrate mweststrate changed the title [breaking change] Get rid of field initializers (and decorators) [breaking change] Get rid of field initializers (and legacy decorators) Mar 3, 2020
@spion
Copy link

spion commented Mar 4, 2020

I only used Reflect.defineMetadata out of habit! You could use any old Map / WeakMap to remember that metadata if we don't want to depend on that compiler mode or the reflect-metadata module 😀

Either way, I don't think the emitDecoratorMetadata mode is required. Using the reflect-metadata module is less invasive, but also optional - although it does take care of poly filling Map/Set as appropriate.

@kubk
Copy link
Collaborator

kubk commented Mar 4, 2020

@mweststrate Am I correct that your sixth example will not work because primitives should be wrapped in observable.box? #2288 (comment)
This is how your example will look like with observable.box:

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

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

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

  utility() {
    return 1;
  }
}

This example is much more verbose. Or there are plans to get rid of observable.box?

@mweststrate
Copy link
Member

mweststrate commented Mar 4, 2020 via email

@mweststrate
Copy link
Member

mweststrate commented Mar 4, 2020

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

@wickedev
Copy link

wickedev commented Mar 4, 2020

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;
  }
class Counter {
  @observable declare count: number;

  constructor({ count }: Counter) {
    this.count = count;
  }
}

The declare keyword is not available in codesandbox, but it is available in the actual typescript compiler. Of course, if you use the declare keyword, it becomes more verbose and can't use field initializers, but giving the initial value in the constructor can keep the code clean even when initializing the store for testing or SSR. What do you think?

https://github.com/wickedev/mobx-poc

@urugator
Copy link
Collaborator

urugator commented Mar 4, 2020

reduces the many ways of achieving the same thing in MobX

I dunno... do we create observable objects like this:

const o = {
  width: observable.box(20),
  height: observable.box(10),
  surface: computed(() => {
    this.height * this.width * this.utility();
  }),
  double = action(() => {
    this.width = this.width * 2;
  }),
};
initializeObservables(o);

How do I create an actual box?

class {
  width = box(box(20));
  computed = computed(computed(() => {}));
  constructor() {
     initializeObservables(this);
  }
}

How do I create a ref to box?

class {
  width = ignoreMe(box(20));
  constructor() {
     initializeObservables(this);
  }
}

What about other "boxes": array/map/set (or other classes?)

class {
  map: observable.map(); // is this.map observable?
  map: observable.box(observable.map()); // or do i need "extra" box
  constructor() {
     initializeObservables(this);
  }
}

If we want to simplify things I still vote for:

// Creating object:
const o = observable(object, decorators)

// Extending object
o.newProp1 = "a";
extendObservable(object, decorators)

// Extending instance
class {
  constructor() {
    extendObservable(this, decorators)
  }	
}
// In all cases the default behavior is same:
// fn => action (bound)
// get => computed
// other => observable

extendObservable can additionally look for decorators on prototype if one prefers an actual @decorator or decorate

@mweststrate
Copy link
Member

mweststrate commented Mar 4, 2020 via email

@elektronik2k5
Copy link
Contributor

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

What about using Babel macros? I have no idea whether the necessary transformations are possible in macros, but it is worth checking.

@mweststrate
Copy link
Member

@urugator care to share the babel transform?

I dunno... do we create observable objects like this:

No, we can keep that as is / support both. The main goal is to be able to convert classes while keeping 'decorators' co-located

How do I create an actual box?
How do I create a ref to box?

I don't think there any actual use case for that, but if there is, an assignment in the constructor or double wrapping would work in deed

map: observable.map(); // is this.map observable?

yes, like with @observable, you will get automatically a box for the property that holds the ref to the observable collection

@wickedev
as far as I can see that still won't work without another abstraction that interprets the decorators; with define semantics the local fields still shadow the decorators by default playground

@urugator

Eg. calling action from computed or reaction is detectable and both can throw.

It is an interesting thought, but I feel like we have been kept piling rules and ways to make exceptions to rules on top of each other (e.g. calling actions from reaction is quite valid, and lazy loading patterns where a computed might trick a side effect action if called for the first time, like in mobx-utils, or making props observable or stores mobx-react(-lite) observable has resulted in a lot of _unblessed api's, which are ok for building libs, but which we don't want to encourage to be used, as it is too easy too overuse them without grokking the mental model of mobx fully.

In hindsight I'd love to rely on naming convention, e.g. only actX... for auto conersion, but that would create impossible migration paths at this point :) I don't think this proposal in its current shape removes the need for those _unblessed api's at this moment, but at least hopefully prevents in making the hole deeper.

I think in the end if we don't have enough info to always make the correct choice, opt-in is better than opt-out. That being said, feel free to flesh out a bit how that api would look like?
In short, I'd be interested how we can design a class that has members that should not be observables, and methods that should not be actions. I think in any case it would be nice to add an initializeObservables(this, 'auto') for classes that don't need exceptions.

Also, better name for initializeObservables is welcome. Was thinking about unbox, or mobxInit, but that might be too abstract :)

@urugator
Copy link
Collaborator

urugator commented Mar 6, 2020

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

@JohnNilsson
Copy link

JohnNilsson commented Mar 6, 2020

If the decorator argument was a function (or visitor) it would allow default policies to be decided per project. Could even implement things like naming convention based policies. And would allow such things to be factored out into independent npm packages.
Edit: To clarify

import customPolicy from './mobx-policy';
const customInitializeObservables = (self, options) => initializeObservables(self, customPolicy, options);
//...
  customInitializeObservables(this,{...})

@Maaartinus
Copy link

I think in any case it would be nice to add an initializeObservables(this, 'auto') for classes that don't need exceptions.

And maybe something like initializeObservables(this, 'auto', {someField: 'ignore', someAction: 'action'}) when there are exceptions.

Using initializeObservables(this, 'auto', {ignore: ['someField', 'someField1'], action: ['someAction', 'someAction2']}) might be better (more compact, but possibly more error-prone).

I'm unsure what's more common, actions or views. Maybe there should be auto-action and auto-view instead.

For naming conventions, initializeObservables(this, 'naming') could be used; possibly with added exceptions as above.

Also, better name for initializeObservables is welcome. Was thinking about unbox, or mobxInit, but that might be too abstract :)

As it's about the only user-facing thing mobx does, mobxInit is IMHO not bad. Maybe mobxInitObject as it deals with a whole object containing observables and other stuff. I don't understand unbox.

@mweststrate
Copy link
Member

mweststrate commented Apr 2, 2020

Closing this one now in favor of #2325, to avoid that the discussion happens in two places.

@sonhanguyen
Copy link

sonhanguyen commented Jul 9, 2020

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

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

@xaviergonz this seems to work (no inference but not terribly annoying)

https://www.typescriptlang.org/play/?ssl=7&ssc=24&pln=7&pc=30#code/MYewdgzgLgBAJgU1AJwIZQQQgFwwDwAqAfABQD6uqYAngJQwC8RM5uB9TMBAUN8ADaoIEGAGUoIZAkLMA3txgwAbqn4BXBIxgAGbgF9eoSLGiTNDeEknppUagAcEIAGZiJU0vMUr1CXAHIQACMIBGQVIP4EfwAafVoScTNaQ3BoGFQtMAQAdxhTKTwwNQBbILDSWiA

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests