Review feedback

src/api/configure.ts

@@ -71,6 +71,8 @@ export function configure(options: {
     }
     if (observableRequiresReaction !== undefined) {
         globalState.observableRequiresReaction = !!observableRequiresReaction
+
+        globalState.allowStateReads = !globalState.observableRequiresReaction
     }
     if (computedConfigurable !== undefined) {
         globalState.computedConfigurable = !!computedConfigurable

src/api/transaction.ts

@@ -1,4 +1,5 @@
 import { endBatch, startBatch } from "../internal"
+import { allowStateReadsStart, allowStateReadsEnd } from "../core/derivation"
 
 /**
  * During a transaction no views are updated until the end of the transaction.
@@ -8,10 +9,12 @@ import { endBatch, startBatch } from "../internal"
  * @returns any value that was returned by the 'action' parameter.
  */
 export function transaction<T>(action: () => T, thisArg = undefined): T {
+    const prevAllowStateReads = allowStateReadsStart(true)
     startBatch()
     try {
         return action.apply(thisArg)
     } finally {
         endBatch()
+        allowStateReadsEnd(prevAllowStateReads)
     }
 }

src/core/action.ts

@@ -11,6 +11,7 @@ import {
     untrackedEnd,
     untrackedStart
 } from "../internal"
+import { allowStateReadsStart, allowStateReadsEnd } from "./derivation"
 
 export interface IAction {
     isMobxAction: boolean
@@ -50,6 +51,7 @@ export function executeAction(actionName: string, fn: Function, scope?: any, arg
 interface IActionRunInfo {
     prevDerivation: IDerivation | null
     prevAllowStateChanges: boolean
+    prevAllowStateReads: boolean
     notifySpy: boolean
     startTime: number
 }
@@ -77,16 +79,19 @@ function startAction(
     const prevDerivation = untrackedStart()
     startBatch()
     const prevAllowStateChanges = allowStateChangesStart(true)
+    const prevAllowStateReads = allowStateReadsStart(true)
     return {
         prevDerivation,
         prevAllowStateChanges,
+        prevAllowStateReads,
         notifySpy,
         startTime
     }
 }
 
 function endAction(runInfo: IActionRunInfo) {
     allowStateChangesEnd(runInfo.prevAllowStateChanges)
+    allowStateReadsEnd(runInfo.prevAllowStateReads)
     endBatch()
     untrackedEnd(runInfo.prevDerivation)
     if (runInfo.notifySpy && process.env.NODE_ENV !== "production")

src/core/derivation.ts

@@ -160,12 +160,23 @@ export function checkIfStateModificationsAreAllowed(atom: IAtom) {
         )
 }
 
+export function checkIfStateReadsAreAllowed(observable: IObservable) {
+    if (
+        process.env.NODE_ENV !== "production" &&
+        !globalState.allowStateReads &&
+        globalState.observableRequiresReaction
+    ) {
+        console.warn(`[mobx] Observable ${observable.name} being read outside a reactive context`)
+    }
+}
+
 /**
  * Executes the provided function `f` and tracks which observables are being accessed.
  * The tracking information is stored on the `derivation` object and the derivation is registered
  * as observer of any of the accessed observables.
  */
 export function trackDerivedFunction<T>(derivation: IDerivation, f: () => T, context: any) {
+    const prevAllowStateReads = allowStateReadsStart(true)
     // pre allocate array allocation + room for variation in deps
     // array will be trimmed by bindDependencies
     changeDependenciesStateTo0(derivation)
@@ -191,6 +202,8 @@ export function trackDerivedFunction<T>(derivation: IDerivation, f: () => T, con
         warnAboutDerivationWithoutDependencies(derivation)
     }
 
+    allowStateReadsEnd(prevAllowStateReads)
+
     return result
 }
 
@@ -281,11 +294,13 @@ export function clearObserving(derivation: IDerivation) {
 }
 
 export function untracked<T>(action: () => T): T {
+    const prevAllowStateReads = allowStateReadsStart(false)
     const prev = untrackedStart()
     try {
         return action()
     } finally {
         untrackedEnd(prev)
+        allowStateReadsEnd(prevAllowStateReads)
     }
 }
 
@@ -299,6 +314,16 @@ export function untrackedEnd(prev: IDerivation | null) {
     globalState.trackingDerivation = prev
 }
 
+export function allowStateReadsStart(allowStateReads: boolean) {
+    const prev = globalState.allowStateReads
+    globalState.allowStateReads = allowStateReads
+    return prev
+}
+
+export function allowStateReadsEnd(prev: boolean) {
+    globalState.allowStateReads = prev
+}
+
 /**
  * needed to keep `lowestObserverState` correct. when changing from (2 or 1) to 0
  *

src/core/globalstate.ts

@@ -10,6 +10,7 @@ const persistentKeys: (keyof MobXGlobals)[] = [
     "computedRequiresReaction",
     "reactionRequiresObservable",
     "observableRequiresReaction",
+    "allowStateReads",
     "disableErrorBoundaries",
     "runId",
     "UNCHANGED"
@@ -83,6 +84,12 @@ export class MobXGlobals {
      */
     allowStateChanges = true
 
+    /**
+     * Is it allowed to read observables at this point?
+     * Used to hold the state needed for `observableRequiresReaction`
+     */
+    allowStateReads = true
+
     /**
      * If strict mode is enabled, state changes are by default not allowed
      */

src/core/observable.ts

@@ -7,7 +7,8 @@ import {
     TraceMode,
     getDependencyTree,
     globalState,
-    runReactions
+    runReactions,
+    checkIfStateReadsAreAllowed
 } from "../internal"
 
 export interface IDepTreeNode {
@@ -131,6 +132,8 @@ export function endBatch() {
 }
 
 export function reportObserved(observable: IObservable): boolean {
+    checkIfStateReadsAreAllowed(observable)
+
     const derivation = globalState.trackingDerivation
     if (derivation !== null) {
         /**
@@ -148,13 +151,10 @@ export function reportObserved(observable: IObservable): boolean {
             }
         }
         return true
-    } else if (globalState.inBatch > 0) {
-        if (observable.observers.size === 0) {
-            queueForUnobservation(observable)
-        }
-    } else {
-        warnAboutUntrackedObservableRead(observable.name)
+    } else if (observable.observers.size === 0 && globalState.inBatch > 0) {
+        queueForUnobservation(observable)
     }
+
     return false
 }
 
@@ -270,10 +270,3 @@ function printDepTree(tree: IDependencyTree, lines: string[], depth: number) {
     lines.push(`${new Array(depth).join("\t")}${tree.name}`) // MWE: not the fastest, but the easiest way :)
     if (tree.dependencies) tree.dependencies.forEach(child => printDepTree(child, lines, depth + 1))
 }
-
-function warnAboutUntrackedObservableRead(name: string) {
-    if (process.env.NODE_ENV === "production") return
-    if (globalState.observableRequiresReaction) {
-        console.warn(`[mobx] Observable ${name} being read outside a reactive context`)
-    }
-}

test/base/strict-mode.js

@@ -291,7 +291,7 @@ describe("observableRequiresReaction", function() {
             mobx.configure({ observableRequiresReaction: false })
         }
     })
-    test("don't warn on reads inside batch - computed", function() {
+    test("don't warn on reads inside a computed", function() {
         try {
             mobx.configure({ observableRequiresReaction: true })
             const x = mobx.observable({
@@ -311,7 +311,7 @@ describe("observableRequiresReaction", function() {
         }
     })
 
-    test("don't warn on reads inside batch - action", function() {
+    test("don't warn on reads inside an action", function() {
         try {
             mobx.configure({ observableRequiresReaction: true })
             const x = mobx.observable({
@@ -330,7 +330,7 @@ describe("observableRequiresReaction", function() {
         }
     })
 
-    test("don't warn on reads inside batch - transaction", function() {
+    test("don't warn on reads inside a transaction", function() {
         try {
             mobx.configure({ observableRequiresReaction: true })
             const x = mobx.observable({
@@ -347,7 +347,7 @@ describe("observableRequiresReaction", function() {
         }
     })
 
-    test("don't warn on reads inside batch - untracked inside autorun", function() {
+    test("warn on reads inside untracked", function() {
         try {
             mobx.configure({ observableRequiresReaction: true })
             const x = mobx.observable({
@@ -362,7 +362,7 @@ describe("observableRequiresReaction", function() {
                 disposer()
             })
 
-            expect(messages.length).toBe(0)
+            expect(messages.length).toBe(1)
         } finally {
             mobx.configure({ observableRequiresReaction: false })
         }