Reactions are executed after onPatches listener returns

[sisp]

Related to #96, I'm wondering whether there is any argument against executing reactions inside the onPatches listener, i.e. when an action inside an onPatches listener triggers a reaction the reaction would be executed right after the action call and before the listener returns. The onPatches listener seems to be wrapped as an action

https://github.com/xaviergonz/mobx-keystone/blob/44a1cf2fb7d322ce09ea50eb47457074ca1a0177/packages/lib/src/patch/emitPatch.ts#L59-L61

but even when I remove this part, reactions are still executed after the listener returns, so something else must be causing this behavior and I can't seem to find it.

This is a test case meant to be included in test/patch/patch.test.ts for the behavior I'd like to have:

test("reaction is executed inside onPatches listener", () => {
  let reactionCalls = 0

  @model("test/reactionOnPatchesListener/M")
  class M extends Model({ a: prop<number>(10), b: prop<number>(20) }) {
    onInit() {
      autoDispose(
        reaction(
          () => this.b,
          () => {
            reactionCalls++
          }
        )
      )
    }
  }

  const m = new M({})

  autoDispose(
    onPatches(m, () => {
      expect(reactionCalls).toBe(0)

      runUnprotected(() => {
        m.b = 21
      })

      expect(reactionCalls).toBe(1)
    })
  )

  runUnprotected(() => {
    m.a = 11
  })
})

This is the behavior I would have expected, but in the context of #96 I discovered that a reaction is executed after the onPatches listener returns.

[sisp]

After further investigation, I believe the patches generated by

runUnprotected(() => {
  m.a = 11
})

are applied before the callback returns, so the onPatches listener is executed inside an action which prevents reactions inside the listener from being executed before it returns.

[sisp]

The use of observe causes the current behavior, e.g.:

https://github.com/xaviergonz/mobx-keystone/blob/779116ecf481ec690cd77de8ec166c9eb2a4c2f2/packages/lib/src/tweaker/tweakPlainObject.ts#L86

observe reacts to mutations, when they are being made, while reactions like autorun or reaction react to new values when they become available.

-- https://mobx.js.org/refguide/observe.html

Since the patches are emitted inside the observe listener call, the onPatches listener is invoked immediately, ignoring an active transaction.

[xaviergonz]

Wait, let me get this straight, you expect the onPatches listener to trigger when something is changed:

a) after all current actions are finished b) immediately

and then reactions from changes triggered inside onPatches to be triggered:

a) after all current actions are finished (including the current onPatches listener) b) immediately

?

[sisp]

Wait, let me get this straight, you expect the onPatches listener to trigger when something is changed:

a) after all current actions are finished b) immediately

a)

and then reactions from changes triggered inside onPatches to be triggered:

a) after all current actions are finished (including the current onPatches listener) b) immediately

b)

Is this contradictory somehow or is there any counter example that makes this a bad idea?

[xaviergonz]

and what happens currently is b / a ?

Would it be ok if it was changed to be a / a, or it has to be a / b to fix the issue?

[sisp]

and what happens currently is b / a ?

From what I can see, yes.

Would it be ok if it was changed to be a / a, or it has to be a / b to fix the issue?

I think a/b and b/b would solve the problem, so the second option must be b. The reason is that

https://github.com/xaviergonz/mobx-keystone/blob/ee6d06fa7a89b6b790ab989dbb30e141e447b85c/packages/lib/src/treeUtils/sandbox.ts#L75-L82

does not work when the tree contains a reaction because when applyPatches is finished, the reaction that results from applying the patches is not executed immediately (inside allowWrite) but after the onPatches listener returns, i.e. outside allowWrite.

[xaviergonz]

This PR in theory addresses that problem for sandboxes.

107

The problem with this approach though is that this actually allows the sandbox copy and the original copy to be out of sync (as shown in the unit test)

[sisp]

https://github.com/xaviergonz/mobx-keystone/pull/114 is a good solution to this problem, so I'll close this issue.