diff --git a/.changeset/fluffy-hairs-study.md b/.changeset/fluffy-hairs-study.md new file mode 100644 index 000000000..80838c462 --- /dev/null +++ b/.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)` diff --git a/.changeset/shaggy-carpets-move.md b/.changeset/shaggy-carpets-move.md new file mode 100644 index 000000000..031730e8f --- /dev/null +++ b/.changeset/shaggy-carpets-move.md @@ -0,0 +1,5 @@ +--- +"mobx": patch +--- + +`computedRequiresReaction` respects `globalState.allowStateReads` diff --git a/packages/mobx-react-lite/__tests__/__snapshots__/observer.test.tsx.snap b/packages/mobx-react-lite/__tests__/__snapshots__/observer.test.tsx.snap index 2186b994d..5d2ea11d0 100644 --- a/packages/mobx-react-lite/__tests__/__snapshots__/observer.test.tsx.snap +++ b/packages/mobx-react-lite/__tests__/__snapshots__/observer.test.tsx.snap @@ -16,6 +16,8 @@ exports[`changing state in render should fail 2`] = ` `; +exports[`#3648 enableStaticRendering doesn't warn with observableRequiresReaction/computedRequiresReaction 1`] = `[MockFunction]`; + exports[`issue 12 init state is correct 1`] = `
diff --git a/packages/mobx-react-lite/__tests__/observer.test.tsx b/packages/mobx-react-lite/__tests__/observer.test.tsx index 607e5d21d..4cf50b4a5 100644 --- a/packages/mobx-react-lite/__tests__/observer.test.tsx +++ b/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(() =>
{o.get() + c.get()}
) + + const { unmount, container } = render() + 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 }) diff --git a/packages/mobx-react-lite/src/observer.ts b/packages/mobx-react-lite/src/observer.ts index b625a742e..e484f0541 100644 --- a/packages/mobx-react-lite/src/observer.ts +++ b/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

( } // 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 diff --git a/packages/mobx-react-lite/src/useObserver.ts b/packages/mobx-react-lite/src/useObserver.ts index 8bc07b360..03119c10b 100644 --- a/packages/mobx-react-lite/src/useObserver.ts +++ b/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(render: () => T, baseComponentName: string = "observed"): T { if (isUsingStaticRendering()) { - return render() + let allowStateReads = _allowStateReadsStart?.(true) + try { + return render() + } finally { + _allowStateReadsEnd?.(allowStateReads) + } } const admRef = React.useRef(null) diff --git a/packages/mobx-react/__tests__/__snapshots__/observer.test.tsx.snap b/packages/mobx-react/__tests__/__snapshots__/observer.test.tsx.snap index 78c26f6b9..a1e13922e 100644 --- a/packages/mobx-react/__tests__/__snapshots__/observer.test.tsx.snap +++ b/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]`; diff --git a/packages/mobx-react/__tests__/observer.test.tsx b/packages/mobx-react/__tests__/observer.test.tsx index cf8d296f8..cc91f630a 100644 --- a/packages/mobx-react/__tests__/observer.test.tsx +++ b/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

{data.get()}
- }) - render() - - act(() => data.set(3)) - _resetGlobalState() -}) - test("observer component can be injected", () => { const msg: Array = [] 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 { + render() { + expect(this).toBeDefined() + return o.get() + c.get() + } + } + + const { unmount, container } = render() + 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 "" diff --git a/packages/mobx-react/src/observerClass.ts b/packages/mobx-react/src/observerClass.ts index c428e8003..79bd169bf 100644 --- a/packages/mobx-react/src/observerClass.ts +++ b/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 || "" } +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) diff --git a/packages/mobx/CHANGELOG.md b/packages/mobx/CHANGELOG.md index 7b7a564d8..3461beca9 100644 --- a/packages/mobx/CHANGELOG.md +++ b/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) } ) diff --git a/packages/mobx/__tests__/v5/base/autorun.js b/packages/mobx/__tests__/v5/base/autorun.js index 6cf422207..e6ce78b10 100644 --- a/packages/mobx/__tests__/v5/base/autorun.js +++ b/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([]) }) diff --git a/packages/mobx/__tests__/v5/base/reaction.js b/packages/mobx/__tests__/v5/base/reaction.js index 4ac9e273f..fd4de1517 100644 --- a/packages/mobx/__tests__/v5/base/reaction.js +++ b/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 } diff --git a/packages/mobx/src/api/autorun.ts b/packages/mobx/src/api/autorun.ts index ddf96ed3f..f55cc6062 100644 --- a/packages/mobx/src/api/autorun.ts +++ b/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( firstTime = false } - if(!opts?.signal?.aborted) { + if (!opts?.signal?.aborted) { r.schedule_() } return r.getDisposer_(opts?.signal) diff --git a/packages/mobx/src/api/configure.ts b/packages/mobx/src/api/configure.ts index cfabf9f2d..23e617b6e 100644 --- a/packages/mobx/src/api/configure.ts +++ b/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." diff --git a/packages/mobx/src/core/computedvalue.ts b/packages/mobx/src/core/computedvalue.ts index a56d59525..8a6d0c0ac 100644 --- a/packages/mobx/src/core/computedvalue.ts +++ b/packages/mobx/src/core/computedvalue.ts @@ -309,9 +309,10 @@ export class ComputedValue implements IObservable, IComputedValue, 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.` diff --git a/packages/mobx/src/core/globalstate.ts b/packages/mobx/src/core/globalstate.ts index 7ffa26006..87efdb850 100644 --- a/packages/mobx/src/core/globalstate.ts +++ b/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 diff --git a/packages/mobx/src/core/reaction.ts b/packages/mobx/src/core/reaction.ts index 7baf5bb41..0482ba6a9 100644 --- a/packages/mobx/src/core/reaction.ts +++ b/packages/mobx/src/core/reaction.ts @@ -18,7 +18,8 @@ import { spyReportStart, startBatch, trace, - trackDerivedFunction, GenericAbortSignal + trackDerivedFunction, + GenericAbortSignal } from "../internal" /**