Review feedback

CHANGELOG.md

@@ -1,6 +1,7 @@
 # 5.14.0 / 4.14.0
 
-* Added `derivationRequiresObservable` & `observablesRequiresReaction` config [#2079](https://github.com/mobxjs/mobx/pull/2079), [Docs](https://github.com/mobxjs/mobx/pull/2082)
+* Added experimental `reactionRequiresObservable` & `observableRequiresReaction` config [#2079](https://github.com/mobxjs/mobx/pull/2079), [Docs](https://github.com/mobxjs/mobx/pull/2082)
+* Added experimental `requiresObservable` config to `reaction` & `autorun` options [#2079](https://github.com/mobxjs/mobx/pull/2079), [Docs](https://github.com/mobxjs/mobx/pull/2082)
 
 # 5.13.1 / 4.13.1 (TO BE RELEASED)
 

flow-typed/mobx.js

@@ -5,8 +5,8 @@ export type IObservableMapInitialValues<K, V> = IMapEntries<K, V> | KeyValueMap<
 export interface IMobxConfigurationOptions {
     +enforceActions?: boolean | "strict" | "never" | "always" | "observed";
     computedRequiresReaction?: boolean;
-    derivationRequiresObservable?: boolean;
-    observablesRequiresReaction?: boolean;
+    reactionRequiresObservable?: boolean;
+    observableRequiresReaction?: boolean;
     isolateGlobalState?: boolean;
     disableErrorBoundaries?: boolean;
     arrayBuffer?: number;
@@ -18,6 +18,10 @@ declare export function configure(options: IMobxConfigurationOptions): void
 export interface IAutorunOptions {
     delay?: number;
     name?: string;
+    /**
+     *  warn if the derivation has no dependencies after creation/update
+     */
+    requiresObservable?: boolean;
     scheduler?: (callback: () => void) => any;
     onError?: (error: any) => void;
 }

src/api/autorun.ts

@@ -15,6 +15,11 @@ import {
 export interface IAutorunOptions {
     delay?: number
     name?: string
+    /**
+     * Experimental.
+     * Warns if the view doesn't track observables
+     */
+    requiresObservable?: boolean
     scheduler?: (callback: () => void) => any
     onError?: (error: any) => void
 }
@@ -48,7 +53,8 @@ export function autorun(
             function(this: Reaction) {
                 this.track(reactionRunner)
             },
-            opts.onError
+            opts.onError,
+            opts.requiresObservable
         )
     } else {
         const scheduler = createSchedulerFromOptions(opts)
@@ -66,7 +72,8 @@ export function autorun(
                     })
                 }
             },
-            opts.onError
+            opts.onError,
+            opts.requiresObservable
         )
     }
 
@@ -89,8 +96,8 @@ function createSchedulerFromOptions(opts: IReactionOptions) {
     return opts.scheduler
         ? opts.scheduler
         : opts.delay
-            ? (f: Lambda) => setTimeout(f, opts.delay!)
-            : run
+        ? (f: Lambda) => setTimeout(f, opts.delay!)
+        : run
 }
 
 export function reaction<T>(
@@ -131,7 +138,8 @@ export function reaction<T>(
                 scheduler!(reactionRunner)
             }
         },
-        opts.onError
+        opts.onError,
+        opts.requiresObservable
     )
 
     function reactionRunner() {

src/api/configure.ts

@@ -9,8 +9,16 @@ import {
 export function configure(options: {
     enforceActions?: boolean | "strict" | "never" | "always" | "observed"
     computedRequiresReaction?: boolean
-    derivationRequiresObservable?: boolean
-    observablesRequiresReaction?: boolean
+    /**
+     * (Experimental)
+     * Warn if you try to create to derivation / reactive context without accessing any observable.
+     */
+    reactionRequiresObservable?: boolean
+    /**
+     * (Experimental)
+     * Warn if observables are accessed outside a reactive context
+     */
+    observableRequiresReaction?: boolean
     computedConfigurable?: boolean
     isolateGlobalState?: boolean
     disableErrorBoundaries?: boolean
@@ -22,8 +30,8 @@ export function configure(options: {
         computedConfigurable,
         disableErrorBoundaries,
         reactionScheduler,
-        derivationRequiresObservable,
-        observablesRequiresReaction
+        reactionRequiresObservable,
+        observableRequiresReaction
     } = options
     if (options.isolateGlobalState === true) {
         isolateGlobalState()
@@ -58,11 +66,11 @@ export function configure(options: {
     if (computedRequiresReaction !== undefined) {
         globalState.computedRequiresReaction = !!computedRequiresReaction
     }
-    if (derivationRequiresObservable !== undefined) {
-        globalState.derivationRequiresObservable = !!derivationRequiresObservable
+    if (reactionRequiresObservable !== undefined) {
+        globalState.reactionRequiresObservable = !!reactionRequiresObservable
     }
-    if (observablesRequiresReaction !== undefined) {
-        globalState.observablesRequiresReaction = !!observablesRequiresReaction
+    if (observableRequiresReaction !== undefined) {
+        globalState.observableRequiresReaction = !!observableRequiresReaction
     }
     if (computedConfigurable !== undefined) {
         globalState.computedConfigurable = !!computedConfigurable

src/core/derivation.ts

@@ -55,6 +55,11 @@ export interface IDerivation extends IDepTreeNode {
     __mapid: string
     onBecomeStale(): void
     isTracing: TraceMode
+
+    /**
+     *  warn if the derivation has no dependencies after creation/update
+     */
+    requiresObservable?: boolean
 }
 
 export class CaughtException {
@@ -183,16 +188,20 @@ export function trackDerivedFunction<T>(derivation: IDerivation, f: () => T, con
     bindDependencies(derivation)
 
     if (derivation.observing.length === 0) {
-        warnAboutDerivationWithoutDependencies(derivation.name)
+        warnAboutDerivationWithoutDependencies(derivation)
     }
 
     return result
 }
 
-function warnAboutDerivationWithoutDependencies(name: string) {
+function warnAboutDerivationWithoutDependencies(derivation: IDerivation) {
     if (process.env.NODE_ENV === "production") return
-    if (globalState.derivationRequiresObservable) {
-        console.warn(`[mobx] Derivation ${name} is created without reading any observable value`)
+    if (globalState.reactionRequiresObservable || derivation.requiresObservable) {
+        console.warn(
+            `[mobx] Derivation ${
+                derivation.name
+            } is created/updated without reading any observable value`
+        )
     }
 }
 

src/core/globalstate.ts

@@ -8,6 +8,8 @@ const persistentKeys: (keyof MobXGlobals)[] = [
     "spyListeners",
     "enforceActions",
     "computedRequiresReaction",
+    "reactionRequiresObservable",
+    "observableRequiresReaction",
     "disableErrorBoundaries",
     "runId",
     "UNCHANGED"
@@ -102,14 +104,16 @@ export class MobXGlobals {
     computedRequiresReaction = false
 
     /**
+     * (Experimental)
      * Warn if you try to create to derivation / reactive context without accessing any observable.
      */
-    derivationRequiresObservable = false
+    reactionRequiresObservable = false
 
     /**
+     * (Experimental)
      * Warn if observables are accessed outside a reactive context
      */
-    observablesRequiresReaction = false
+    observableRequiresReaction = false
 
     /**
      * Allows overwriting of computed properties, useful in tests but not prod as it can cause

src/core/observable.ts

@@ -148,10 +148,11 @@ export function reportObserved(observable: IObservable): boolean {
             }
         }
         return true
-    } else if (observable.observers.size === 0) {
-        if (globalState.inBatch > 0) {
+    } else if (globalState.inBatch > 0) {
+        if (observable.observers.size === 0) {
             queueForUnobservation(observable)
         }
+    } else {
         warnAboutUntrackedObservableRead(observable.name)
     }
     return false
@@ -272,7 +273,7 @@ function printDepTree(tree: IDependencyTree, lines: string[], depth: number) {
 
 function warnAboutUntrackedObservableRead(name: string) {
     if (process.env.NODE_ENV === "production") return
-    if (globalState.observablesRequiresReaction) {
+    if (globalState.observableRequiresReaction) {
         console.warn(`[mobx] Observable ${name} being read outside a reactive context`)
     }
 }

src/core/reaction.ts

@@ -67,7 +67,8 @@ export class Reaction implements IDerivation, IReactionPublic {
     constructor(
         public name: string = "Reaction@" + getNextId(),
         private onInvalidate: () => void,
-        private errorHandler?: (error: any, derivation: IDerivation) => void
+        private errorHandler?: (error: any, derivation: IDerivation) => void,
+        public requiresObservable = false
     ) {}
 
     onBecomeStale() {

test/base/autorun.js

@@ -1,10 +1,14 @@
-const m = require("../../src/mobx.ts")
+/**
+ * @type {typeof import("./../../src/mobx")}
+ */
+const mobx = require("../../src/mobx.ts")
+const utils = require("../utils/test-utils")
 
 test("autorun passes Reaction as an argument to view function", function() {
-    const a = m.observable.box(1)
+    const a = mobx.observable.box(1)
     const values = []
 
-    m.autorun(r => {
+    mobx.autorun(r => {
         expect(typeof r.dispose).toBe("function")
         if (a.get() === "pleaseDispose") r.dispose()
         values.push(a.get())
@@ -20,10 +24,10 @@ test("autorun passes Reaction as an argument to view function", function() {
 })
 
 test("autorun can be disposed on first run", function() {
-    const a = m.observable.box(1)
+    const a = mobx.observable.box(1)
     const values = []
 
-    m.autorun(r => {
+    mobx.autorun(r => {
         r.dispose()
         values.push(a.get())
     })
@@ -34,17 +38,17 @@ test("autorun can be disposed on first run", function() {
 })
 
 test("autorun warns when passed an action", function() {
-    const action = m.action(() => {})
+    const action = mobx.action(() => {})
     expect.assertions(1)
-    expect(() => m.autorun(action)).toThrowError(/Autorun does not accept actions/)
+    expect(() => mobx.autorun(action)).toThrowError(/Autorun does not accept actions/)
 })
 
 test("autorun batches automatically", function() {
     let runs = 0
     let a1runs = 0
     let a2runs = 0
 
-    const x = m.observable({
+    const x = mobx.observable({
         a: 1,
         b: 1,
         c: 1,
@@ -54,12 +58,12 @@ test("autorun batches automatically", function() {
         }
     })
 
-    const d1 = m.autorun(() => {
+    const d1 = mobx.autorun(() => {
         a1runs++
         x.d // read
     })
 
-    const d2 = m.autorun(() => {
+    const d2 = mobx.autorun(() => {
         a2runs++
         x.b = x.a
         x.c = x.a
@@ -80,12 +84,12 @@ test("autorun batches automatically", function() {
 })
 
 test("autorun tracks invalidation of unbound dependencies", function() {
-    const a = m.observable.box(0)
-    const b = m.observable.box(0)
-    const c = m.computed(() => a.get() + b.get())
+    const a = mobx.observable.box(0)
+    const b = mobx.observable.box(0)
+    const c = mobx.computed(() => a.get() + b.get())
     const values = []
 
-    m.autorun(() => {
+    mobx.autorun(() => {
         values.push(c.get())
         b.set(100)
     })
@@ -95,21 +99,49 @@ test("autorun tracks invalidation of unbound dependencies", function() {
 })
 
 test("when effect is an action", function(done) {
-    const a = m.observable.box(0)
+    const a = mobx.observable.box(0)
 
-    m.configure({ enforceActions: "observed" })
-    m.when(
+    mobx.configure({ enforceActions: "observed" })
+    mobx.when(
         () => a.get() === 1,
         () => {
             a.set(2)
 
-            m.configure({ enforceActions: "never" })
+            mobx.configure({ enforceActions: "never" })
             done()
         },
         { timeout: 1 }
     )
 
-    m.runInAction(() => {
+    mobx.runInAction(() => {
         a.set(1)
     })
 })
+
+describe("autorun opts requiresObservable", () => {
+    test("warn when no observable", () => {
+        utils.consoleWarn(() => {
+            const disposer = mobx.autorun(() => 2, {
+                requiresObservable: true
+            })
+
+            disposer()
+        }, /is created\/updated without reading any observable value/)
+    })
+
+    test("Don't warn when observable", () => {
+        const obsr = mobx.observable({
+            x: 1
+        })
+
+        const messages = utils.supressConsole(() => {
+            const disposer = mobx.autorun(() => obsr.x, {
+                requiresObservable: true
+            })
+
+            disposer()
+        })
+
+        expect(messages.length).toBe(0)
+    })
+})