Made onBecomeObserved behavior consistent by making keepAlive compute…

…ds possible reactiveContexts. Fixed mobxjs#2309

notes.md

@@ -66,13 +66,12 @@
     -   [x] ~add deprecation messages to latest mobx 5~ tricky; a lot of the api's are valid in v5 without alternative, so wouldn't result in actionable hints if the user doesn't intent to upgrade to v6
     -   [x] merge 2398
     -   [x] make `onReactionError` an option for `configure`
-    -   [ ] investigate the onbecomeunobserved issue
+    -   [x] investigate the onbecomeunobserved issue
     -   [x] support `flow`
     -   [x] fix #2415
     -   [x] fix #2404?
     -   [x] fix #2346?
-    -   [ ] fix #2309?
-    -   [ ]
+    -   [x] fix #2309?
 -   [ ] docs
     -   [ ] clean up docs
     -   [ ] fix interactive tut

src/core/computedvalue.ts

@@ -100,7 +100,7 @@ export class ComputedValue<T> implements IObservable, IComputedValue<T>, IDeriva
     scope_: Object | undefined
     private equals_: IEqualsComparer<any>
     private requiresReaction_: boolean
-    private keepAlive_: boolean
+    keepAlive_: boolean
 
     /**
      * Create a new computed value based on a function expression.
@@ -154,7 +154,12 @@ export class ComputedValue<T> implements IObservable, IComputedValue<T>, IDeriva
      */
     public get(): T {
         if (this.isComputing_) die(32, this.name_, this.derivation_)
-        if (globalState.inBatch === 0 && this.observers_.size === 0 && !this.keepAlive_) {
+        if (
+            globalState.inBatch === 0 &&
+            // !globalState.trackingDerivatpion &&
+            this.observers_.size === 0 &&
+            !this.keepAlive_
+        ) {
             if (shouldCompute(this)) {
                 this.warnAboutUntrackedRead_()
                 startBatch() // See perf test 'computed memoization'
@@ -163,7 +168,12 @@ export class ComputedValue<T> implements IObservable, IComputedValue<T>, IDeriva
             }
         } else {
             reportObserved(this)
-            if (shouldCompute(this)) if (this.trackAndCompute_()) propagateChangeConfirmed(this)
+            if (shouldCompute(this)) {
+                let prevTrackingContext = globalState.trackingContext
+                if (this.keepAlive_ && !prevTrackingContext) globalState.trackingContext = this
+                if (this.trackAndCompute_()) propagateChangeConfirmed(this)
+                globalState.trackingContext = prevTrackingContext
+            }
         }
         const result = this.value_!
 

src/core/derivation.ts

@@ -169,6 +169,7 @@ export function trackDerivedFunction<T>(derivation: IDerivation, f: () => T, con
     derivation.runId_ = ++globalState.runId
     const prevTracking = globalState.trackingDerivation
     globalState.trackingDerivation = derivation
+    globalState.inBatch++
     let result
     if (globalState.disableErrorBoundaries === true) {
         result = f.call(context)
@@ -179,6 +180,7 @@ export function trackDerivedFunction<T>(derivation: IDerivation, f: () => T, con
             result = new CaughtException(e)
         }
     }
+    globalState.inBatch--
     globalState.trackingDerivation = prevTracking
     bindDependencies(derivation)
 

src/core/globalstate.ts

@@ -1,4 +1,5 @@
 import { IDerivation, IObservable, Reaction, die, getGlobal } from "../internal"
+import { ComputedValue } from "./computedvalue"
 
 /**
  * These values will persist if global state is reset
@@ -45,7 +46,7 @@ export class MobXGlobals {
      * (Tracking derivation is also set for temporal tracking of computed values inside actions,
      * but trackingReaction can only be set by a form of Reaction)
      */
-    trackingReaction: Reaction | null = null
+    trackingContext: Reaction | ComputedValue<any> | null = null
 
     /**
      * Each time a derivation is tracked, it is assigned a unique run-id

src/core/observable.ts

@@ -145,7 +145,7 @@ export function reportObserved(observable: IObservable): boolean {
             observable.lastAccessedBy_ = derivation.runId_
             // Tried storing newObserving, or observing, or both as Set, but performance didn't come close...
             derivation.newObserving_![derivation.unboundDepsCount_++] = observable
-            if (globalState.trackingReaction && !observable.isBeingObserved_) {
+            if (!observable.isBeingObserved_ && globalState.trackingContext) {
                 observable.isBeingObserved_ = true
                 observable.onBO()
             }

src/core/reaction.ts

@@ -130,10 +130,10 @@ export class Reaction implements IDerivation, IReactionPublic {
             })
         }
         this.isRunning_ = true
-        const prevReaction = globalState.trackingReaction // reactions could create reactions...
-        globalState.trackingReaction = this
+        const prevReaction = globalState.trackingContext // reactions could create reactions...
+        globalState.trackingContext = this
         const result = trackDerivedFunction(this, fn, undefined)
-        globalState.trackingReaction = prevReaction
+        globalState.trackingContext = prevReaction
         this.isRunning_ = false
         this.isTrackPending_ = false
         if (this.isDisposed_) {

test/v5/base/become-observed.ts

@@ -60,3 +60,212 @@ test("#2309 don't trigger oBO for computeds that aren't subscribed to", () => {
     asd.actionComputed()
     expect(events).toEqual(["--"])
 })
+
+describe("#2309 onBecomeObserved inconsistencies", () => {
+    let events: string[] = []
+
+    beforeEach(() => {
+        events = []
+    })
+
+    test("caseA", () => {
+        // Computed {keepAlive: false} -> Observable
+        const o = observable.box(1)
+        const ca = computed(
+            () => {
+                return o.get()
+            },
+            { keepAlive: false }
+        )
+
+        onBecomeObserved(o, () => events.push(`o observed`))
+        onBecomeUnobserved(o, () => events.push(`o unobserved`))
+        onBecomeObserved(ca, () => events.push(`ca observed`))
+        onBecomeUnobserved(ca, () => events.push(`ca unobserved`))
+        expect(events).toEqual([])
+        ca.get()
+        expect(events).toEqual([])
+    })
+
+    test("caseB", () => {
+        // Computed {keepAlive: false} -> Computed {keepAlive: false} -> Observable
+        const o = observable.box(1)
+        const ca = computed(
+            () => {
+                return o.get()
+            },
+            { keepAlive: false }
+        )
+
+        const cb = computed(
+            () => {
+                return ca.get() * 2
+            },
+            { keepAlive: false }
+        )
+
+        onBecomeObserved(o, () => events.push(`o observed`))
+        onBecomeUnobserved(o, () => events.push(`o unobserved`))
+        onBecomeObserved(ca, () => events.push(`ca observed`))
+        onBecomeUnobserved(ca, () => events.push(`ca unobserved`))
+        onBecomeObserved(cb, () => events.push(`cb observed`))
+        onBecomeUnobserved(cb, () => events.push(`cb unobserved`))
+        expect(events).toEqual([])
+        cb.get()
+        expect(events).toEqual([])
+    })
+
+    test("caseC", () => {
+        // Computed {keepAlive: true} -> Observable
+        const o = observable.box(1)
+        const ca = computed(
+            () => {
+                return o.get()
+            },
+            { keepAlive: true }
+        )
+
+        onBecomeObserved(o, () => events.push(`o observed`))
+        onBecomeUnobserved(o, () => events.push(`o unobserved`))
+        onBecomeObserved(ca, () => events.push(`ca observed`))
+        onBecomeUnobserved(ca, () => events.push(`ca unobserved`))
+        expect(events).toEqual([])
+        ca.get()
+        expect(events).toEqual(["o observed"]) // everything is hot, and 'o' is really observed so that the keptAlive computed knows about its state
+    })
+
+    test("caseD", () => {
+        // Computed {keepAlive: true} -> Computed {keepAlive: false} -> Observable
+        // logs: `o observed`
+        // potential issue: why are the callbacks not called on `ca` ?
+        const o = observable.box(1)
+        const ca = computed(
+            () => {
+                return o.get()
+            },
+            { keepAlive: false }
+        )
+
+        const cb = computed(
+            () => {
+                return ca.get() * 2
+            },
+            { keepAlive: true }
+        )
+
+        onBecomeObserved(o, () => events.push(`o observed`))
+        onBecomeUnobserved(o, () => events.push(`o unobserved`))
+        onBecomeObserved(ca, () => events.push(`ca observed`))
+        onBecomeUnobserved(ca, () => events.push(`ca unobserved`))
+        onBecomeObserved(cb, () => events.push(`cb observed`))
+        onBecomeUnobserved(cb, () => events.push(`cb unobserved`))
+        expect(events).toEqual([])
+        cb.get()
+        expect(events).toEqual(["ca observed", "o observed"]) // see above
+    })
+
+    test("caseE - base", () => {
+        const o = observable.box(1)
+        const ca = computed(
+            () => {
+                return o.get()
+            },
+            { keepAlive: false }
+        )
+
+        const cb = computed(
+            () => {
+                return ca.get() * 2
+            },
+            { keepAlive: false }
+        )
+
+        onBecomeObserved(o, () => events.push(`o observed`))
+        onBecomeUnobserved(o, () => events.push(`o unobserved`))
+        onBecomeObserved(ca, () => events.push(`ca observed`))
+        onBecomeUnobserved(ca, () => events.push(`ca unobserved`))
+        onBecomeObserved(cb, () => events.push(`cb observed`))
+        onBecomeUnobserved(cb, () => events.push(`cb unobserved`))
+
+        const u = autorun(() => cb.get())
+        u()
+        expect(events).toEqual([
+            "cb observed",
+            "ca observed",
+            "o observed",
+            "cb unobserved",
+            "ca unobserved",
+            "o unobserved"
+        ])
+    })
+
+    test("caseE", () => {
+        const o = observable.box(1)
+        const ca = computed(
+            () => {
+                return o.get()
+            },
+            { keepAlive: false }
+        )
+
+        const cb = computed(
+            () => {
+                return ca.get() * 2
+            },
+            { keepAlive: true }
+        )
+
+        onBecomeObserved(o, () => events.push(`o observed`))
+        onBecomeUnobserved(o, () => events.push(`o unobserved`))
+        onBecomeObserved(ca, () => events.push(`ca observed`))
+        onBecomeUnobserved(ca, () => events.push(`ca unobserved`))
+        onBecomeObserved(cb, () => events.push(`cb observed`))
+        onBecomeUnobserved(cb, () => events.push(`cb unobserved`))
+
+        const u = autorun(() => cb.get())
+        u()
+        // Note that at this point the observables never become unobserved anymore!
+        // That is correct, because if doing our kept-alive computed doesn't recompute until reobserved,
+        // itself it is still observing all the values of its own deps to figure whether it is still
+        // up to date or not
+        expect(events).toEqual(["cb observed", "ca observed", "o observed", "cb unobserved"])
+
+        events.splice(0)
+        const u2 = autorun(() => cb.get())
+        u2()
+        expect(events).toEqual(["cb observed", "cb unobserved"])
+    })
+
+    test("caseF", () => {
+        // Computed {keepAlive: true} -> Computed {keepAlive: false} -> Observable
+        // cb.get() first then autorun() then unsub()
+        const o = observable.box(1)
+        const ca = computed(
+            () => {
+                return o.get()
+            },
+            { keepAlive: false }
+        )
+
+        const cb = computed(
+            () => {
+                return ca.get() * 2
+            },
+            { keepAlive: true }
+        )
+
+        onBecomeObserved(o, () => events.push(`o observed`))
+        onBecomeUnobserved(o, () => events.push(`o unobserved`))
+        onBecomeObserved(ca, () => events.push(`ca observed`))
+        onBecomeUnobserved(ca, () => events.push(`ca unobserved`))
+        onBecomeObserved(cb, () => events.push(`cb observed`))
+        onBecomeUnobserved(cb, () => events.push(`cb unobserved`))
+        cb.get()
+
+        expect(events).toEqual(["ca observed", "o observed"])
+        events.splice(0)
+        const u = autorun(() => cb.get())
+        u()
+        expect(events).toEqual(["cb observed", "cb unobserved"])
+    })
+})