Migrate from extendObservable to decorate?

[nykula]

Following the example from readme:

// Todo.js

import { decorate, observable } from "mobx";

class Todo {
  constructor() {
    this.finished = false;
    this.id = Math.random();
    this.title = "";
  }
}

decorate(Todo, {
  finished: observable,
  title: observable
});

However, the code is highlighted as an error:

[js]
Argument of type '{ title: IObservableFactory & IObservableFactories & { enhancer: IEnhancer; }; finished: IOb...' is not assignable to parameter of type '{ prototype: MethodDecorator | PropertyDecorator; }'.
Object literal may only specify known properties, and 'title' does not exist in type '{ prototype: MethodDecorator | PropertyDecorator; }'.

What am I doing wrong?

---

Have to move from extendObservable, because it "can only be used to introduce new properties" now, so this crashes in development:

// TodoOld.js

import { extendObservable } from "mobx";

class TodoOld {
  constructor() {
    this.finished = false;
    this.id = Math.random();
    this.title = "";

    extendObservable(TodoOld, {
      finished: this.finished,
      title: this.title
    });
  }
}

---

2. Issue:

• Provide error messages including stacktrace
  • No stacktrace, type check issue.
• Provide a minimal sample reproduction. Create a reproduction based on this sandbox
  • No sandbox, type check issue.
• Did you check this issue wasn't filed before?
• Elaborate on your issue. What behavior did you expect?
• State the versions of MobX and relevant libraries. Which browser / node / ... version?
  • mobx 4.1.0
  • typescript 2.7.2 (allowJs, checkJs, noEmit, strict)
  • VS Code 1.21.1

tsconfig.json:

{
  "compilerOptions": {
    "target": "esnext",
    "module": "commonjs",
    "allowJs": true,
    "checkJs": true,
    "jsx": "preserve",
    "noEmit": true,
    "strict": true,
    "noUnusedLocals": true,
    "noUnusedParameters": true,
    "noImplicitReturns": true,
    "noFallthroughCasesInSwitch": true,
    "esModuleInterop": true
  },
  "exclude": [
    "node_modules",
    "src/legacy",
    "src/owl"
  ]
}

[mweststrate]

The problem is that you are running decorate with typescript strictness, but not building your class through the typescript standards. decorate should decorate existing fields, but TS cannot infer from your class title is a field of of Todo. Instead, this should work:

import { decorate, observable } from "mobx";

class Todo {
  finished = false;
  id = Math.random();
  title = ""
}

decorate(Todo, {
  finished: observable,
  title: observable
});

If you don't have field initializers enabled in your config, use:

class Todo {
  finished; // declare these fields so TS knows about them
  id;
  title;

  constructor() {
    this.finished = false;
    this.id = Math.random();
    this.title = "";
  }
}

decorate(Todo, {
  finished: observable,
  title: observable
});

[nykula]

App is JavaScript, can't declare anything on class. Works on desktop as a set of scripts, no build step at all. TS acts as a linter (allowJs, checkJs), and does know these fields are present on every instance, since type checking in strict mode and auto complete work fine throughout the app, only MobX upgrade broke.

[mweststrate]

but your error is based on the typescript types, if the inferred type of Todo is not what you expect it would to be, there is not much we can do I think, probably there is a comment to make decorate ignore the typings or something.

[nykula]

Inferred type of Todo is exactly what I expect it to be :) Let's look into MobX typings.

decorate.d.ts exports an overloaded decorate<T>, one for class and one for prototype object. The error above is related to the prototype one (while I have a class), because as usual in TypeScript, if one definition fails, it keeps trying with others. I commented out the prototype overload, to see why it doesn't like the class one. A different error appears, exposing one part of the issue:

[js]
Argument of type '{ finished: IObservableFactory & IObservableFactories & { enhancer: IEnhancer; }; title: IOb...' is not assignable to parameter of type '{ finished: MethodDecorator | PropertyDecorator; id: MethodDecorator | PropertyDecorator; title: ...'.
Property 'id' is missing in type '{ finished: IObservableFactory & IObservableFactories & { enhancer: IEnhancer; }; title: IOb...'.

Looks like it wants a decorator for every property, while the Todo needs only finished and title observables. Added question mark to decorate.d.ts:

export declare function decorate<T>(clazz: new (...args: any[]) => T, decorators: {
    [P in keyof T]?: MethodDecorator | PropertyDecorator;
}): void;

Yet another error, finally with enough detail:

[js]
Argument of type '{ finished: IObservableFactory & IObservableFactories & { enhancer: IEnhancer; }; title: IOb...' is not assignable to parameter of type '{ finished?: MethodDecorator | PropertyDecorator | undefined; id?: MethodDecorator | PropertyDeco...'.
Types of property 'finished' are incompatible.
Type 'IObservableFactory & IObservableFactories & { enhancer: IEnhancer; }' is not assignable to type 'MethodDecorator | PropertyDecorator | undefined'.
Type 'IObservableFactory & IObservableFactories & { enhancer: IEnhancer; }' is not assignable to type 'PropertyDecorator'.
Types of parameters 'value' and 'target' are incompatible.
Type 'Object' is not assignable to type 'string | number | boolean | null | undefined'.
The 'Object' type is assignable to very few other types. Did you mean to use the 'any' type instead?
Type 'Object' is not assignable to type 'false'.

Not sure how to alter typings from here. Where in MobX should I look?

---

Update, the conflict seems to happen between observable.d.ts from MobX and lib.es5.d.ts from TypeScript itself:

// observable.d.ts

export interface IObservableFactory {
    (value: number | string | null | undefined | boolean): never;

    // --> key: string <--
    (target: Object, key: string, baseDescriptor?: PropertyDescriptor): any;

    <T = any>(value: T[], options?: CreateObservableOptions): IObservableArray<T>;
    <K = any, V = any>(value: Map<K, V>, options?: CreateObservableOptions): ObservableMap<K, V>;
    <T extends Object>(value: T, decorators?: {
        [K in keyof T]?: Function;
    }, options?: CreateObservableOptions): T & IObservableObject;
}

// lib.es.5.d.ts

// --> propertyKey: string | symbol <--
declare type PropertyDecorator = (target: Object, propertyKey: string | symbol) => void;

[mweststrate]

@makepost the types are defined here, and your Todo should take the first overload:

mobx/src/api/decorate.ts

Lines 3 to 10 in 057df95

	export function decorate<T>(
	clazz: new (...args: any[]) => T,
	decorators: { [P in keyof T]: MethodDecorator | PropertyDecorator }
	): void
	export function decorate<T>(
	object: T,
	decorators: { [P in keyof T]: MethodDecorator | PropertyDecorator }
	): T

export function decorate<T>(
    clazz: new (...args: any[]) => T,
    decorators: { [P in keyof T]: MethodDecorator | PropertyDecorator }
): void
export function decorate<T>(
    object: T,
    decorators: { [P in keyof T]: MethodDecorator | PropertyDecorator }
): T

[mweststrate]

Can you configure your project to use es6 typings instead? That is what MobX 4 expects (it has symbols, maps, promises and such)

[nykula]

Should take the first overload, but doesn't because of the conflict I explained.

---

The project has "target": "esnext", app makes heavy use of ES6 such as proxies. Error goes away if, in observable.d.ts, I change IObservableFactory this way:

// Before:
    (target: Object, key: string, baseDescriptor?: PropertyDescriptor): any;

// After:
    (target: Object, key: string | symbol, baseDescriptor?: PropertyDescriptor): any;

[mweststrate]

that change makes sense in general, although not sure why it makes a difference here. Feel free to submit a PR

[nykula]

As I'm adding a case and enable type checking for test/base/decorate.js, lots of errors appear. For example, is the following inteded?

test("decorate should work", function () {
    class Box {
        sizes = [2]
        // ...
        addSize() {
            this.sizes.push([3])
        }
    }
})

I see a lot of this; the question is whether to add // @ts-ignore or set type to (number | number[])[] for sizes and other such arrays in this file.