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

Created on 15 Feb 2020  ·  65Comments  ·  Source: mobxjs/mobx

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)

🐛 bug 🔨 breaking-change

Most helpful comment

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:

All 65 comments

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?

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

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
.

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 😭

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.

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

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?

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

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

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

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

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
.

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?

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

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

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)

@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

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

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.

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.

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.

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

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
.

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

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

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

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

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

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

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.

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.

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

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.

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?

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.

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

@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

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

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:

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

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

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

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

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

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

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

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)

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

@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

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

Have people enable in TS enable emitDecoratorMetadata option

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

@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

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

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.

@mweststrate Am I correct that your sixth example will not work because primitives should be wrapped in observable.box? 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);

  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?

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
.

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

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

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

Have to go atm, so quick reply without looking into individual cases, but
we'd always box, which would be basically no different from now where
@observable x = {}, just like x = observable({}) would both create an
observable object, and an observable property that holds that object

On Wed, Mar 4, 2020 at 6:25 PM urugator notifications@github.com wrote:

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? Like this?

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

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 objecto.newProp1 = "a";extendObservable(object, decorators)
// "Extending" classclass {
constructor() {
extendObservable(this, decorators)
}
}// In all cases the behavior is same:// fn => action// get => computed// other => observable

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


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=AAN4NBBMRZND4QHYGGABKOTRF2MJ5A5CNFSM4KV4PIV2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOENZNNEQ#issuecomment-594728594,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AAN4NBD4FGKHHHKE67L27ZLRF2MJ5ANCNFSM4KV4PIVQ
.

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

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

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

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,{...})

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.

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

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

Was this page helpful?
0 / 5 - 0 ratings

Related issues

kirhim picture kirhim  ·  3Comments

geohuz picture geohuz  ·  3Comments

thepian picture thepian  ·  3Comments

mehdi-cit picture mehdi-cit  ·  3Comments

cafreeman picture cafreeman  ·  4Comments