Merge pull request #2032 from mobxjs/fix-observablemap-leak

Use onBecomeUnobserved for ObservableMap._hasMap garbage collection
mobxjs · Jul 17, 2019 · bf2a60c · bf2a60c
2 parents b1861d7 + 152f283
commit bf2a60c
Show file tree

Hide file tree

Showing 2 changed files with 40 additions and 14 deletions.
diff --git a/src/types/observablemap.ts b/src/types/observablemap.ts
@@ -31,6 +31,7 @@ import {
     stringifyKey,
     transaction,
     untracked,
+    onBecomeUnobserved,
     globalState
 } from "../internal"
 
@@ -108,8 +109,22 @@ export class ObservableMap<K = any, V = any>
     }
 
     has(key: K): boolean {
-        if (this._hasMap.has(key)) return this._hasMap.get(key)!.get()
-        return this._updateHasMapEntry(key, false).get()
+        if (!globalState.trackingDerivation) return this._has(key)
+
+        let entry = this._hasMap.get(key)
+        if (!entry) {
+            // todo: replace with atom (breaking change)
+            const newEntry = (entry = new ObservableValue(
+                this._has(key),
+                referenceEnhancer,
+                `${this.name}.${stringifyKey(key)}?`,
+                false
+            ))
+            this._hasMap.set(key, newEntry)
+            onBecomeUnobserved(newEntry, () => this._hasMap.delete(key))
+        }
+
+        return entry.get()
     }
 
     set(key: K, value: V) {
@@ -170,21 +185,11 @@ export class ObservableMap<K = any, V = any>
         return false
     }
 
-    private _updateHasMapEntry(key: K, value: boolean): ObservableValue<boolean> {
-        // optimization; don't fill the hasMap if we are not observing, or remove entry if there are no observers anymore
+    private _updateHasMapEntry(key: K, value: boolean) {
         let entry = this._hasMap.get(key)
         if (entry) {
             entry.setNewValue(value)
-        } else {
-            entry = new ObservableValue(
-                value,
-                referenceEnhancer,
-                `${this.name}.${stringifyKey(key)}?`,
-                false
-            )
-            this._hasMap.set(key, entry)
         }
-        return entry
     }
 
     private _updateValue(key: K, newValue: V | undefined) {

diff --git a/test/base/map.js b/test/base/map.js
@@ -227,7 +227,28 @@ test("cleanup", function() {
     disposer()
     expect(aValue).toBe(2)
     expect(observable.observers.size).toBe(0)
-    expect(x._hasMap.get("a").observers.size).toBe(0)
+    expect(x._hasMap.has("a")).toBe(false)
+})
+
+test("getAtom encapsulation leak test", function() {
+    const x = map({})
+
+    let disposer = autorun(function() {
+        x.has("a")
+    })
+
+    let atom = mobx.getAtom(x, "a")
+
+    disposer()
+
+    expect(x._hasMap.get("a")).toBe(undefined)
+
+    disposer = autorun(function() {
+        x.has("a")
+        atom && atom.reportObserved()
+    })
+
+    expect(x._hasMap.get("a")).not.toBe(atom)
 })
 
 test("strict", function() {