Observables in a sandbox are not tracked outside the sandbox

[sisp]

A computed property which uses a sandbox to determine its return value is not tracked. The test is meant to be included in packages/lib/tests/treeUtils/sandbox.test.ts:

test("sandbox props are tracked", () => {
  const sandboxContext = createContext<SandboxManager>()

  @model("R")
  class R extends Model({ a: prop<A>() }) {
    @computed
    get times2(): number {
      return sandboxContext.get(this)!.withSandbox(this.a.b, b => {
        b.setValue(b.value * 2)
        return { commit: false, return: b.value }
      })
    }
  }

  const r = new R({ a: new A({ b: new B({ value: 0 }) }) })
  const manager = sandbox(r)
  autoDispose(() => manager.dispose())
  sandboxContext.setComputed(r, () => manager)

  const values: number[] = []
  autoDispose(
    reaction(
      () => r.times2,
      times2 => values.push(times2)
    )
  )

  r.a.b.setValue(1)
  expect(values).toEqual([2])

  r.a.b.setValue(2)
  expect(values).toEqual([2, 4])
})

This test results in the following error:

[mobx] Encountered an uncaught exception that was thrown by a reaction or observer component, in: 'Reaction[Reaction@222]' Error: [mobx] Computed values are not allowed to cause side effects by changing observables that are already being observed. Tried to modify: ObservableObject@214.value
    at invariant (mobx-keystone/node_modules/mobx/lib/mobx.js:94:15)
    at fail (mobx-keystone/node_modules/mobx/lib/mobx.js:89:5)
    at checkIfStateModificationsAreAllowed (mobx-keystone/node_modules/mobx/lib/mobx.js:726:9)
    at ObservableValue.Object.<anonymous>.ObservableValue.prepareNewValue (mobx-keystone/node_modules/mobx/lib/mobx.js:1044:9)
    at ObservableObjectAdministration.Object.<anonymous>.ObservableObjectAdministration.write (mobx-keystone/node_modules/mobx/lib/mobx.js:4012:31)
    at set (mobx-keystone/node_modules/mobx/lib/mobx.js:2615:17)
    at Object.set (mobx-keystone/node_modules/mobx/lib/mobx.js:2944:9)
    at BaseModel.set (mobx-keystone/packages/lib/src/model/Model.ts:250:28)
    at BaseModel.setValue (mobx-keystone/packages/lib/test/treeUtils/sandbox.test.ts:30:15)
    at BaseModel.<anonymous> (mobx-keystone/packages/lib/src/action/wrapInAction.ts:77:19)
    at executeAction (mobx-keystone/node_modules/mobx/lib/mobx.js:910:19)
    at BaseModel.res (mobx-keystone/node_modules/mobx/lib/mobx.js:902:16)
    at mobx-keystone/packages/lib/test/treeUtils/sandbox.test.ts:356:11
    at mobx-keystone/packages/lib/src/treeUtils/sandbox.ts:123:9
    at SandboxManager.allowWrite (mobx-keystone/packages/lib/src/actionMiddlewares/readonlyMiddleware.ts:94:16)
    at SandboxManager.withSandbox (mobx-keystone/packages/lib/src/treeUtils/sandbox.ts:122:32)
    at BaseModel.get times2 (mobx-keystone/packages/lib/test/treeUtils/sandbox.test.ts:355:40)
    at trackDerivedFunction (mobx-keystone/node_modules/mobx/lib/mobx.js:764:24)
    at ComputedValue.Object.<anonymous>.ComputedValue.computeValue (mobx-keystone/node_modules/mobx/lib/mobx.js:1254:19)
    at ComputedValue.Object.<anonymous>.ComputedValue.trackAndCompute (mobx-keystone/node_modules/mobx/lib/mobx.js:1239:29)
    at ComputedValue.Object.<anonymous>.ComputedValue.get (mobx-keystone/node_modules/mobx/lib/mobx.js:1199:26)
    at shouldCompute (mobx-keystone/node_modules/mobx/lib/mobx.js:684:33)
    at Reaction.Object.<anonymous>.Reaction.runReaction (mobx-keystone/node_modules/mobx/lib/mobx.js:1754:17)
    at runReactionsHelper (mobx-keystone/node_modules/mobx/lib/mobx.js:1894:35)
    at reactionScheduler (mobx-keystone/node_modules/mobx/lib/mobx.js:1872:47)
    at runReactions (mobx-keystone/node_modules/mobx/lib/mobx.js:1877:5)
    at endBatch (mobx-keystone/node_modules/mobx/lib/mobx.js:1577:9)
    at _endAction (mobx-keystone/node_modules/mobx/lib/mobx.js:963:5)
    at executeAction (mobx-keystone/node_modules/mobx/lib/mobx.js:917:9)
    at BaseModel.res (mobx-keystone/node_modules/mobx/lib/mobx.js:902:16)
    at Object.<anonymous> (mobx-keystone/packages/lib/test/treeUtils/sandbox.test.ts:375:9)
    at Object.asyncJestTest (mobx-keystone/node_modules/jest-jasmine2/build/jasmineAsyncInstall.js:102:37)
    at mobx-keystone/node_modules/jest-jasmine2/build/queueRunner.js:43:12
    at new Promise (<anonymous>)
    at mapper (mobx-keystone/node_modules/jest-jasmine2/build/queueRunner.js:26:19)
    at mobx-keystone/node_modules/jest-jasmine2/build/queueRunner.js:73:41
    at processTicksAndRejections (internal/process/task_queues.js:93:5)

[sisp]

The more I think about it, the more it feels like the idea of a sandbox in which actions can be performed contradicts the idea of computed properties and that's why the above error occurs. But if that's the case, I still believe that it's a valid use case to have a computed property that needs to run actions (on a copy of the state) in order to determine its return value because it must simulate a possible future state and evaluate the result. Then, the computation performed in the computed property is not purely derivative, but at the same time there is no permanent state mutation if the state copy is discarded or put back in sync with the original state. I'm a bit stuck now seeing options how to make this possible, though.

[xaviergonz]

Yeah, the weird thing here is that a computed is not supposed to alter state, and withSandbox is supposed to alter state, so they are contradictory. Do you really need it to be computed?

Maybe in this case it should really be a reaction/autorun + observable volatile?

@observable times2?: number

onInit() { // maybe onAttachedToRootStore?
  // this should automatically run everytime tracked (used) values change
  // in this case if this.a.b or sandobox.b.value changes

  // if it shouldn't autorun when sandbox.b.value changes then either
  // make the callback to withSandbox an action (which is untracked)
  // or use a reaction and specify explicitely what needs to be tracked
  const disp = autorun(() => {
    const ctx = sandboxContext.get(this)
    if (!ctx) return
    this.times2 = ctx.withSandbox(this.a.b, b => {
        b.setValue(b.value * 2)
        return { commit: false, return: b.value }
      })
  })
  return disp;
}
...
whatever.times2

?

[sisp]

That's more or less how I'm doing it right now to work around the problem with a computed property. It just feels like a workaround.

[sisp]

By the way, the workaround using autorun results in a cycle:

test("observables in sandbox are tracked (autorun)", () => {
  const sandboxContext = createContext<SandboxManager>()

  @model("R2")
  class R extends Model({ a: prop<A>() }) {
    @observable
    times2!: number

    onAttachedToRootStore() {
      return autorun(() => {
        const ctx = sandboxContext.get(this)
        if (!ctx) return
        const result = ctx.withSandbox(this.a.b, b => {
          b.setValue(b.value * 2)
          return { commit: false, return: b.value }
        })
        runInAction(() => {
          this.times2 = result
        })
      })
    }
  }

  const r = new R({ a: new A({ b: new B({ value: 0 }) }) })
  autoDispose(() => {
    if (isRootStore(r)) {
      unregisterRootStore(r)
    }
  })

  const manager = sandbox(r)
  autoDispose(() => manager.dispose())
  sandboxContext.set(r, manager)

  registerRootStore(r)

  const times2: number[] = []
  autoDispose(
    reaction(
      () => r.times2,
      t2 => times2.push(t2)
    )
  )

  r.a.b.setValue(2)

  expect(times2).toEqual([4])
})

Reaction doesn't converge to a stable state after 100 iterations. Probably there is a cycle in the reactive function: Reaction[Autorun@314]

I can make it work using reaction if I know the exact dependencies, but automated dependency tracking would be safer and more convenient.

[sisp]

Interestingly, this (computed property with sandbox) works with both autorun and reaction:

test("observables in sandbox are tracked", () => {
  const sandboxContext = createContext<SandboxManager>()

  @model("R2")
  class R extends Model({ a: prop<A>(), x: prop<number>() }) {
    @computed
    get times2(): number {
      return computed(() => { // equivalent to `expr` from `mobx-utils`
        const ctx = sandboxContext.get(this)
        if (!ctx) {
          throw new Error("sandbox context required")
        }
        let result!: number
        _allowStateChangesInsideComputed(() => { // from `mobx`
          result = ctx.withSandbox(this.a.b, b => {
            b.setValue(b.value * 2)
            return { commit: false, return: b.value }
          })
        })
        return result
      }).get()
    }

    @modelAction
    incX(): void {
      this.x++
    }
  }

  const r = new R({ a: new A({ b: new B({ value: 0 }) }), x: 0 })

  const manager = sandbox(r)
  autoDispose(() => manager.dispose())
  sandboxContext.set(r, manager)

  const times2: number[] = []
  autoDispose(autorun(() => times2.push(r.times2)))

  r.a.b.setValue(2)
  r.a.b.setValue(3)
  r.incX() // should trigger no reaction of `r.times2`

  expect(times2).toEqual([0, 4, 6])
})

I'm not exactly sure why. Do you know?