fix(4610): computed keepAlive_ 'true' by default

.changeset/big-dots-run.md

@@ -0,0 +1,17 @@
+---
+"mobx": minor
+---
+
+Adds a global flag for computed `keepAlive` default value:
+`configure({ globalKeepAliveState })`
+
+Current behavior is preserved by default (`globalKeepAliveState: false`),
+explicit `keepAlive` still takes precedence
+
+Computed keepAlive now resolves in this order:
+
+1. explicit `keepAlive`
+2. `configure({ globalKeepAliveState })`
+3. default `true`
+
+MobX now warns once when a computed is created without an explicit `keepAlive`, to help prepare for the next major version where the default will change.

docs/computeds.md

@@ -98,17 +98,11 @@ When using computed values there are a couple of best practices to follow:
 
 ## Tips
 
-<details id="computed-suspend"><summary>**Tip:** computed values will be suspended if they are _not_ observed<a href="#computed-suspend" class="tip-anchor"></a></summary>
+<details id="computed-suspend"><summary>**Tip:** computed values will be suspended if they are _not_ observed (by default)<a href="#computed-suspend" class="tip-anchor"></a></summary>
 
-It sometimes confuses people new to MobX, perhaps used to a library like [Reselect](https://github.com/reduxjs/reselect), that if you create a computed property but don't use it anywhere in a reaction, it is not memoized and appears to be recomputed more often than necessary.
-For example, if we extended the above example with calling `console.log(order.total)` twice, after we called `stop()`, the value would be recomputed twice.
+By default, computeds that are read outside a reaction are not kept alive, so they can recompute on each untracked access. _This will be changed in next major release_
 
-This allows MobX to automatically suspend computations that are not actively in use
-to avoid unnecessary updates to computed values that are not being accessed. But if a computed property is _not_ in use by some reaction, then computed expressions are evaluated each time their value is requested, so they behave just like a normal property.
-
-If you only fiddle around computed properties might not seem efficient, but when applied in a project that uses `observer`, `autorun`, etc., they become very efficient.
-
-The following code demonstrates the issue:
+The following code demonstrates it:
 
 ```javascript
 // OrderLine has a computed property `total`.
@@ -120,8 +114,8 @@ setInterval(() => {
 }, 60)
 ```
 
-It can be overridden by setting the annotation with the `keepAlive` option ([try it out yourself](https://codesandbox.io/s/computed-3cjo9?file=/src/index.tsx)) or by creating a no-op `autorun(() => { someObject.someComputed })`, which can be nicely cleaned up later if needed.
-Note that both solutions have the risk of creating memory leaks. Changing the default behavior here is an anti-pattern.
+You can override this per computed with `keepAlive: true`, or globally with `configure({ globalKeepAliveState: true })`.
+Keeping computeds alive can increase memory retention, but avoids extra recomputation on untracked reads.
 
 MobX can also be configured with the [`computedRequiresReaction`](configuration.md#computedrequiresreaction-boolean) option, to report an error when computeds are accessed outside of a reactive context.
 
@@ -237,4 +231,7 @@ It is recommended to set this one to `true` on very expensive computed values. I
 
 ### `keepAlive`
 
-This avoids suspending computed values when they are not being observed by anything (see the above explanation). Can potentially create memory leaks, similar to the ones discussed for [reactions](reactions.md#always-dispose-of-reactions).
+`keepAlive` controls whether a computed gets suspended when it has no observers.
+Explicit `keepAlive` takes precedence.
+If omitted, MobX uses `configure({ globalKeepAliveState })`, which currently defaults to `false` to preserve existing behavior.
+In the next major version, `globalKeepAliveState` will default to `true`

docs/configuration.md

@@ -96,9 +96,9 @@ In the rare case where you create observables lazily, for example in a computed
 #### `computedRequiresReaction: boolean`
 
 Forbids the direct access of any unobserved computed value from outside an action or reaction.
-This guarantees you aren't using computed values in a way where MobX won't cache them. **Default: `false`**.
+Use this to enforce that computeds are only read in a reactive context. **Default: `false`**.
 
-In the following example, MobX won't cache the computed value in the first code block, but will cache the result in the second and third block:
+In the following example, the first block will trigger a warning when this option is enabled:
 
 ```javascript
 class Clock {
@@ -116,7 +116,7 @@ class Clock {
 
 const clock = new Clock()
 {
-    // This would compute twice, but is warned against by this flag.
+    // This is warned against by this flag.
     console.log(clock.milliseconds)
     console.log(clock.milliseconds)
 }
@@ -136,6 +136,15 @@ const clock = new Clock()
 }
 ```
 
+#### `globalKeepAliveState: boolean`
+
+Overrides the default `keepAlive` behavior for computed's that do not set `keepAlive` explicitly.
+Explicit `keepAlive` takes precedence. `globalKeepAliveState` currently defaults to `false` to preserve existing behavior. In next major, the flag will default to `true`
+
+```javascript
+configure({ globalKeepAliveState: false })
+```
+
 #### `observableRequiresReaction: boolean`
 
 Warns about any unobserved observable access.

packages/mobx/__tests__/decorators_20223/stage3-decorators.ts

@@ -962,7 +962,7 @@ test("unobserved computed reads should warn with requiresReaction enabled", () =
         class A {
             @observable accessor x = 0
 
-            @computed({ requiresReaction: true })
+            @computed({ requiresReaction: true, keepAlive: false })
             get y() {
                 return this.x * 2
             }

packages/mobx/__tests__/v5/base/observables.js

@@ -55,6 +55,78 @@ test("basic", function () {
     expect(mobx._isComputingDerivation()).toBe(false)
 })
 
+describe("computed keepAlive: migration flag", () => {
+    afterEach(() => {
+        mobx.configure({ globalKeepAliveState: false })
+        mobx._resetGlobalState()
+    })
+
+    test("warns once when implicit `keepAlive`", () => {
+        const consoleWarnSpy = jest.spyOn(console, "warn").mockImplementation(() => {})
+        try {
+            const box = observable.box(1)
+            mobx.computed(() => box.get() * 2)
+            mobx.computed(() => box.get() * 3)
+
+            const migrationWarnings = consoleWarnSpy.mock.calls.filter(([msg]) =>
+                msg.includes("Computed created without explicit `keepAlive`")
+            )
+            expect(migrationWarnings).toHaveLength(1)
+        } finally {
+            consoleWarnSpy.mockRestore()
+        }
+    })
+
+    test("implicit `keepAlive` defaults to global flag", () => {
+        const consoleWarnSpy = jest.spyOn(console, "warn").mockImplementation(() => {})
+        try {
+            mobx.configure({ globalKeepAliveState: false })
+            let computations = 0
+            const box = observable.box(1)
+            const computed = mobx.computed(() => {
+                computations++
+                return box.get() * 2
+            })
+
+            expect(computed.get()).toBe(2)
+            expect(computed.get()).toBe(2)
+            expect(computations).toBe(2)
+        } finally {
+            consoleWarnSpy.mockRestore()
+        }
+    })
+
+    test("explicit `keepAlive` overrides global flag", () => {
+        mobx.configure({ globalKeepAliveState: false })
+        let computationsWhenTrue = 0
+        const a = observable.box(1)
+        const keepAliveTrue = mobx.computed(
+            () => {
+                computationsWhenTrue++
+                return a.get() * 2
+            },
+            { keepAlive: true }
+        )
+        expect(keepAliveTrue.get()).toBe(2)
+        expect(keepAliveTrue.get()).toBe(2)
+        expect(computationsWhenTrue).toBe(1)
+
+        mobx.configure({ globalKeepAliveState: true })
+        let computationsWhenFalse = 0
+        const b = observable.box(1)
+        const keepAliveFalse = mobx.computed(
+            () => {
+                computationsWhenFalse++
+                return b.get() * 2
+            },
+            { keepAlive: false }
+        )
+        expect(keepAliveFalse.get()).toBe(2)
+        expect(keepAliveFalse.get()).toBe(2)
+        expect(computationsWhenFalse).toBe(2)
+    })
+})
+
 test("basic2", function () {
     const x = observable.box(3)
     const z = computed(function () {
@@ -2307,21 +2379,21 @@ describe("`requiresReaction` takes precedence over global `computedRequiresReact
 
     test("`undefined`", () => {
         mobx.configure({ computedRequiresReaction: true })
-        const c = mobx.computed(() => {}, { name })
+        const c = mobx.computed(() => {}, { name, keepAlive: false })
         c.get()
         expect(consoleWarnSpy).toHaveBeenLastCalledWith(warnMsg)
     })
 
     test("`true` over `false`", () => {
         mobx.configure({ computedRequiresReaction: false })
-        const c = mobx.computed(() => {}, { name, requiresReaction: true })
+        const c = mobx.computed(() => {}, { name, requiresReaction: true, keepAlive: false })
         c.get()
         expect(consoleWarnSpy).toHaveBeenLastCalledWith(warnMsg)
     })
 
     test("`false` over `true`", () => {
         mobx.configure({ computedRequiresReaction: true })
-        const c = mobx.computed(() => {}, { name, requiresReaction: false })
+        const c = mobx.computed(() => {}, { name, requiresReaction: false, keepAlive: false })
         c.get()
         expect(consoleWarnSpy).not.toHaveBeenCalled()
     })
@@ -2396,7 +2468,7 @@ test('Observables initialization does not violate `enforceActions: "always"`', (
         check(() => mobx.observable({ x: 0 }, { proxy: true }))
         check(() => mobx.observable([0], { proxy: false }))
         check(() => mobx.observable([0], { proxy: true }))
-        check(() => mobx.computed(() => 0))
+        check(() => mobx.computed(() => 0, { keepAlive: false }))
     } finally {
         consoleWarnSpy.mockRestore()
         mobx._resetGlobalState()
@@ -2467,7 +2539,7 @@ test("state version does not update on observable creation", () => {
     check(() => mobx.observable({ x: 0 }, { proxy: true }))
     check(() => mobx.observable([0], { proxy: false }))
     check(() => mobx.observable([0], { proxy: true }))
-    check(() => mobx.computed(() => 0))
+    check(() => mobx.computed(() => 0, { keepAlive: false }))
 })
 
 test("#3747", () => {

packages/mobx/__tests__/v5/base/typescript-decorators.ts

@@ -1110,7 +1110,7 @@ test("unobserved computed reads should warn with requiresReaction enabled", () =
         class A {
             @observable x = 0
 
-            @computed({ requiresReaction: true })
+            @computed({ requiresReaction: true, keepAlive: false })
             get y() {
                 return this.x * 2
             }

packages/mobx/__tests__/v5/base/typescript-tests.ts

@@ -1672,7 +1672,7 @@ test("unobserved computed reads should warn with requiresReaction enabled", () =
                     this,
                     {
                         x: observable,
-                        y: computed({ requiresReaction: true })
+                        y: computed({ requiresReaction: true, keepAlive: false })
                     },
                     { name: "a" }
                 )

packages/mobx/src/api/configure.ts

@@ -16,6 +16,7 @@ export function configure(options: {
      * Warn if observables are accessed outside a reactive context
      */
     observableRequiresReaction?: boolean
+    globalKeepAliveState?: boolean
     isolateGlobalState?: boolean
     disableErrorBoundaries?: boolean
     safeDescriptors?: boolean
@@ -53,6 +54,9 @@ export function configure(options: {
             globalState[key] = !!options[key]
         }
     })
+    if (options.globalKeepAliveState !== undefined) {
+        globalState.globalKeepAliveState = !!options.globalKeepAliveState
+    }
     globalState.allowStateReads = !globalState.observableRequiresReaction
     if (__DEV__ && globalState.disableErrorBoundaries === true) {
         console.warn(

packages/mobx/src/core/computedvalue.ts

@@ -16,6 +16,7 @@ import {
     globalState,
     isCaughtException,
     isSpyEnabled,
+    once,
     propagateChangeConfirmed,
     propagateMaybeChanged,
     reportObserved,
@@ -34,6 +35,12 @@ import {
 
 import { getFlag, setFlag } from "../utils/utils"
 
+const implicitKeepAliveDefaultWarning = once(() =>
+    console.warn(
+        "[mobx] Computed created without explicit `keepAlive`. It will use `configure({ globalKeepAliveState })` if set, otherwise the current default (`false`, changing to `true` in the next major version). Set `keepAlive` explicitly to avoid relying on global / default behavior."
+    )
+)
+
 export interface IComputedValue<T> {
     get(): T
     set(value: T): void
@@ -136,7 +143,10 @@ export class ComputedValue<T> implements IObservable, IComputedValue<T>, IDeriva
                 : comparer.default)
         this.scope_ = options.context
         this.requiresReaction_ = options.requiresReaction
-        this.keepAlive_ = !!options.keepAlive
+        if (__DEV__ && options.keepAlive === undefined) {
+            implicitKeepAliveDefaultWarning()
+        }
+        this.keepAlive_ = options.keepAlive ?? globalState.globalKeepAliveState
     }
 
     onBecomeStale_() {
@@ -206,14 +216,14 @@ export class ComputedValue<T> implements IObservable, IComputedValue<T>, IDeriva
         if (this.isComputing) {
             die(32, this.name_, this.derivation)
         }
-        if (
-            globalState.inBatch === 0 &&
-            // !globalState.trackingDerivatpion &&
-            this.observers_.size === 0 &&
-            !this.keepAlive_
-        ) {
+        const isUntrackedRead = globalState.inBatch === 0 && this.observers_.size === 0
+
+        if (isUntrackedRead && !globalState.trackingDerivation) {
+            this.warnAboutUntrackedRead_()
+        }
+
+        if (isUntrackedRead && !this.keepAlive_) {
             if (shouldCompute(this)) {
-                this.warnAboutUntrackedRead_()
                 startBatch() // See perf test 'computed memoization'
                 this.value_ = this.computeValue_(false)
                 endBatch()
@@ -348,19 +358,20 @@ export class ComputedValue<T> implements IObservable, IComputedValue<T>, IDeriva
         if (!__DEV__) {
             return
         }
+        let untrackedReadWarning = `Computed value '${this.name_}' is being read outside a reactive context.`
+        if (!this.keepAlive_) {
+            untrackedReadWarning += " Doing a full recompute."
+        }
+
         if (this.isTracing_ !== TraceMode.NONE) {
-            console.log(
-                `[mobx.trace] Computed value '${this.name_}' is being read outside a reactive context. Doing a full recompute.`
-            )
+            console.log(`[mobx.trace] ${untrackedReadWarning}`)
         }
         if (
             typeof this.requiresReaction_ === "boolean"
                 ? this.requiresReaction_
                 : globalState.computedRequiresReaction
         ) {
-            console.warn(
-                `[mobx] Computed value '${this.name_}' is being read outside a reactive context. Doing a full recompute.`
-            )
+            console.warn(`[mobx] ${untrackedReadWarning}`)
         }
     }
 

packages/mobx/src/core/globalstate.ts

@@ -9,6 +9,7 @@ const persistentKeys: (keyof MobXGlobals)[] = [
     "spyListeners",
     "enforceActions",
     "computedRequiresReaction",
+    "globalKeepAliveState",
     "reactionRequiresObservable",
     "observableRequiresReaction",
     "allowStateReads",
@@ -114,6 +115,12 @@ export class MobXGlobals {
      */
     computedRequiresReaction = false
 
+    /**
+     * Global flag for computed keepAlive behavior.
+     * Defaults to `false` to preserve current behavior.
+     */
+    globalKeepAliveState = false
+
     /**
      * (Experimental)
      * Warn if you try to create to derivation / reactive context without accessing any observable.