Fix #3648: observableRequiresReaction/computedRequiresReaction shouldn't warn with enableStaticRendering(true)

.changeset/fluffy-hairs-study.md

@@ -0,0 +1,6 @@
+---
+"mobx-react": patch
+"mobx-react-lite": patch
+---
+
+fix #3648: `observableRequiresReaction`/`computedRequiresReaction` shouldn't warn with `enableStaticRendering(true)`

.changeset/shaggy-carpets-move.md

@@ -0,0 +1,5 @@
+---
+"mobx": patch
+---
+
+`computedRequiresReaction` respects `globalState.allowStateReads`

packages/mobx-react-lite/__tests__/__snapshots__/observer.test.tsx.snap

@@ -16,6 +16,8 @@ exports[`changing state in render should fail 2`] = `
 </div>
 `;
 
+exports[`#3648 enableStaticRendering doesn't warn with observableRequiresReaction/computedRequiresReaction 1`] = `[MockFunction]`;
+
 exports[`issue 12 init state is correct 1`] = `
 <div>
   <div>

packages/mobx-react-lite/__tests__/observer.test.tsx

@@ -1088,6 +1088,27 @@ test("Anonymous component displayName #3192", () => {
     consoleErrorSpy.mockRestore()
 })
 
+test("#3648 enableStaticRendering doesn't warn with observableRequiresReaction/computedRequiresReaction", () => {
+    consoleWarnMock = jest.spyOn(console, "warn").mockImplementation(() => {})
+    try {
+        enableStaticRendering(true)
+        mobx.configure({ observableRequiresReaction: true, computedRequiresReaction: true })
+        const o = mobx.observable.box(0, { name: "o" })
+        const c = mobx.computed(() => o.get(), { name: "c" })
+        const TestCmp = observer(() => <div>{o.get() + c.get()}</div>)
+
+        const { unmount, container } = render(<TestCmp />)
+        expect(container).toHaveTextContent("0")
+        unmount()
+
+        expect(consoleWarnMock).toMatchSnapshot()
+    } finally {
+        enableStaticRendering(false)
+        mobx._resetGlobalState()
+        consoleWarnMock.mockRestore()
+    }
+})
+
 test("StrictMode #3671", async () => {
     const o = mobx.observable({ x: 0 })
 

packages/mobx-react-lite/src/observer.ts

@@ -1,6 +1,4 @@
 import { forwardRef, memo } from "react"
-
-import { isUsingStaticRendering } from "./staticRendering"
 import { useObserver } from "./useObserver"
 
 let warnObserverOptionsDeprecated = true
@@ -79,10 +77,6 @@ export function observer<P extends object, TRef = {}>(
     }
 
     // The working of observer is explained step by step in this talk: https://www.youtube.com/watch?v=cPF4iBedoF0&feature=youtu.be&t=1307
-    if (isUsingStaticRendering()) {
-        return baseComponent
-    }
-
     let useForwardRef = options?.forwardRef ?? false
     let render = baseComponent
 

packages/mobx-react-lite/src/useObserver.ts

@@ -1,4 +1,4 @@
-import { Reaction } from "mobx"
+import { Reaction, _allowStateReadsEnd, _allowStateReadsStart } from "mobx"
 import React from "react"
 import { printDebugValue } from "./utils/printDebugValue"
 import { isUsingStaticRendering } from "./staticRendering"
@@ -36,7 +36,12 @@ function createReaction(adm: ObserverAdministration) {
 
 export function useObserver<T>(render: () => T, baseComponentName: string = "observed"): T {
     if (isUsingStaticRendering()) {
-        return render()
+        let allowStateReads = _allowStateReadsStart?.(true)
+        try {
+            return render()
+        } finally {
+            _allowStateReadsEnd?.(allowStateReads)
+        }
     }
 
     const admRef = React.useRef<ObserverAdministration | null>(null)

packages/mobx-react/__tests__/__snapshots__/observer.test.tsx.snap

@@ -2,6 +2,8 @@
 
 exports[`#3492 should not cause warning by calling forceUpdate on uncommited components 1`] = `[MockFunction]`;
 
+exports[`#3648 enableStaticRendering doesn't warn with observableRequiresReaction/computedRequiresReaction 1`] = `[MockFunction]`;
+
 exports[`Redeclaring an existing observer component as an observer should throw 1`] = `"The provided component class (AlreadyObserver) has already been declared as an observer component."`;
 
 exports[`SSR works #3448 1`] = `[MockFunction]`;

packages/mobx-react/__tests__/observer.test.tsx

@@ -13,7 +13,8 @@ import {
     autorun,
     IReactionDisposer,
     reaction,
-    configure
+    configure,
+    runInAction
 } from "mobx"
 import { withConsole } from "./utils/withConsole"
 import { shallowEqual } from "../src/utils/utils"
@@ -295,27 +296,6 @@ test("issue 12", () => {
     expect(events).toEqual(["table", "row: coffee", "row: tea", "table", "row: soup"])
 })
 
-test("changing state in render should fail", () => {
-    const data = observable.box(2)
-    const Comp = observer(() => {
-        if (data.get() === 3) {
-            try {
-                data.set(4) // wouldn't throw first time for lack of observers.. (could we tighten this?)
-            } catch (err) {
-                expect(err).toBeInstanceOf(Error)
-                expect(err).toMatch(
-                    /Side effects like changing state are not allowed at this point/
-                )
-            }
-        }
-        return <div>{data.get()}</div>
-    })
-    render(<Comp />)
-
-    act(() => data.set(3))
-    _resetGlobalState()
-})
-
 test("observer component can be injected", () => {
     const msg: Array<any> = []
     const baseWarn = console.warn
@@ -1011,6 +991,36 @@ test("#3492 should not cause warning by calling forceUpdate on uncommited compon
     unmount()
     expect(consoleWarnMock).toMatchSnapshot()
 })
+
+test("#3648 enableStaticRendering doesn't warn with observableRequiresReaction/computedRequiresReaction", () => {
+    consoleWarnMock = jest.spyOn(console, "warn").mockImplementation(() => {})
+    const { observableRequiresReaction, computedRequiresReaction } = _getGlobalState()
+    try {
+        enableStaticRendering(true)
+        configure({ observableRequiresReaction: true, computedRequiresReaction: true })
+        const o = observable.box(0, { name: "o" })
+        const c = computed(() => o.get(), { name: "c" })
+
+        @observer
+        class TestCmp extends React.Component<any> {
+            render() {
+                expect(this).toBeDefined()
+                return o.get() + c.get()
+            }
+        }
+
+        const { unmount, container } = render(<TestCmp />)
+        expect(container).toHaveTextContent("0")
+        unmount()
+
+        expect(consoleWarnMock).toMatchSnapshot()
+    } finally {
+        enableStaticRendering(false)
+        _resetGlobalState()
+        configure({ observableRequiresReaction, computedRequiresReaction })
+        consoleWarnMock.mockRestore()
+    }
+})
 ;["props", "state", "context"].forEach(key => {
     test(`using ${key} in computed throws`, () => {
         // React prints errors even if we catch em
@@ -1299,7 +1309,7 @@ test("Class observer can react to changes made before mount #3730", () => {
     @observer
     class Child extends React.Component {
         componentDidMount(): void {
-            o.set(1)
+            runInAction(() => o.set(1))
         }
         render() {
             return ""

packages/mobx-react/src/observerClass.ts

@@ -100,7 +100,7 @@ export function makeClassComponentObserver(
             configurable: false,
             writable: false,
             value: isUsingStaticRendering()
-                ? originalRender
+                ? createStaticRender.call(this, originalRender)
                 : createReactiveRender.call(this, originalRender)
         })
         return this.render()
@@ -178,6 +178,19 @@ function getDisplayName(componentClass: ComponentClass) {
     return componentClass.displayName || componentClass.name || "<component>"
 }
 
+function createStaticRender(originalRender: any) {
+    const boundOriginalRender = originalRender.bind(this)
+
+    return function staticRender() {
+        let allowStateReads = _allowStateReadsStart?.(true)
+        try {
+            return boundOriginalRender()
+        } finally {
+            _allowStateReadsEnd?.(allowStateReads)
+        }
+    }
+}
+
 function createReactiveRender(originalRender: any) {
     const boundOriginalRender = originalRender.bind(this)
 

packages/mobx/CHANGELOG.md

@@ -1355,7 +1355,7 @@ A deprecation message will now be printed if creating computed properties while
 
 ```javascript
 const x = observable({
-    computedProp: function() {
+    computedProp: function () {
         return someComputation
     }
 })
@@ -1380,7 +1380,7 @@ or alternatively:
 
 ```javascript
 observable({
-    computedProp: computed(function() {
+    computedProp: computed(function () {
         return someComputation
     })
 })
@@ -1398,7 +1398,7 @@ N.B. If you want to introduce actions on an observable that modify its state, us
 ```javascript
 observable({
     counter: 0,
-    increment: action(function() {
+    increment: action(function () {
         this.counter++
     })
 })
@@ -1524,10 +1524,10 @@ function Square() {
     extendObservable(this, {
         length: 2,
         squared: computed(
-            function() {
+            function () {
                 return this.squared * this.squared
             },
-            function(surfaceSize) {
+            function (surfaceSize) {
                 this.length = Math.sqrt(surfaceSize)
             }
         )

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

@@ -42,9 +42,12 @@ test("autorun can be disposed using AbortSignal", function () {
     const ac = new AbortController()
     const values = []
 
-    mobx.autorun(() => {
-        values.push(a.get())
-    }, { signal: ac.signal })
+    mobx.autorun(
+        () => {
+            values.push(a.get())
+        },
+        { signal: ac.signal }
+    )
 
     a.set(2)
     a.set(3)
@@ -61,9 +64,12 @@ test("autorun should not run first time when passing already aborted AbortSignal
 
     ac.abort()
 
-    mobx.autorun(() => {
-        values.push(a.get())
-    }, { signal: ac.signal })
+    mobx.autorun(
+        () => {
+            values.push(a.get())
+        },
+        { signal: ac.signal }
+    )
 
     expect(values).toEqual([])
 })

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

@@ -293,7 +293,7 @@ test("fireImmediately should not be honored when passed already aborted AbortSig
 
     reaction(
         () => a.get(),
-        (newValue) => {
+        newValue => {
             values.push(newValue)
         },
         { signal: ac.signal, fireImmediately: true }

packages/mobx/src/api/autorun.ts

@@ -90,7 +90,7 @@ export function autorun(
         view(reaction)
     }
 
-    if(!opts?.signal?.aborted) {
+    if (!opts?.signal?.aborted) {
         reaction.schedule_()
     }
     return reaction.getDisposer_(opts?.signal)
@@ -182,7 +182,7 @@ export function reaction<T, FireImmediately extends boolean = false>(
         firstTime = false
     }
 
-    if(!opts?.signal?.aborted) {
+    if (!opts?.signal?.aborted) {
         r.schedule_()
     }
     return r.getDisposer_(opts?.signal)

packages/mobx/src/api/configure.ts

@@ -53,7 +53,6 @@ export function configure(options: {
             globalState[key] = !!options[key]
         }
     })
-    globalState.allowStateReads = !globalState.observableRequiresReaction
     if (__DEV__ && globalState.disableErrorBoundaries === true) {
         console.warn(
             "WARNING: Debug feature only. MobX will NOT recover from errors when `disableErrorBoundaries` is enabled."

packages/mobx/src/core/computedvalue.ts

@@ -309,9 +309,10 @@ export class ComputedValue<T> implements IObservable, IComputedValue<T>, IDeriva
             )
         }
         if (
-            typeof this.requiresReaction_ === "boolean"
+            !globalState.allowStateReads &&
+            (typeof this.requiresReaction_ === "boolean"
                 ? this.requiresReaction_
-                : globalState.computedRequiresReaction
+                : globalState.computedRequiresReaction)
         ) {
             console.warn(
                 `[mobx] Computed value '${this.name_}' is being read outside a reactive context. Doing a full recompute.`

packages/mobx/src/core/globalstate.ts

@@ -96,9 +96,9 @@ export class MobXGlobals {
 
     /**
      * Is it allowed to read observables at this point?
-     * Used to hold the state needed for `observableRequiresReaction`
+     * Used to hold the state needed for `observableRequiresReaction`/`computedRequiresReaction`
      */
-    allowStateReads = true
+    allowStateReads = false
 
     /**
      * If strict mode is enabled, state changes are by default not allowed

packages/mobx/src/core/reaction.ts

@@ -18,7 +18,8 @@ import {
     spyReportStart,
     startBatch,
     trace,
-    trackDerivedFunction, GenericAbortSignal
+    trackDerivedFunction,
+    GenericAbortSignal
 } from "../internal"
 
 /**