fix: earlier onBecome(Un)Observed disposer will not dispose later lis…

…tener (fix mobxjs#1537)
fi3ework · Dec 7, 2018 · 7d42f47 · 7d42f47
1 parent d55c50a
commit 7d42f47
Show file tree

Hide file tree

Showing 4 changed files with 101 additions and 14 deletions.
diff --git a/src/api/become-observed.ts b/src/api/become-observed.ts
@@ -38,16 +38,25 @@ function interceptHook(hook: "onBecomeObserved" | "onBecomeUnobserved", thing, a
     const atom: IObservable =
         typeof arg2 === "string" ? getAtom(thing, arg2) : (getAtom(thing) as any)
     const cb = typeof arg2 === "string" ? arg3 : arg2
-    const orig = atom[hook]
+    const listenersKey = `${hook}Listeners`
+
+    if (!atom[listenersKey]) {
+        atom[listenersKey] = new Set<Lambda>([cb])
+    } else {
+        ;(atom[listenersKey] as Set<Lambda>).add(cb)
+    }
 
+    const orig = atom[hook]
     if (typeof orig !== "function")
         return fail(process.env.NODE_ENV !== "production" && "Not an atom that can be (un)observed")
 
-    atom[hook] = function() {
-        orig.call(this)
-        cb.call(this)
-    }
     return function() {
-        atom[hook] = orig
+        const hookListeners = atom[listenersKey] as Set<Lambda> | undefined
+        if (hookListeners) {
+            hookListeners.delete(cb)
+            if (hookListeners.size === 0) {
+                delete atom[listenersKey]
+            }
+        }
     }
 }
diff --git a/src/core/atom.ts b/src/core/atom.ts
@@ -11,6 +11,7 @@ import {
     reportObserved,
     startBatch
 } from "../internal"
+import { Lambda } from "../utils/utils"
 
 export const $mobx = Symbol("mobx administration")
 
@@ -33,12 +34,19 @@ export class Atom implements IAtom {
      */
     constructor(public name = "Atom@" + getNextId()) {}
 
-    public onBecomeUnobserved() {
-        // noop
-    }
+    public onBecomeObservedListeners: Set<Lambda> | undefined
+    public onBecomeUnobservedListeners: Set<Lambda> | undefined
 
     public onBecomeObserved() {
-        /* noop */
+        if (this.onBecomeObservedListeners) {
+            this.onBecomeObservedListeners.forEach(listener => listener())
+        }
+    }
+
+    public onBecomeUnobserved() {
+        if (this.onBecomeUnobservedListeners) {
+            this.onBecomeUnobservedListeners.forEach(listener => listener())
+        }
     }
 
     /**
@@ -71,7 +79,13 @@ export function createAtom(
     onBecomeUnobservedHandler: () => void = noop
 ): IAtom {
     const atom = new Atom(name)
-    onBecomeObserved(atom, onBecomeObservedHandler)
-    onBecomeUnobserved(atom, onBecomeUnobservedHandler)
+    // default `noop` listener will not initialize the hook Set
+    if (onBecomeObservedHandler !== noop) {
+        onBecomeObserved(atom, onBecomeObservedHandler)
+    }
+
+    if (onBecomeUnobservedHandler !== noop) {
+        onBecomeUnobserved(atom, onBecomeUnobservedHandler)
+    }
     return atom
 }
diff --git a/src/core/computedvalue.ts b/src/core/computedvalue.ts
@@ -124,9 +124,20 @@ export class ComputedValue<T> implements IObservable, IComputedValue<T>, IDeriva
         propagateMaybeChanged(this)
     }
 
-    onBecomeUnobserved() {}
+    public onBecomeObservedListeners: Set<Lambda> | undefined
+    public onBecomeUnobservedListeners: Set<Lambda> | undefined
 
-    onBecomeObserved() {}
+    public onBecomeObserved() {
+        if (this.onBecomeObservedListeners) {
+            this.onBecomeObservedListeners.forEach(listener => listener())
+        }
+    }
+
+    public onBecomeUnobserved() {
+        if (this.onBecomeUnobservedListeners) {
+            this.onBecomeUnobservedListeners.forEach(listener => listener())
+        }
+    }
 
     /**
      * Returns the current value of this computed value.

diff --git a/test/base/extras.js b/test/base/extras.js
@@ -399,6 +399,59 @@ test("onBecome(Un)Observed - less simple", () => {
     expect(events.length).toBe(0)
 })
 
+test("onBecomeObserved correctly disposes second listener #1537", () => {
+    const x = mobx.observable.box(3)
+    const events = []
+    const d1 = mobx.onBecomeObserved(x, "a", () => {
+        events.push("a observed")
+    })
+    const d2 = mobx.onBecomeObserved(x, "b", () => {
+        events.push("b observed")
+    })
+    d1()
+    mobx.reaction(() => x.get(), () => {})
+    expect(events.length).toBe(1)
+    expect(events).toEqual(["b observed"])
+})
+
+test("onBecomeObserved correctly disposes second listener #1537", () => {
+    const x = mobx.observable.box(3)
+    const events = []
+    const d1 = mobx.onBecomeObserved(x, "a", () => {
+        events.push("a observed")
+    })
+    const d2 = mobx.onBecomeObserved(x, "b", () => {
+        events.push("b observed")
+    })
+    d1()
+    const d3 = mobx.reaction(() => x.get(), () => {})
+    d3()
+    expect(events.length).toBe(1)
+    expect(events).toEqual(["b observed"])
+    d2()
+    const d4 = mobx.reaction(() => x.get(), () => {})
+    expect(events).toEqual(["b observed"])
+})
+
+test("onBecomeUnobserved correctly disposes second listener #1537", () => {
+    const x = mobx.observable.box(3)
+    const events = []
+    const d1 = mobx.onBecomeUnobserved(x, "a", () => {
+        events.push("a unobserved")
+    })
+    const d2 = mobx.onBecomeUnobserved(x, "b", () => {
+        events.push("b unobserved")
+    })
+    d1()
+    const d3 = mobx.reaction(() => x.get(), () => {})
+    d3()
+    expect(events.length).toBe(1)
+    expect(events).toEqual(["b unobserved"])
+    d2()
+    const d4 = mobx.reaction(() => x.get(), () => {})
+    expect(events).toEqual(["b unobserved"])
+})
+
 test("deepEquals should yield correct results for complex objects #1118 - 1", () => {
     const d2016jan1 = new Date("2016-01-01")
     const d2016jan1_2 = new Date("2016-01-01")