xaviergonz / mobx-keystone

A MobX powered state management solution based on data trees with first class support for Typescript, support for snapshots, patches and much more
https://mobx-keystone.js.org
MIT License
554 stars 25 forks source link

Array of models in computedTree #483

Closed ZFail closed 2 years ago

ZFail commented 2 years ago

Does computedTree support an array of models? I get an error "an object cannot be assigned a new parent when it already has one" when the tree is updated for the second time. https://codesandbox.io/s/mobx-keystone-computedtree-cscdc

xaviergonz commented 2 years ago

@sisp

sisp commented 2 years ago

Already checking. I've been able to reproduce the error with this extension of the tests:

--- a/packages/lib/test/computedTree/computedTree.test.ts
+++ b/packages/lib/test/computedTree/computedTree.test.ts
@@ -73,6 +73,14 @@ class R extends Model({
     return ["abc"]
   }

+  @computedTree
+  get arrayOfModels() {
+    return [
+      new M({ id: `${this.id}.model1`, value: this.value }),
+      new M({ id: `${this.id}.model2`, value: this.value }),
+    ]
+  }
+
   @computedTree
   get plainObject() {
     return { key: "value" }
@@ -481,3 +489,9 @@ test("computed tree is readonly", () => {
     "tried to invoke action 'setValue' over a readonly node"
   )
 })
+
+test("computed tree works with an array of models", () => {
+  const r = new R({})
+  r.setValue(10)
+  expect(r.arrayOfModels.map((m) => m.value)).toEqual([10, 10])
+})

But I'm not sure yet why the error is happening.

@xaviergonz Do you think

https://github.com/xaviergonz/mobx-keystone/blob/e35a792789fc66734b43cbf801d93a4e29ef9bd1/packages/lib/src/computedTree/computedTree.ts#L78-L79

is correct or should entry.tweakedValue be untweaked as well somehow? I've been trying to debug it already but without success so far.

sisp commented 2 years ago

The error is occurring when this line is executed:

https://github.com/xaviergonz/mobx-keystone/blob/e35a792789fc66734b43cbf801d93a4e29ef9bd1/packages/lib/src/computedTree/computedTree.ts#L78

sisp commented 2 years ago

This fixes the new test:

--- a/packages/lib/src/computedTree/computedTree.ts
+++ b/packages/lib/src/computedTree/computedTree.ts
@@ -75,8 +75,9 @@ export function computedTree(
       return entry.tweakedValue
     }

-    tweak(oldValue, undefined)
-    tryUntweak(oldValue)
+    const oldTweakedValue = entry.tweakedValue
+    tweak(oldTweakedValue, undefined)
+    tryUntweak(oldTweakedValue)

     const tweakedValue = tweakComputedTreeNode(newValue, this, propertyKey)
     entry.value = newValue

But the getChildrenObjects and walkTree tests are failing then. :thinking:

sisp commented 2 years ago

My bad, those tests need to be adapted.

sisp commented 2 years ago

I'll submit a PR with a fix later today.

xaviergonz commented 2 years ago

Fixed in v1.1.1. Thanks!