From bbcb12dc754524552181b177a52ffdbe80ecb953 Mon Sep 17 00:00:00 2001 From: urugator <11457665+urugator@users.noreply.github.com> Date: Mon, 27 Jun 2022 09:34:09 +0200 Subject: [PATCH] Fix #3395: observer not working with (#3434) --- .changeset/clean-islands-retire.md | 5 + .../mobx-react/__tests__/observer.test.tsx | 35 +++++- packages/mobx-react/src/observerClass.ts | 105 +++++++++++------- 3 files changed, 106 insertions(+), 39 deletions(-) create mode 100644 .changeset/clean-islands-retire.md diff --git a/.changeset/clean-islands-retire.md b/.changeset/clean-islands-retire.md new file mode 100644 index 000000000..e1d99f1d9 --- /dev/null +++ b/.changeset/clean-islands-retire.md @@ -0,0 +1,5 @@ +--- +"mobx-react": patch +--- + +Support re-mounting of class components. Fixes #3395: observer not working with React@18 <StrictMode>. diff --git a/packages/mobx-react/__tests__/observer.test.tsx b/packages/mobx-react/__tests__/observer.test.tsx index c909f17aa..5c2454ca4 100644 --- a/packages/mobx-react/__tests__/observer.test.tsx +++ b/packages/mobx-react/__tests__/observer.test.tsx @@ -1,4 +1,4 @@ -import React, { createContext } from "react" +import React, { createContext, StrictMode } from "react" import { inject, observer, Observer, enableStaticRendering } from "../src" import { render, act } from "@testing-library/react" import { @@ -976,3 +976,36 @@ test("this.context is observable if ComponentName.contextType is set", () => { expect(container).toHaveTextContent("1") unmount() }) + +test("class observer supports re-mounting #3395", () => { + const state = observable.box(1) + let mountCounter = 0 + + @observer + class TestCmp extends React.Component { + componentDidMount() { + mountCounter++ + } + render() { + return state.get() + } + } + + const app = ( + + + + ) + + const { unmount, container } = render(app) + + expect(mountCounter).toBe(2) + expect(container).toHaveTextContent("1") + act(() => { + state.set(2) + }) + expect(mountCounter).toBe(2) + expect(container).toHaveTextContent("2") + + unmount() +}) diff --git a/packages/mobx-react/src/observerClass.ts b/packages/mobx-react/src/observerClass.ts index e905bc32d..fb49214dc 100644 --- a/packages/mobx-react/src/observerClass.ts +++ b/packages/mobx-react/src/observerClass.ts @@ -11,7 +11,7 @@ import { isUsingStaticRendering } from "mobx-react-lite" import { newSymbol, shallowEqual, setHiddenProp, patch } from "./utils/utils" -const mobxAdminProperty = $mobx || "$mobx" +const mobxAdminProperty = $mobx || "$mobx" // BC const mobxObserverProperty = newSymbol("isMobXReactObserver") const mobxIsUnmounted = newSymbol("isUnmounted") const skipRenderKey = newSymbol("skipRender") @@ -32,15 +32,18 @@ export function makeClassComponentObserver( componentClass[mobxObserverProperty] = true } - if (target.componentWillReact) + if (target.componentWillReact) { throw new Error("The componentWillReact life-cycle event is no longer supported") + } if (componentClass["__proto__"] !== PureComponent) { - if (!target.shouldComponentUpdate) target.shouldComponentUpdate = observerSCU - else if (target.shouldComponentUpdate !== observerSCU) + if (!target.shouldComponentUpdate) { + target.shouldComponentUpdate = observerSCU + } else if (target.shouldComponentUpdate !== observerSCU) { // n.b. unequal check, instead of existence check, as @observer might be on superclass as well throw new Error( "It is not allowed to use shouldComponentUpdate in observer based components." ) + } } // this.props and this.state are made observable, just to make sure @computed fields that @@ -53,8 +56,8 @@ export function makeClassComponentObserver( makeObservableProp(target, "context") } - const baseRender = target.render - if (typeof baseRender !== "function") { + const originalRender = target.render + if (typeof originalRender !== "function") { const displayName = getDisplayName(target) throw new Error( `[mobx-react] class component (${displayName}) is missing \`render\` method.` + @@ -63,14 +66,30 @@ export function makeClassComponentObserver( ) } target.render = function () { - return makeComponentReactive.call(this, baseRender) + if (!isUsingStaticRendering()) { + this.render = createReactiveRender.call(this, originalRender) + } + return this.render() } + patch(target, "componentDidMount", function () { + this[mobxIsUnmounted] = false + if (!this.render[mobxAdminProperty]) { + // Reaction is re-created automatically during render, but a component can re-mount and skip render #3395. + // To re-create the reaction and re-subscribe to relevant observables we have to force an update. + Component.prototype.forceUpdate.call(this) + } + }) patch(target, "componentWillUnmount", function () { - if (isUsingStaticRendering() === true) return - this.render[mobxAdminProperty]?.dispose() - this[mobxIsUnmounted] = true + if (isUsingStaticRendering()) { + return + } - if (!this.render[mobxAdminProperty]) { + const reaction = this.render[mobxAdminProperty] + if (reaction) { + reaction.dispose() + // Forces reaction to be re-created on next render + this.render[mobxAdminProperty] = null + } else { // Render may have been hot-swapped and/or overriden by a subclass. const displayName = getDisplayName(this) console.warn( @@ -79,6 +98,8 @@ export function makeClassComponentObserver( overriden reactive render was not properly disposed.` ) } + + this[mobxIsUnmounted] = true }) return componentClass } @@ -93,9 +114,7 @@ function getDisplayName(comp: any) { ) } -function makeComponentReactive(render: any) { - if (isUsingStaticRendering() === true) return render.call(this) - +function createReactiveRender(originalRender: any) { /** * If props are shallowly modified, react will render anyway, * so atom.reportChanged() should not result in yet another re-render @@ -108,41 +127,51 @@ function makeComponentReactive(render: any) { setHiddenProp(this, isForcingUpdateKey, false) const initialName = getDisplayName(this) - const baseRender = render.bind(this) + const boundOriginalRender = originalRender.bind(this) let isRenderingPending = false - const reaction = new Reaction(`${initialName}.render()`, () => { - if (!isRenderingPending) { - // N.B. Getting here *before mounting* means that a component constructor has side effects (see the relevant test in misc.js) - // This unidiomatic React usage but React will correctly warn about this so we continue as usual - // See #85 / Pull #44 - isRenderingPending = true - if (this[mobxIsUnmounted] !== true) { - let hasError = true - try { - setHiddenProp(this, isForcingUpdateKey, true) - if (!this[skipRenderKey]) Component.prototype.forceUpdate.call(this) - hasError = false - } finally { - setHiddenProp(this, isForcingUpdateKey, false) - if (hasError) reaction.dispose() + const createReaction = () => { + const reaction = new Reaction(`${initialName}.render()`, () => { + if (!isRenderingPending) { + // N.B. Getting here *before mounting* means that a component constructor has side effects (see the relevant test in misc.test.tsx) + // This unidiomatic React usage but React will correctly warn about this so we continue as usual + // See #85 / Pull #44 + isRenderingPending = true + if (this[mobxIsUnmounted] !== true) { + let hasError = true + try { + setHiddenProp(this, isForcingUpdateKey, true) + if (!this[skipRenderKey]) { + Component.prototype.forceUpdate.call(this) + } + hasError = false + } finally { + setHiddenProp(this, isForcingUpdateKey, false) + if (hasError) { + reaction.dispose() + // Forces reaction to be re-created on next render + this.render[mobxAdminProperty] = null + } + } } } - } - }) - - reaction["reactComponent"] = this - reactiveRender[mobxAdminProperty] = reaction - this.render = reactiveRender + }) + reaction["reactComponent"] = this + return reaction + } function reactiveRender() { isRenderingPending = false + // Create reaction lazily to support re-mounting #3395 + const reaction = (reactiveRender[mobxAdminProperty] ??= createReaction()) let exception: unknown = undefined let rendering = undefined reaction.track(() => { try { - rendering = _allowStateChanges(false, baseRender) + // TODO@major + // Optimization: replace with _allowStateChangesStart/End (not available in mobx@6.0.0) + rendering = _allowStateChanges(false, boundOriginalRender) } catch (e) { exception = e } @@ -153,7 +182,7 @@ function makeComponentReactive(render: any) { return rendering } - return reactiveRender.call(this) + return reactiveRender } function observerSCU(nextProps: React.ClassAttributes, nextState: any): boolean {