fix getEnv, some refactoring (mobxjs#1233)

.vscode/launch.json

@@ -8,6 +8,7 @@
             "name": "debug unit test",
             "program": "${workspaceRoot}/node_modules/jest/bin/jest.js",
             "args": ["-i", "${fileBasename}"],
+            "skipFiles": ["<node_internals>/**/*", "${workspaceRoot}/node_modules/**/*"]
         }
     ]
-}
+}

changelog.md

@@ -1,3 +1,7 @@
+# 3.12.1
+
+-   Fixed a regression with `getEnv` sometimes not returning the proper environment.
+
 # 3.12.0
 
 -   Added `TypeOfValue<typeof variable>` to extract the type of a complex (non primitive) variable in Typescript.

packages/mobx-state-tree/__tests__/core/custom-type.test.ts

@@ -14,12 +14,12 @@ class Decimal {
 
     constructor(value: string) {
         const parts = value.split(".")
-        this.number = parseInt(parts[0])
-        this.fraction = parseInt(parts[1])
+        this.number = Number(parts[0])
+        this.fraction = Number(parts[1])
     }
 
     toNumber() {
-        return this.number + parseInt("0." + this.fraction)
+        return this.number + Number("0." + this.fraction)
     }
 
     toString() {

packages/mobx-state-tree/__tests__/core/env.test.ts

@@ -1,4 +1,24 @@
-import { types, getEnv, clone, detach, unprotect } from "../../src"
+import {
+    types,
+    getEnv,
+    clone,
+    detach,
+    unprotect,
+    walk,
+    getPath,
+    castToSnapshot,
+    hasParent,
+    Instance,
+    destroy,
+    IStateTreeNode,
+    getParent,
+    IAnyStateTreeNode,
+    isStateTreeNode,
+    getSnapshot,
+    isAlive,
+    getType,
+    isOptionalType
+} from "../../src"
 
 const Todo = types
     .model({
@@ -112,3 +132,179 @@ test("clone preserves environnment", () => {
         expect(env2 === getEnv(todo)).toBe(true)
     }
 })
+
+test("#1231", () => {
+    const envObj = createEnvironment()
+    const logs: string[] = []
+
+    function nofParents(node: IAnyStateTreeNode) {
+        let parents = 0
+        let parent = node
+        while (hasParent(parent)) {
+            parents++
+            parent = getParent(parent)
+        }
+        return parents
+    }
+
+    function leafsFirst(root: IAnyStateTreeNode) {
+        const nodes: IAnyStateTreeNode[] = []
+        walk(root, i => {
+            if (isStateTreeNode(i)) {
+                nodes.push(i)
+            }
+        })
+        // sort by number of parents
+        nodes.sort((a, b) => {
+            return nofParents(b) - nofParents(a)
+        })
+        return nodes
+    }
+
+    function check(root: Instance<typeof RS>, name: string, mode: "detach" | "destroy") {
+        function logFail(operation: string, n: any) {
+            logs.push(`fail: (${name}) ${operation}: ${getPath(n)}, ${n}`)
+        }
+        function log(operation: string, n: any) {
+            logs.push(`ok: (${name}) ${operation}: ${getPath(n)}, ${n}`)
+        }
+
+        // make sure all nodes are there
+        root.s1.arr[0].title
+        root.s1.m.get("one")!.title
+        root.s2
+        const nodes = leafsFirst(root)
+        expect(nodes.length).toBe(7)
+
+        nodes.forEach(i => {
+            const env = getEnv(i)
+            const parent = hasParent(i)
+            if (!parent && i !== root) {
+                logFail("expected a parent, but none found", i)
+            } else {
+                log("had parent or was root", i)
+            }
+            if (env !== envObj) {
+                logFail("expected same env as root, but was different", i)
+            } else {
+                log("same env as root", i)
+            }
+        })
+
+        unprotect(root)
+        nodes.forEach(i => {
+            const optional = optionalPaths.includes(getPath(i))
+            if (mode === "detach") {
+                log("detaching node", i)
+                detach(i)
+            } else {
+                log("destroying node", i)
+                destroy(i)
+            }
+            const env = getEnv(i)
+            const parent = hasParent(i)
+            const alive = isAlive(i)
+            if (mode === "detach") {
+                if (parent) {
+                    logFail(`expected no parent after detach, but one was found`, i)
+                } else {
+                    log(`no parent after detach`, i)
+                }
+                if (env !== envObj) {
+                    logFail("expected same env as root after detach, but it was not", i)
+                } else {
+                    log("env kept after detach", i)
+                }
+                if (!alive) {
+                    logFail("expected to be alive after detach, but it was not", i)
+                } else {
+                    log("alive after detach", i)
+                }
+            } else {
+                // destroy might or might not keep the env, but doesn't matter so we don't check
+                if (optional) {
+                    // optional (undefined) nodes will be assigned undefined and reconciled, therefore they will be kept alive
+                    if (!parent) {
+                        logFail(
+                            `expected a parent after destroy (since it is optional), but none was found`,
+                            i
+                        )
+                    } else {
+                        log(`had parent after destroy (since it is optional)`, i)
+                    }
+                    if (!alive) {
+                        logFail(
+                            "expected to be alive after destroy (since it is optional), but it was not",
+                            i
+                        )
+                    } else {
+                        log("alive after destroy (since it is optional)", i)
+                    }
+                } else {
+                    if (parent) {
+                        logFail(`expected no parent after destroy, but one was found`, i)
+                    } else {
+                        log(`no parent after destroy`, i)
+                    }
+                    if (alive) {
+                        logFail("expected to be dead after destroy, but it was not", i)
+                    } else {
+                        log("dead after destroy", i)
+                    }
+                }
+            }
+        })
+    }
+
+    const T = types.model("T", { title: "some title" })
+
+    const S1Arr = types.array(T)
+    const S1Map = types.map(T)
+    const S1 = types.model("S1", {
+        arr: S1Arr,
+        m: S1Map
+    })
+
+    const S2 = types.model("S2", {})
+
+    const RS = types.model("RS", {
+        s1: types.optional(S1, {}),
+        s2: types.optional(S2, {})
+    })
+
+    const optionalPaths = ["/s1", "/s2", "/s1/m", "/s1/arr"]
+
+    const data = {
+        s1: castToSnapshot(
+            S1.create({
+                arr: S1Arr.create([T.create({})]),
+                m: castToSnapshot(S1Map.create({ one: T.create({}) }))
+            })
+        ),
+        s2: S2.create()
+    }
+    const rsCreate = RS.create(data, envObj)
+    const rsCreate2 = clone(rsCreate, true)
+
+    const rsSnap = RS.create(
+        {
+            s1: {
+                arr: [{}],
+                m: { one: {} }
+            },
+            s2: {}
+        },
+        envObj
+    )
+    const rsSnap2 = clone(rsCreate, true)
+
+    check(rsCreate, "using create", "detach")
+    check(rsSnap, "using snapshot", "detach")
+    check(rsCreate2, "using create", "destroy")
+    check(rsSnap2, "using snapshot", "destroy")
+
+    const fails = logs.filter(l => l.startsWith("fail:"))
+    if (fails.length > 0) {
+        fail(`\n${fails.join("\n")}`)
+    }
+})

packages/mobx-state-tree/__tests__/core/hooks.test.ts

@@ -141,7 +141,7 @@ const Car = types
         id: types.number
     })
     .preProcessSnapshot<CarSnapshot>(snapshot =>
-        Object.assign({}, snapshot, { id: parseInt(snapshot.id) * 2 })
+        Object.assign({}, snapshot, { id: Number(snapshot.id) * 2 })
     )
     .postProcessSnapshot<CarSnapshot>(snapshot =>
         Object.assign({}, snapshot, { id: "" + snapshot.id / 2 })

packages/mobx-state-tree/src/core/mst-operations.ts

@@ -644,7 +644,7 @@ export function clone<T extends IAnyStateTreeNode>(
     return node.type.create(
         node.snapshot,
         keepEnvironment === true
-            ? node.environment
+            ? node.root.environment
             : keepEnvironment === false
             ? undefined
             : keepEnvironment
@@ -747,8 +747,8 @@ export function getEnv<T = any>(target: IAnyStateTreeNode): T {
     assertIsStateTreeNode(target, 1)
 
     const node = getStateTreeNode(target)
-    const env = node.environment
-    if (!!!env) return EMPTY_OBJECT as T
+    const env = node.root.environment
+    if (!env) return EMPTY_OBJECT as T
     return env
 }
 

packages/mobx-state-tree/src/core/node/object-node.ts

@@ -212,7 +212,7 @@ export class ObjectNode<C, S, T> extends BaseNode<C, S, T> {
         const type = this.type
 
         try {
-            this.storedValue = type.createNewInstance(this, this._childNodes, this._initialSnapshot)
+            this.storedValue = type.createNewInstance(this._childNodes)
             this.preboot()
 
             this._isRunningAction = true
@@ -254,8 +254,8 @@ export class ObjectNode<C, S, T> extends BaseNode<C, S, T> {
         const previousState = this.state
         this.state = NodeLifeCycle.DETACHING
 
-        const newEnv = this.parent.environment
         const root = this.root
+        const newEnv = root.environment
         const newIdCache = root.identifierCache!.splitCache(this)
 
         try {
@@ -272,7 +272,9 @@ export class ObjectNode<C, S, T> extends BaseNode<C, S, T> {
         const parentChanged = newParent !== this.parent
         const subpathChanged = subpath !== this.subpath
 
-        if (!parentChanged && !subpathChanged) return
+        if (!parentChanged && !subpathChanged) {
+            return
+        }
 
         if (process.env.NODE_ENV !== "production") {
             if (!subpath) {
@@ -298,7 +300,11 @@ export class ObjectNode<C, S, T> extends BaseNode<C, S, T> {
                     }/${subpath}'`
                 )
             }
-            if (!this.parent && !!this.environment && this.environment !== newParent.environment) {
+            if (
+                !this.parent &&
+                !!this.environment &&
+                this.environment !== newParent.root.environment
+            ) {
                 throw fail(
                     `A state tree cannot be made part of another state tree as long as their environments are different.`
                 )
@@ -307,6 +313,7 @@ export class ObjectNode<C, S, T> extends BaseNode<C, S, T> {
 
         if (parentChanged) {
             // attach to new parent
+            this.environment = undefined // will use root's
             newParent.root.identifierCache!.mergeCache(this)
             this.baseSetParent(newParent, subpath)
             this.fireHook(Hook.afterAttach)
@@ -464,10 +471,9 @@ export class ObjectNode<C, S, T> extends BaseNode<C, S, T> {
     }
 
     toString(): string {
+        const path = (this.isAlive ? this.path : this.pathUponDeath) || "<root>"
         const identifier = this.identifier ? `(id: ${this.identifier})` : ""
-        return `${this.type.name}@${this.path || "<root>"}${identifier}${
-            this.isAlive ? "" : "[dead]"
-        }`
+        return `${this.type.name}@${path}${identifier}${this.isAlive ? "" : " [dead]"}`
     }
 
     finalizeCreation(): void {

packages/mobx-state-tree/src/core/node/scalar-node.ts

@@ -57,7 +57,9 @@ export class ScalarNode<C, S, T> extends BaseNode<C, S, T> {
         const parentChanged = this.parent !== newParent
         const subpathChanged = this.subpath !== subpath
 
-        if (!parentChanged && !subpathChanged) return
+        if (!parentChanged && !subpathChanged) {
+            return
+        }
 
         if (process.env.NODE_ENV !== "production") {
             if (!subpath) {
@@ -74,6 +76,7 @@ export class ScalarNode<C, S, T> extends BaseNode<C, S, T> {
             }
         }
 
+        this.environment = undefined // use parent's
         this.baseSetParent(this.parent, subpath)
     }
 
@@ -86,7 +89,8 @@ export class ScalarNode<C, S, T> extends BaseNode<C, S, T> {
     }
 
     toString(): string {
-        return `${this.type.name}@${this.path || "<root>"}${this.isAlive ? "" : "[dead]"}`
+        const path = (this.isAlive ? this.path : this.pathUponDeath) || "<root>"
+        return `${this.type.name}@${path}${this.isAlive ? "" : " [dead]"}`
     }
 
     @action