diff --git a/.changeset/bright-hornets-listen.md b/.changeset/bright-hornets-listen.md new file mode 100644 index 0000000000..77f75b119c --- /dev/null +++ b/.changeset/bright-hornets-listen.md @@ -0,0 +1,5 @@ +--- +"mobx-react": patch +--- + +Reintroduce observable state,props,context in mobx-react diff --git a/packages/mobx-react/README.md b/packages/mobx-react/README.md index 4bdf704af9..51b556efb2 100644 --- a/packages/mobx-react/README.md +++ b/packages/mobx-react/README.md @@ -58,6 +58,8 @@ Function (and decorator) that converts a React component definition, React compo #### Class Components +When using component classes, `this.props` and `this.state` will be made observables, so the component will react to all changes in props and state that are used by `render`. + `shouldComponentUpdate` is not supported. As such, it is recommended that class components extend `React.PureComponent`. The `observer` will automatically patch non-pure class components with an internal implementation of `React.PureComponent` if necessary. Extending `observer` class components is not supported. Always apply `observer` only on the last class in the inheritance chain. @@ -88,92 +90,6 @@ class TodoView extends React.Component { const TodoView = observer(({ todo }) =>
{todo.title}
) ``` -##### Note on using props and state in derivations - -`mobx-react` version 6 and lower would automatically turn `this.state` and `this.props` into observables. -This has the benefit that computed properties and reactions were able to observe those. -However, since this pattern is fundamentally incompatible with `StrictMode` in React 18.2 and higher, this behavior has been removed in React 18. - -As a result, we recommend to no longer mark properties as `@computed` in observer components if they depend on `this.state` or `this.props`. - -```javascript -@observer -class Doubler extends React.Component<{ counter: number }> { - @computed // BROKEN! <-- @computed should be removed in mobx-react > 7 - get doubleValue() { - // Changes to this.props will no longer be detected properly, to fix it, - // remove the @computed annotation. - return this.props * 2 - } - - render() { - return
{this.doubleValue}
- } -} -``` - -Similarly, reactions will no longer respond to `this.state` / `this.props`. This can be overcome by creating an observable copy: - -```javascript -@observer -class Alerter extends React.Component<{ counter: number }> { - @observable observableCounter: number - reactionDisposer - - constructor(props) { - this.observableCounter = counter - } - - componentDidMount() { - // set up a reaction, by observing the observable, - // rather than the prop which is non-reactive: - this.reactionDisposer = autorun(() => { - if (this.observableCounter > 10) { - alert("Reached 10!") - } - }) - } - - componentDidUpdate() { - // sync the observable from props - this.observableCounter = this.props.counter - } - - componentWillUnmount() { - this.reactionDisposer() - } - - render() { - return
{this.props.counter}
- } -} -``` - -MobX-react will try to detect cases where `this.props`, `this.state` or `this.context` are used by any other derivation than the `render` method of the owning component and throw. -This is to make sure that neither computed properties, nor reactions, nor other components accidentally rely on those fields to be reactive. - -This includes cases where a render callback is passed to a child, that will read from the props or state of a parent component. -As a result, passing a function that might later read a property of a parent in a reactive context will throw as well. -Instead, when using a callback function that is being passed to an `observer` based child, the capture should be captured locally first: - -```javascript -@observer -class ChildWrapper extends React.Component<{ counter: number }> { - render() { - // Collapsible is an observer component that should respond to this.counter, - // if it is expanded - - // BAD: - return

{this.props.counter}

} /> - - // GOOD: (causes to pass down a fresh callback whenever counter changes, - // that doesn't depend on its parents props) - const counter = this.props.counter - return

{counter}

} /> - } -} -``` - ### `Observer` `Observer` is a React component, which applies `observer` to an anonymous region in your component. diff --git a/packages/mobx-react/__tests__/exp.test.tsx b/packages/mobx-react/__tests__/exp.test.tsx new file mode 100644 index 0000000000..11a6e8f72b --- /dev/null +++ b/packages/mobx-react/__tests__/exp.test.tsx @@ -0,0 +1,63 @@ +import React from "react" +import { observer } from "../src" +import { render, act } from "@testing-library/react" + +/** + * some test suite is too tedious + */ + +afterEach(() => { + jest.useRealTimers() +}) + +// let consoleWarnMock: jest.SpyInstance | undefined +// afterEach(() => { +// consoleWarnMock?.mockRestore() +// }) + +test("TODO", async () => { + let renderCount = 0 + const Child = observer(function Child({ children }) { + renderCount++ + // Accesses Parent's this.props + return children() + }) + + @observer + class Parent extends React.Component { + // intentionally stable, so test breaks when you disable observable props (comment line 239 in observerClass) + renderCallback = () => { + return this.props.x + } + render() { + // Access observable props as part of child + return {this.renderCallback} + } + } + + function Root() { + const [x, setX] = React.useState(0) + // Send new props to Parent + return ( +
setX(x => x + 1)}> + +
+ ) + } + + const app = + + const { unmount, container } = render(app) + + expect(container).toHaveTextContent("0") + expect(renderCount).toBe(1) + + await new Promise(resolve => setTimeout(() => resolve(null), 1000)) + act(() => { + console.log("changing state") + container.querySelector("div")?.click() + }) + expect(container).toHaveTextContent("1") + expect(renderCount).toBe(2) + unmount() +}) diff --git a/packages/mobx-react/__tests__/issue21.test.tsx b/packages/mobx-react/__tests__/issue21.test.tsx index 99c8c3d792..cd23e77d73 100644 --- a/packages/mobx-react/__tests__/issue21.test.tsx +++ b/packages/mobx-react/__tests__/issue21.test.tsx @@ -212,6 +212,113 @@ test("verify prop changes are picked up", () => { expect(container.textContent).toMatchInlineSnapshot(`"1.2.test.0"`) }) +test("verify props is reactive", () => { + function createItem(subid, label) { + const res = observable( + { + subid, + id: 1, + label: label, + get text() { + events.push(["compute", this.subid]) + return ( + this.id + + "." + + this.subid + + "." + + this.label + + "." + + data.items.indexOf(this as any) + ) + } + }, + {}, + { proxy: false } + ) + res.subid = subid // non reactive + return res + } + + const data = observable({ + items: [createItem(1, "hi")] + }) + const events: Array = [] + + class Child extends React.Component { + constructor(p) { + super(p) + makeObservable(this) + } + + @computed + get computedLabel() { + events.push(["computed label", this.props.item.subid]) + return this.props.item.label + } + componentDidMount() { + events.push(["mount"]) + } + componentDidUpdate(prevProps) { + events.push(["update", prevProps.item.subid, this.props.item.subid]) + } + render() { + events.push(["render", this.props.item.subid, this.props.item.text, this.computedLabel]) + return ( + + {this.props.item.text} + {this.computedLabel} + + ) + } + } + + const ChildAsObserver = observer(Child) + + const Parent = observer( + class Parent extends React.Component { + render() { + return ( +
+ {data.items.map(item => ( + + ))} +
+ ) + } + } + ) + + const Wrapper = () => + + function changeStuff() { + act(() => { + transaction(() => { + // components start rendeirng a new item, but computed is still based on old value + data.items = [createItem(2, "test")] + }) + }) + } + + const { container } = render() + expect(events.sort()).toEqual( + [["mount"], ["compute", 1], ["computed label", 1], ["render", 1, "1.1.hi.0", "hi"]].sort() + ) + + events.splice(0) + let testDiv = container.querySelector("#testDiv") as HTMLElement + testDiv.click() + + expect(events.sort()).toEqual( + [ + ["compute", 1], + ["update", 1, 2], + ["compute", 2], + ["computed label", 2], + ["render", 2, "1.2.test.0", "test"] + ].sort() + ) +}) + test("no re-render for shallow equal props", async () => { function createItem(subid, label) { const res = observable({ @@ -308,3 +415,40 @@ test("lifecycle callbacks called with correct arguments", () => { let testButton = container.querySelector("#testButton") as HTMLElement testButton.click() }) + +test("verify props are reactive in constructor", () => { + const propValues: Array = [] + let constructorCallsCount = 0 + + const Component = observer( + class Component extends React.Component { + disposer: IReactionDisposer + constructor(props, context) { + super(props, context) + constructorCallsCount++ + this.disposer = reaction( + () => this.props.prop, + prop => propValues.push(prop), + { + fireImmediately: true + } + ) + } + + componentWillUnmount() { + this.disposer() + } + + render() { + return
+ } + } + ) + + const { rerender } = render() + rerender() + rerender() + rerender() + expect(constructorCallsCount).toEqual(1) + expect(propValues).toEqual(["1", "2", "3", "4"]) +}) diff --git a/packages/mobx-react/__tests__/observer.test.tsx b/packages/mobx-react/__tests__/observer.test.tsx index cf8d296f84..37dab4697b 100644 --- a/packages/mobx-react/__tests__/observer.test.tsx +++ b/packages/mobx-react/__tests__/observer.test.tsx @@ -1,4 +1,4 @@ -import React, { StrictMode, Suspense } from "react" +import React, { createContext, StrictMode, Suspense } from "react" import { inject, observer, Observer, enableStaticRendering } from "../src" import { render, act, waitFor } from "@testing-library/react" import { @@ -10,10 +10,7 @@ import { observable, transaction, makeObservable, - autorun, - IReactionDisposer, - reaction, - configure + runInAction } from "mobx" import { withConsole } from "./utils/withConsole" import { shallowEqual } from "../src/utils/utils" @@ -506,14 +503,14 @@ describe("should render component even if setState called with exactly the same expect(renderCount).toBe(2) }) - test("after click twice renderCount === 3", () => { + test("after click twice renderCount === 2", () => { const { container } = render() const clickableDiv = container.querySelector("#clickableDiv") as HTMLElement act(() => clickableDiv.click()) act(() => clickableDiv.click()) - expect(renderCount).toBe(3) + expect(renderCount).toBe(2) }) }) @@ -897,6 +894,74 @@ test("Missing render should throw", () => { expect(() => observer(Component)).toThrow() }) +test("this.context is observable if ComponentName.contextType is set", () => { + const Context = createContext({}) + + let renderCounter = 0 + + @observer + class Parent extends React.Component { + constructor(props: any) { + super(props) + makeObservable(this) + } + + @observable + counter = 0 + + @computed + get contextValue() { + return { counter: this.counter } + } + + render() { + return ( + {this.props.children} + ) + } + } + + @observer + class Child extends React.Component { + static contextType = Context + + constructor(props: any) { + super(props) + makeObservable(this) + } + + @computed + get counterValue() { + return (this.context as any).counter + } + + render() { + renderCounter++ + return
{this.counterValue}
+ } + } + + const parentRef = React.createRef() + + const app = ( + + + + ) + + const { unmount, container } = render(app) + + act(() => { + if (parentRef.current) { + parentRef.current!.counter = 1 + } + }) + + expect(renderCounter).toBe(2) + expect(container).toHaveTextContent("1") + unmount() +}) + test("class observer supports re-mounting #3395", () => { const state = observable.box(1) let mountCounter = 0 @@ -1011,61 +1076,6 @@ test("#3492 should not cause warning by calling forceUpdate on uncommited compon unmount() expect(consoleWarnMock).toMatchSnapshot() }) -;["props", "state", "context"].forEach(key => { - test(`using ${key} in computed throws`, () => { - // React prints errors even if we catch em - const consoleErrorSpy = jest.spyOn(console, "error").mockImplementation(() => {}) - - const TestCmp = observer( - class TestCmp extends React.Component { - render() { - computed(() => this[key]).get() - return "" - } - } - ) - - expect(() => render()).toThrowError( - new RegExp(`^\\[mobx-react\\] Cannot read "TestCmp.${key}" in a reactive context`) - ) - - consoleErrorSpy.mockRestore() - }) - - test(`using ${key} in autorun throws`, () => { - // React prints errors even if we catch em - const consoleErrorSpy = jest.spyOn(console, "error").mockImplementation(() => {}) - - let caughtError - - const TestCmp = observer( - class TestCmp extends React.Component { - disposeAutorun: IReactionDisposer | undefined - - componentDidMount(): void { - this.disposeAutorun = autorun(() => this[key], { - onError: error => (caughtError = error) - }) - } - - componentWillUnmount(): void { - this.disposeAutorun?.() - } - - render() { - return "" - } - } - ) - - render() - expect(caughtError?.message).toMatch( - new RegExp(`^\\[mobx-react\\] Cannot read "TestCmp.${key}" in a reactive context`) - ) - - consoleErrorSpy.mockRestore() - }) -}) test(`Component react's to observable changes in componenDidMount #3691`, () => { const o = observable.box(0) @@ -1115,189 +1125,12 @@ test(`Observable changes in componenWillUnmount don't cause any warnings or erro consoleWarnSpy.mockRestore() }) -test(`Observable prop workaround`, () => { - configure({ - enforceActions: "observed" - }) - - const propValues: Array = [] - - const TestCmp = observer( - class TestCmp extends React.Component<{ prop: number }> { - disposeReaction: IReactionDisposer | undefined - observableProp: number - - get computed() { - return this.observableProp + 100 - } - - constructor(props) { - super(props) - // Synchronize our observableProp with the actual prop on the first render. - this.observableProp = this.props.prop - makeObservable(this, { - observableProp: observable, - computed: computed, - // Mutates observable therefore must be action - componentDidUpdate: action - }) - } - - componentDidMount(): void { - // Reactions/autoruns must be created in componenDidMount (not in constructor). - this.disposeReaction = reaction( - () => this.observableProp, - prop => propValues.push(prop), - { - fireImmediately: true - } - ) - } - - componentDidUpdate(): void { - // Synchronize our observableProp with the actual prop on every update. - this.observableProp = this.props.prop - } - - componentWillUnmount(): void { - this.disposeReaction?.() - } - - render() { - return this.computed - } - } - ) - - const { container, unmount, rerender } = render() - expect(container).toHaveTextContent("101") - rerender() - expect(container).toHaveTextContent("102") - rerender() - expect(container).toHaveTextContent("103") - rerender() - expect(container).toHaveTextContent("104") - expect(propValues).toEqual([1, 2, 3, 4]) - unmount() -}) - -test(`Observable props/state/context workaround`, () => { - configure({ - enforceActions: "observed" - }) - - const reactionResults: Array = [] - - const ContextType = React.createContext(0) - - const TestCmp = observer( - class TestCmp extends React.Component { - static contextType = ContextType - - disposeReaction: IReactionDisposer | undefined - observableProps: any - observableState: any - observableContext: any - - constructor(props, context) { - super(props, context) - this.state = { - x: 0 - } - this.observableState = this.state - this.observableProps = this.props - this.observableContext = this.context - makeObservable(this, { - observableProps: observable, - observableState: observable, - observableContext: observable, - computed: computed, - componentDidUpdate: action - }) - } - - get computed() { - return `${this.observableProps?.x}${this.observableState?.x}${this.observableContext}` - } - - componentDidMount(): void { - this.disposeReaction = reaction( - () => this.computed, - prop => reactionResults.push(prop), - { - fireImmediately: true - } - ) - } - - componentDidUpdate(): void { - // Props are different object with every update - if (!shallowEqual(this.observableProps, this.props)) { - this.observableProps = this.props - } - if (!shallowEqual(this.observableState, this.state)) { - this.observableState = this.state - } - if (!shallowEqual(this.observableContext, this.context)) { - this.observableContext = this.context - } - } - - componentWillUnmount(): void { - this.disposeReaction?.() - } - - render() { - return ( - this.setState(state => ({ x: state.x + 1 }))} - > - {this.computed} - - ) - } - } - ) - - const App = () => { - const [context, setContext] = React.useState(0) - const [prop, setProp] = React.useState(0) - return ( - - setContext(val => val + 1)}> - setProp(val => val + 1)}> - - - ) - } - - const { container, unmount } = render() - - const updateProp = () => - act(() => (container.querySelector("#updateProp") as HTMLElement).click()) - const updateState = () => - act(() => (container.querySelector("#updateState") as HTMLElement).click()) - const updateContext = () => - act(() => (container.querySelector("#updateContext") as HTMLElement).click()) - - expect(container).toHaveTextContent("000") - updateProp() - expect(container).toHaveTextContent("100") - updateState() - expect(container).toHaveTextContent("110") - updateContext() - expect(container).toHaveTextContent("111") - - expect(reactionResults).toEqual(["000", "100", "110", "111"]) - unmount() -}) - test("Class observer can react to changes made before mount #3730", () => { const o = observable.box(0) @observer class Child extends React.Component { + @action componentDidMount(): void { o.set(1) } diff --git a/packages/mobx-react/src/observerClass.ts b/packages/mobx-react/src/observerClass.ts index c428e8003f..8e2ee64b7a 100644 --- a/packages/mobx-react/src/observerClass.ts +++ b/packages/mobx-react/src/observerClass.ts @@ -1,10 +1,12 @@ import { PureComponent, Component, ComponentClass, ClassAttributes } from "react" import { + createAtom, _allowStateChanges, Reaction, _allowStateReadsStart, _allowStateReadsEnd, - _getGlobalState + _getGlobalState, + IAtom } from "mobx" import { isUsingStaticRendering, @@ -15,25 +17,24 @@ import { shallowEqual, patch } from "./utils/utils" const administrationSymbol = Symbol("ObserverAdministration") const isMobXReactObserverSymbol = Symbol("isMobXReactObserver") -let observablePropDescriptors: PropertyDescriptorMap -if (__DEV__) { - observablePropDescriptors = { - props: createObservablePropDescriptor("props"), - state: createObservablePropDescriptor("state"), - context: createObservablePropDescriptor("context") - } -} - type ObserverAdministration = { reaction: Reaction | null // also serves as disposed flag forceUpdate: Function | null mounted: boolean // we could use forceUpdate as mounted flag reactionInvalidatedBeforeMount: boolean name: string - // Used only on __DEV__ + propsAtom: IAtom + stateAtom: IAtom + contextAtom: IAtom props: any state: any context: any + // Setting this.props causes forceUpdate, because this.props is observable. + // forceUpdate sets this.props. + // This flag is used to avoid the loop. + isUpdating: boolean + changedVariables: WeakSet + unchangedVariables: WeakSet } function getAdministration(component: Component): ObserverAdministration { @@ -48,7 +49,13 @@ function getAdministration(component: Component): ObserverAdministration { name: getDisplayName(component.constructor as ComponentClass), state: undefined, props: undefined, - context: undefined + context: undefined, + propsAtom: createAtom("props"), + stateAtom: createAtom("state"), + contextAtom: createAtom("context"), + isUpdating: false, + changedVariables: new WeakSet(), + unchangedVariables: new WeakSet() }) } @@ -80,9 +87,15 @@ export function makeClassComponentObserver( } } - if (__DEV__) { - Object.defineProperties(prototype, observablePropDescriptors) - } + // this.props and this.state are made observable, just to make sure @computed fields that + // are defined inside the component, and which rely on state or props, re-compute if state or props change + // (otherwise the computed wouldn't update and become stale on props change, since props are not observable) + // However, this solution is not without it's own problems: https://github.com/mobxjs/mobx-react/issues?utf8=%E2%9C%93&q=is%3Aissue+label%3Aobservable-props-or-not+ + Object.defineProperties(prototype, { + props: observablePropsDescriptor, + state: observableStateDescriptor, + context: observableContextDescriptor + }) const originalRender = prototype.render if (typeof originalRender !== "function") { @@ -216,6 +229,12 @@ function createReactiveRender(originalRender: any) { function createReaction(admin: ObserverAdministration) { return new Reaction(`${admin.name}.render()`, () => { + if (admin.isUpdating) { + // Reaction is suppressed when setting new state/props/context, + // this is when component is already being updated. + return + } + if (!admin.mounted) { // This is neccessary to avoid react warning about calling forceUpdate on component that isn't mounted yet. // This happens when component is abandoned after render - our reaction is already created and reacts to changes. @@ -226,10 +245,14 @@ function createReaction(admin: ObserverAdministration) { } try { + // forceUpdate sets new `props`, since we made it observable, it would `reportChanged`, causing a loop. + admin.isUpdating = true admin.forceUpdate?.() } catch (error) { admin.reaction?.dispose() admin.reaction = null + } finally { + admin.isUpdating = false } }) } @@ -240,35 +263,73 @@ function observerSCU(nextProps: ClassAttributes, nextState: any): boolean { "[mobx-react] It seems that a re-rendering of a React component is triggered while in static (server-side) mode. Please make sure components are rendered only once server-side." ) } - // update on any state changes (as is the default) - if (this.state !== nextState) { - return true - } // update if props are shallowly not equal, inspired by PureRenderMixin // we could return just 'false' here, and avoid the `skipRender` checks etc // however, it is nicer if lifecycle events are triggered like usually, // so we return true here if props are shallowly modified. - return !shallowEqual(this.props, nextProps) + const propsChanged = !shallowEqual(this.props, nextProps) + const stateChanged = !shallowEqual(this.state, nextState) + const admin = getAdministration(this) + const shouldUpdate = propsChanged || stateChanged + + if (propsChanged) { + nextProps && admin.changedVariables.add(nextProps) + } else { + nextProps && admin.unchangedVariables.add(nextProps) + } + if (stateChanged) { + nextState && admin.changedVariables.add(nextState) + } else { + nextState && admin.unchangedVariables.add(nextState) + } + return shouldUpdate } function createObservablePropDescriptor(key: "props" | "state" | "context") { + const atomKey = `${key}Atom` return { configurable: true, enumerable: true, get() { const admin = getAdministration(this) - const derivation = _getGlobalState().trackingDerivation - if (derivation && derivation !== admin.reaction) { - throw new Error( - `[mobx-react] Cannot read "${admin.name}.${key}" in a reactive context, as it isn't observable. - Please use component lifecycle method to copy the value into a local observable first. - See https://github.com/mobxjs/mobx/blob/main/packages/mobx-react/README.md#note-on-using-props-and-state-in-derivations` - ) - } + + let prevReadState = _allowStateReadsStart(true) + + admin[atomKey].reportObserved() + + _allowStateReadsEnd(prevReadState) + return admin[key] }, set(value) { - getAdministration(this)[key] = value + const admin = getAdministration(this) + // forceUpdate issued by reaction sets new props. + // It sets isUpdating to true to prevent loop. + if (!admin.isUpdating && shouldReportChanged(admin, key, value)) { + admin[key] = value + // This notifies all observers including our component, + // but we don't want to cause `forceUpdate`, because component is already updating, + // therefore supress component reaction. + admin.isUpdating = true + admin[atomKey].reportChanged() + admin.isUpdating = false + } else { + admin[key] = value + } } } } + +function shouldReportChanged(admin: ObserverAdministration, key: string, value: any) { + if (admin.changedVariables.has(value)) { + return true + } else if (admin.unchangedVariables.has(value)) { + return false + } else { + return !shallowEqual(admin[key], value) + } +} + +const observablePropsDescriptor = createObservablePropDescriptor("props") +const observableStateDescriptor = createObservablePropDescriptor("state") +const observableContextDescriptor = createObservablePropDescriptor("context")