Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
6 changes: 6 additions & 0 deletions .changeset/fluffy-hairs-study.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
"mobx-react": patch
"mobx-react-lite": patch
---

fix #3648: `observableRequiresReaction`/`computedRequiresReaction` shouldn't warn with `enableStaticRendering(true)`
5 changes: 5 additions & 0 deletions .changeset/shaggy-carpets-move.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"mobx": patch
---

`computedRequiresReaction` respects `globalState.allowStateReads`
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down
21 changes: 21 additions & 0 deletions packages/mobx-react-lite/__tests__/observer.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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()
mweststrate marked this conversation as resolved.
Show resolved Hide resolved
} finally {
enableStaticRendering(false)
mobx._resetGlobalState()
consoleWarnMock.mockRestore()
}
})

test("StrictMode #3671", async () => {
const o = mobx.observable({ x: 0 })

Expand Down
6 changes: 0 additions & 6 deletions packages/mobx-react-lite/src/observer.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,4 @@
import { forwardRef, memo } from "react"

import { isUsingStaticRendering } from "./staticRendering"
import { useObserver } from "./useObserver"

let warnObserverOptionsDeprecated = true
Expand Down Expand Up @@ -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
mweststrate marked this conversation as resolved.
Show resolved Hide resolved
}

let useForwardRef = options?.forwardRef ?? false
let render = baseComponent

Expand Down
9 changes: 7 additions & 2 deletions packages/mobx-react-lite/src/useObserver.ts
Original file line number Diff line number Diff line change
@@ -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"
Expand Down Expand Up @@ -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)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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]`;
Expand Down
56 changes: 33 additions & 23 deletions packages/mobx-react/__tests__/observer.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,8 @@ import {
autorun,
IReactionDisposer,
reaction,
configure
configure,
runInAction
} from "mobx"
import { withConsole } from "./utils/withConsole"
import { shallowEqual } from "../src/utils/utils"
Expand Down Expand Up @@ -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", () => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is wrong on multiple levels:
I don't recall changing state in render should ever throw, with a slight exception of autoAction, which doesn't work ATM as mentioned elsewhere.
It doesn't expect(...).toThrow(), it just checks for the error (that is never thrown).
If it's about enforceAction, it should expect console.warn instead of an error.
It pollutes console with Warning: Cannot update a component while rendering a different component (this is a bit surprising given that only one component is involved, but not a concern atm).

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
Expand Down Expand Up @@ -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()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See previous comment

} 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
Expand Down Expand Up @@ -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 ""
Expand Down
15 changes: 14 additions & 1 deletion packages/mobx-react/src/observerClass.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -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)

Expand Down
10 changes: 5 additions & 5 deletions packages/mobx/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
})
Expand All @@ -1380,7 +1380,7 @@ or alternatively:

```javascript
observable({
computedProp: computed(function() {
computedProp: computed(function () {
return someComputation
})
})
Expand All @@ -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++
})
})
Expand Down Expand Up @@ -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)
}
)
Expand Down
18 changes: 12 additions & 6 deletions packages/mobx/__tests__/v5/base/autorun.js
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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([])
})
Expand Down
2 changes: 1 addition & 1 deletion packages/mobx/__tests__/v5/base/reaction.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 }
Expand Down
4 changes: 2 additions & 2 deletions packages/mobx/src/api/autorun.ts
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ export function autorun(
view(reaction)
}

if(!opts?.signal?.aborted) {
if (!opts?.signal?.aborted) {
reaction.schedule_()
}
return reaction.getDisposer_(opts?.signal)
Expand Down Expand Up @@ -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)
Expand Down
1 change: 0 additions & 1 deletion packages/mobx/src/api/configure.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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."
Expand Down
5 changes: 3 additions & 2 deletions packages/mobx/src/core/computedvalue.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.`
Expand Down
4 changes: 2 additions & 2 deletions packages/mobx/src/core/globalstate.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be a breaking change? By default we allow state reads outside reactive contexts?

Copy link
Collaborator Author

@urugator urugator Mar 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. These flags, allowStateReads and allowStateChanges, says whether you're in a context, where reads/changes are allowed - or perhaps better - where they are supposed to take place. They're basically inDerivation/inAction flags - they're switched on/off when entering/leaving these, regardless of whether it should warn or not. We use these flags instead of directly checking for action/derivation, because the mapping isn't always exactly 1:1. But really the point is to demark these places, not to directly control the warning - the warning is always only raised in combination with requiresReaction/enforceAction. They should always start as false, because there is no running action/derivation by default.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check, that adds up. I haven't been working on this for too long 😅.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Turns out it's more complicated. I think it was originally designed as described, but it was maybe misunderstood and changed with the introduction of autoAction.
The thing is that autoAction is supposed to warn when you modify state inside derivation, regardless of enforceAction configuration. So autoAction assumes that if allowStateChanges is false, there will be a warning when setting state, no other conditions needed.
In order for this to work the configure/resetGlobalState was updated to synchronize allowStateChanges with enforeAction configuration.
But the actual check for the warning doesn't assume this behavior:

if (
!globalState.allowStateChanges &&
(hasObservers || globalState.enforceActions === "always")
) {

Notice there is a bug - the check doesn't respect enforceActions: "never" (enforceActions === false).
This bug is actually a reason why the test for autoAction warning is passing - there is a test above that calls mobx.configure({ enforceActions: "never" }).
If you fix this bug, the autoAction warning (Side effects like changing state are not allowed) can never occur.


/**
* If strict mode is enabled, state changes are by default not allowed
Expand Down
3 changes: 2 additions & 1 deletion packages/mobx/src/core/reaction.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,8 @@ import {
spyReportStart,
startBatch,
trace,
trackDerivedFunction, GenericAbortSignal
trackDerivedFunction,
GenericAbortSignal
} from "../internal"

/**
Expand Down