annotations + subclassing rework (#2641)

* wip * fix unused vars * fix comment * revert enumerability changes * wip * wip * wip * wip * wip * wip * wip * wip * wip * wip * wip * wip * wip * wip * wip * wip * comments * processing comments * processing comments * descriptor cache * ... * don't annotate proto on action.bound * optimize makeAutoObservable, fix symbolic action name, fix inferred annotation caching * cleanup, defineProperty tests * fix comment * fix comment * extendObservable includes non-enumerable, optimize makeAutoObservable * little tweak * fix imports, fix plain object detection * docs * docs * changeset
mobxjs · Jan 27, 2021 · 28f8a11 · 28f8a11
1 parent 945d029
commit 28f8a11
Show file tree

Hide file tree

Showing 47 changed files with 2,765 additions and 1,434 deletions.
diff --git a/.changeset/short-yaks-serve.md b/.changeset/short-yaks-serve.md
@@ -0,0 +1,22 @@
+---
+"mobx": minor
+"mobx-react-lite": minor
+---
+
+`action`, `computed`, `flow` defined on prototype can be overriden by subclass via `override` annotation/decorator. Previously broken.
+Overriding anything defined on instance itself (`this`) is not supported and should throw. Previously partially possible or broken.
+Attempt to re-annotate property always throws. Previously mostly undefined outcome.
+All annotated and non-observable props (action/flow) are non-writable. Previously writable.
+All annotated props of non-plain object are non-configurable. Previously configurable.
+Observable object should now work more reliably in various (edge) cases.
+Proxied objects now support `Object.defineProperty`. Previously unsupported/broken.
+`extendObservable/makeObservable/defineProperty` notifies observers/listeners/interceptors about added props. Peviously inconsistent.
+`keys/values/entries` works like `Object.keys/values/entries`. Previously included only observables.
+`has` works like `in`. Previously reported `true` only for existing own observable props.
+`set` no longer transforms existing non-observable prop to observable prop, but simply sets the value.
+`remove/delete` works with non-observable and computed props. Previously unsupported/broken.
+Passing `options` to `observable/extendObservable/makeObservable` throws if the object is already observable . Previously passed options were mostly ignored.
+`autoBind` option is now sticky - same as `deep` and `name` option.
+`observable/extendObservable` now also picks non-enumerable keys (same as `make[Auto]Observable`).
+Removed deprecated `action.bound("name")`
+Proxied objects should be compatible with `Reflect` API. Previously throwing instead of returning booleans.
diff --git a/.eslintrc.js b/.eslintrc.js
@@ -2,6 +2,7 @@ module.exports = {
     parser: "@typescript-eslint/parser",
     plugins: ["@typescript-eslint"],
     extends: "eslint:recommended",
+    ignorePatterns: ["**/__tests__/**/*"],
     env: {
         browser: true,
         es6: true,

diff --git a/docs/observable-state.md b/docs/observable-state.md
@@ -59,6 +59,91 @@ class Doubler {
 }
 ```
 
+<!--subclass + makeObservable-->
+
+```javascript
+import { makeObservable, observable, computed, action } from "mobx"
+
+class Parent {
+    // not overridable
+    observable1 = 0
+
+    constructor(value) {
+        makeObservable(this, {
+            observable1: observable,
+            computed1: computed,
+            action1: action,
+            arrowAction1: action
+        })
+    }
+
+    // overridable
+    get computed1() {
+        return this.observable * 2
+    }
+
+    // overridable
+    action1() {
+        this.observable++
+    }
+
+    // not overridable
+    arrowAction1 = () => {}
+
+    // workaround - not annotated - overridable
+    overridableArrowAction1 = action(() => {})
+}
+
+class Child extends Parent {
+    // new
+    observable2 = 0
+
+    constructor(value) {
+        makeObservable(this, {
+            // overriden fields
+            action1: override,
+            computed1: override,
+            // new fields
+            observable2: observable,
+            computed2: computed,
+            action2: action,
+            arrowAction2: action
+        })
+    }
+
+    // overrides
+    get computed1() {
+        return super.computed1 * 2
+    }
+
+    // overrides
+    action1() {
+        super.action1()
+    }
+
+    // workaround - not annotated - overrides
+    overridableArrowAction1 = action(() => {})
+
+    // new
+    get computed2() {
+        return super.computed1 * 2
+    }
+
+    // new
+    action2() {
+        super.action1()
+    }
+
+    // new
+    arrowAction2 = () => {}
+}
+```
+
+All annotated fields are non-configurable.
+All non-observable (stateless) fields (`action`, `flow`) are non-writable.
+Only `action`, `computed`, `flow`, `action.bound` defined on prototype can be overriden by subclass.
+Field can't be re-annotated in subclass, except with `override`.
+
 <!--factory function + makeAutoObservable-->
 
 ```javascript
@@ -213,22 +298,28 @@ Note that it is possible to pass `{ proxy: false }` as an option to `observable`
 | `observable.ref`                   | Like `observable`, but only reassignments will be tracked. The assigned values themselves won't be made observable automatically. For example, use this if you intend to store immutable data in an observable field.      |
 | `observable.shallow`               | Like `observable.ref` but for collections. Any collection assigned will be made observable, but the contents of the collection itself won't become observable.                                                             |
 | `observable.struct`                | Like `observable`, except that any assigned value that is structurally equal to the current value will be ignored.                                                                                                         |
-| `action`                           | Mark a method as an action that will modify the state. Check out [actions](actions.md) for more details.                                                                                                                   |
-| `action.bound`                     | Like action, but will also bind the action to the instance so that `this` will always be set.                                                                                                                              |
+| `action`                           | Mark a method as an action that will modify the state. Check out [actions](actions.md) for more details. Non-writable.                                                                                                     |
+| `action.bound`                     | Like action, but will also bind the action to the instance so that `this` will always be set. Non-writable.                                                                                                                |
 | `computed`                         | Can be used on a [getter](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/get) to declare it as a derived value that can be cached. Check out [computeds](computeds.md) for more details.      |
 | `computed.struct`                  | Like `computed`, except that if after recomputing the result is structurally equal to the previous result, no observers will be notified.                                                                                  |
 | `true`                             | Infer the best annotation. Check out [makeAutoObservable](#makeautoobservable) for more details.                                                                                                                           |
 | `false`                            | Explicitly do not annotate this property.                                                                                                                                                                                  |
-| `flow`                             | Creates a `flow` to manage asynchronous processes. Check out [flow](actions.md#using-flow-instead-of-async--await-) for more details. Note that the inferred return type in TypeScript might be off.                       |
+| `flow`                             | Creates a `flow` to manage asynchronous processes. Check out [flow](actions.md#using-flow-instead-of-async--await-) for more details. Note that the inferred return type in TypeScript might be off. Non-writable.         |
+| `override`                         | Applicable to inherited `action`, `flow`, `computed`, `action.bound` overriden by subclass.                                                                                                                                |
 | `autoAction`                       | Should not be used explicitly, but is used under the hood by `makeAutoObservable` to mark methods that can act as action or derivation, based on their calling context.                                                    |
 
 ## Limitations
 
 1. `make(Auto)Observable` only supports properties that are already defined. Make sure your compiler configuration is [correct](installation.md#use-spec-compliant-transpilation-for-class-properties), or as work-around, that a value is assigned to all properties before using `make(Auto)Observable`. Without correct configuration, fields that are declared but not initialized (like in `class X { y; }`) will not be picked up correctly.
 1. `makeObservable` can only annotate properties declared by its own class definition. If a sub- or superclass introduces observable fields, it will have to call `makeObservable` for those properties itself.
+1. Every field can be annotated only once (except for `override`). The field annotation or configuration can't change in subclass.
+1. All annotated fields of **non-plain** objects (classes) are **non-configurable**.
+1. All non-observable (stateless) fields (`action`, `flow`) are **non-writable**.
+1. Only `action`, `computed`, `flow`, `action.bound` defined on _prototype_ can be overriden by subclass.
 1. By default TypeScript will not allow you to annotate private fields. This can be overcome by explicitly passing the relevant private fields as generic argument, like this: `makeObservable<MyStore, "myPrivateField" | "myPrivateField2">(this, { myPrivateField: observable, myPrivateField2: observable })`.
 1. Calling `make(Auto)Observable` and providing annotations must be done unconditionally, as this makes it possible to cache the inference results.
 1. JavaScript private fields are not supported (the `#field` syntax). When using TypeScript, it is recommended to use the `private` modifier instead.
+1. Mixing annotations and decorators within single inheritance chain is not supported - eg you can't use decorators for parent class and annotations for subclass.
 
 ## Options {🚀}
 

diff --git a/packages/mobx-react-lite/__tests__/__snapshots__/printDebugValue.test.ts.snap b/packages/mobx-react-lite/__tests__/__snapshots__/printDebugValue.test.ts.snap
@@ -6,9 +6,6 @@ Object {
     Object {
       "name": "[email protected]",
     },
-    Object {
-      "name": "[email protected]?",
-    },
     Object {
       "dependencies": Array [
         Object {

diff --git a/packages/mobx-react-lite/src/useAsObservableSource.ts b/packages/mobx-react-lite/src/useAsObservableSource.ts
@@ -2,7 +2,7 @@ import { useDeprecated } from "./utils/utils"
 import { observable, runInAction } from "mobx"
 import { useState } from "react"
 
-export function useAsObservableSource<TSource>(current: TSource): TSource {
+export function useAsObservableSource<TSource extends object>(current: TSource): TSource {
     if ("production" !== process.env.NODE_ENV)
         useDeprecated(
             "[mobx-react-lite] 'useAsObservableSource' is deprecated, please store the values directly in an observable, for example by using 'useLocalObservable', and sync future updates using 'useEffect' when needed. See the README for examples."

diff --git a/packages/mobx/__tests__/perf/perf.js b/packages/mobx/__tests__/perf/perf.js
@@ -482,7 +482,7 @@ results of this test:
 
         ar()
 
-        log("expensive sort: disposed" + (now() - start))
+        log("expensive sort: disposed " + (now() - start))
 
         const plain = mobx.toJS(items, false)
         t.equal(plain.length, MAX)

diff --git a/packages/mobx/__tests__/v4/base/__snapshots__/object-api.js.snap b/packages/mobx/__tests__/v4/base/__snapshots__/object-api.js.snap
@@ -34,7 +34,7 @@ exports[`observe & intercept 2`] = `
 Array [
   Object {
     "observe": Object {
-      "debugObjectName": "ObservableObject@34",
+      "debugObjectName": "TestObject",
       "name": "b",
       "newValue": Object {
         "title": "get tea",
@@ -50,7 +50,7 @@ Array [
   },
   Object {
     "observe": Object {
-      "debugObjectName": "ObservableObject@34",
+      "debugObjectName": "TestObject",
       "name": "a",
       "object": Object {
         "b": Object {

diff --git a/packages/mobx/__tests__/v4/base/makereactive.js b/packages/mobx/__tests__/v4/base/makereactive.js
@@ -449,42 +449,6 @@ test("as structure view", function () {
     expect(cc).toBe(2)
 })
 
-test("ES5 non reactive props", function () {
-    expect(function () {
-        m.extendObservable(false, { notConfigurable: 1 })
-    }).toThrow(/'extendObservable' expects an object as first argument/)
-    let te = m.observable({})
-    Object.defineProperty(te, "nonConfigurable", {
-        enumerable: true,
-        configurable: false,
-        writable: true,
-        value: "static"
-    })
-    // should skip non-configurable / writable props when using `observable`
-    expect(() => {
-        te = m.set(te, te)
-    }).toThrow(/Cannot make property 'nonConfigurable' observable/)
-    const d1 = Object.getOwnPropertyDescriptor(te, "nonConfigurable")
-    expect(d1.value).toBe("static")
-
-    const te2 = m.observable({})
-    Object.defineProperty(te2, "notWritable", {
-        enumerable: true,
-        configurable: true,
-        writable: false,
-        value: "static"
-    })
-    // should throw if trying to reconfigure an existing non-writable prop
-    expect(function () {
-        m.set(te2, { notWritable: 1 })
-    }).toThrow(/Cannot make property 'notWritable' observable/)
-    const d2 = Object.getOwnPropertyDescriptor(te2, "notWritable")
-    expect(d2.value).toBe("static")
-
-    // should not throw for other props
-    expect(m.extendObservable(te, { bla: 3 }).bla).toBe(3)
-})
-
 test("540 - extendobservable should not report cycles", function () {
     expect(() => m.extendObservable(Object.freeze({}), {})).toThrowError(
         /Cannot make the designated object observable/