Fix #3671: observer + StrictMode

.changeset/flat-pumas-cross.md

@@ -0,0 +1,9 @@
+---
+"mobx-react-lite": patch
+---
+
+fix #3671: `observer` + `StrictMode`
+BC: Class component's `props`/`state`/`context` are no longer observable. Attempt to use these in any derivation other than component's `render` throws and error.
+BC: Attempt to apply `observer` on a component that is already an `observer` throws instead of warning.
+docs: link for mobx-react docs
+docs: extending `observer` class components is not supported

docs/react-integration.md

@@ -349,7 +349,7 @@ const TimerView = observer(
 )
 ```
 
-Check out [mobx-react docs](https://github.com/mobxjs/mobx-react#api-documentation) for more information.
+Check out [mobx-react docs](https://github.com/mobxjs/mobx/tree/main/packages/mobx-react#class-components) for more information.
 
 </details>
 

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

@@ -1087,3 +1087,23 @@ test("Anonymous component displayName #3192", () => {
     expect(observerError.message).toEqual(memoError.message)
     consoleErrorSpy.mockRestore()
 })
+
+test("StrictMode #3671", async () => {
+    const o = mobx.observable({ x: 0 })
+
+    const Cmp = observer(() => o.x as any)
+
+    const { container, unmount } = render(
+        <React.StrictMode>
+            <Cmp />
+        </React.StrictMode>
+    )
+
+    expect(container).toHaveTextContent("0")
+    act(
+        mobx.action(() => {
+            o.x++
+        })
+    )
+    expect(container).toHaveTextContent("1")
+})

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

@@ -51,7 +51,7 @@ test("uncommitted components should not leak observations", async () => {
             expect(count2IsObserved).toBeFalsy()
         },
         {
-            timeout: 2000,
+            timeout: 10_000,
             interval: 200
         }
     )

packages/mobx-react-lite/__tests__/useAsObservableSource.deprecated.test.tsx

@@ -126,19 +126,19 @@ describe("base useAsObservableSource should work", () => {
         const { container } = render(<Parent />)
 
         expect(container.querySelector("span")!.innerHTML).toBe("10")
-        expect(counterRender).toBe(1)
+        expect(counterRender).toBe(2)
 
         act(() => {
             ;(container.querySelector("#inc")! as any).click()
         })
         expect(container.querySelector("span")!.innerHTML).toBe("11")
-        expect(counterRender).toBe(2)
+        expect(counterRender).toBe(3)
 
         act(() => {
             ;(container.querySelector("#incmultiplier")! as any).click()
         })
         expect(container.querySelector("span")!.innerHTML).toBe("22")
-        expect(counterRender).toBe(4) // TODO: should be 3
+        expect(counterRender).toBe(5) // TODO: should be 3
     })
 })
 
@@ -271,7 +271,7 @@ describe("combining observer with props and stores", () => {
             store.x = 10
         })
 
-        expect(renderedValues).toEqual([10, 15, 15, 20]) // TODO: should have one 15 less
+        expect(renderedValues).toEqual([10, 10, 15, 15, 20]) // TODO: should have one 15 less
 
         // TODO: re-enable this line. When debugging, the correct value is returned from render,
         // which is also visible with renderedValues, however, querying the dom doesn't show the correct result

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

@@ -192,19 +192,19 @@ describe("base useAsObservableSource should work", () => {
         const { container } = render(<Parent />)
 
         expect(container.querySelector("span")!.innerHTML).toBe("10")
-        expect(counterRender).toBe(1)
+        expect(counterRender).toBe(2)
 
         act(() => {
             ;(container.querySelector("#inc")! as any).click()
         })
         expect(container.querySelector("span")!.innerHTML).toBe("11")
-        expect(counterRender).toBe(2)
+        expect(counterRender).toBe(3)
 
         act(() => {
             ;(container.querySelector("#incmultiplier")! as any).click()
         })
         expect(container.querySelector("span")!.innerHTML).toBe("22")
-        expect(counterRender).toBe(4) // TODO: should be 3
+        expect(counterRender).toBe(5) // TODO: should be 3
     })
 })
 
@@ -337,7 +337,7 @@ describe("combining observer with props and stores", () => {
             store.x = 10
         })
 
-        expect(renderedValues).toEqual([10, 15, 15, 20]) // TODO: should have one 15 less
+        expect(renderedValues).toEqual([10, 10, 15, 15, 20]) // TODO: should have one 15 less
 
         expect(container.querySelector("div")!.textContent).toBe("20")
     })

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

@@ -116,7 +116,7 @@ describe("is used to keep observable within component body", () => {
         fireEvent.click(div)
         expect(div.textContent).toBe("3-2")
 
-        expect(renderCount).toBe(3)
+        expect(renderCount).toBe(4)
     })
 
     it("actions can be used", () => {
@@ -418,19 +418,19 @@ describe("is used to keep observable within component body", () => {
             const { container } = render(<Parent />)
 
             expect(container.querySelector("span")!.innerHTML).toBe("10")
-            expect(counterRender).toBe(1)
+            expect(counterRender).toBe(2)
 
             act(() => {
                 ;(container.querySelector("#inc")! as any).click()
             })
             expect(container.querySelector("span")!.innerHTML).toBe("11")
-            expect(counterRender).toBe(2)
+            expect(counterRender).toBe(3)
 
             act(() => {
                 ;(container.querySelector("#incmultiplier")! as any).click()
             })
             expect(container.querySelector("span")!.innerHTML).toBe("22")
-            expect(counterRender).toBe(4) // TODO: should be 3
+            expect(counterRender).toBe(5) // TODO: should be 3
         })
     })
 })

packages/mobx-react-lite/__tests__/useLocalStore.deprecated.test.tsx

@@ -119,8 +119,8 @@ describe("is used to keep observable within component body", () => {
         fireEvent.click(div)
         expect(div.textContent).toBe("3-2")
 
-        // though render 3 times, mobx.observable only called once
-        expect(renderCount).toBe(3)
+        // though render 4 times, mobx.observable only called once
+        expect(renderCount).toBe(4)
     })
 
     it("actions can be used", () => {
@@ -424,19 +424,19 @@ describe("is used to keep observable within component body", () => {
             const { container } = render(<Parent />)
 
             expect(container.querySelector("span")!.innerHTML).toBe("10")
-            expect(counterRender).toBe(1)
+            expect(counterRender).toBe(2)
 
             act(() => {
                 ;(container.querySelector("#inc")! as any).click()
             })
             expect(container.querySelector("span")!.innerHTML).toBe("11")
-            expect(counterRender).toBe(2)
+            expect(counterRender).toBe(3)
 
             act(() => {
                 ;(container.querySelector("#incmultiplier")! as any).click()
             })
             expect(container.querySelector("span")!.innerHTML).toBe("22")
-            expect(counterRender).toBe(4) // TODO: should be 3
+            expect(counterRender).toBe(5) // TODO: should be 3
         })
     })
 })

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

@@ -13,10 +13,11 @@ export function useLocalStore<TStore extends Record<string, any>, TSource extend
     initializer: (source?: TSource) => TStore,
     current?: TSource
 ): TStore {
-    if ("production" !== process.env.NODE_ENV)
+    if ("production" !== process.env.NODE_ENV) {
         useDeprecated(
             "[mobx-react-lite] 'useLocalStore' is deprecated, use 'useLocalObservable' instead."
         )
+    }
     const source = current && useAsObservableSource(current)
     return useState(() => observable(initializer(source), undefined, { autoBind: true }))[0]
 }

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

@@ -12,7 +12,7 @@ const getServerSnapshot = () => {}
 // otherwise it will prevent GC and therefore reaction disposal via FinalizationRegistry.
 type ObserverAdministration = {
     reaction: Reaction | null // also serves as disposed flag
-    forceUpdate: Function | null // also serves as mounted flag
+    onStoreChange: Function | null // also serves as mounted flag
     // BC: we will use local state version if global isn't available.
     // It should behave as previous implementation - tearing is still present,
     // because there is no cross component synchronization,
@@ -27,18 +27,18 @@ type ObserverAdministration = {
 const mobxGlobalState = _getGlobalState()
 
 // BC
-const globalStateVersionIsAvailable = typeof mobxGlobalState.globalVersion !== "undefined"
+const globalStateVersionIsAvailable = typeof mobxGlobalState.stateVersion !== "undefined"
 
 function createReaction(adm: ObserverAdministration) {
     adm.reaction = new Reaction(`observer${adm.name}`, () => {
         if (!globalStateVersionIsAvailable) {
             // BC
             adm.stateVersion = Symbol()
         }
-        // Force update won't be avaliable until the component "mounts".
+        // onStoreChange won't be avaliable until the component "mounts".
         // If state changes in between initial render and mount,
         // `useSyncExternalStore` should handle that by checking the state version and issuing update.
-        adm.forceUpdate?.()
+        adm.onStoreChange?.()
     })
 }
 
@@ -49,28 +49,34 @@ export function useObserver<T>(render: () => T, baseComponentName: string = "obs
 
     const admRef = React.useRef<ObserverAdministration | null>(null)
 
+    // Provides ability to force component update without changing state version
+    const [, forceUpdate] = React.useState<Symbol>()
+
     if (!admRef.current) {
         // First render
         const adm: ObserverAdministration = {
             reaction: null,
-            forceUpdate: null,
+            onStoreChange: null,
             stateVersion: Symbol(),
             name: baseComponentName,
             subscribe(onStoreChange: () => void) {
                 // Do NOT access admRef here!
                 observerFinalizationRegistry.unregister(adm)
-                adm.forceUpdate = onStoreChange
+                adm.onStoreChange = onStoreChange
                 if (!adm.reaction) {
-                    // We've lost our reaction and therefore all subscriptions.
+                    // We've lost our reaction and therefore all subscriptions, occurs when:
+                    // 1. Timer based finalization registry disposed reaction before component mounted.
+                    // 2. React "re-mounts" same component without calling render in between (typically <StrictMode>).
                     // We have to recreate reaction and schedule re-render to recreate subscriptions,
                     // even if state did not change.
                     createReaction(adm)
-                    adm.forceUpdate()
+                    // `onStoreChange` won't force update if subsequent `getSnapshot` returns same value.
+                    forceUpdate(Symbol())
                 }
 
                 return () => {
                     // Do NOT access admRef here!
-                    adm.forceUpdate = null
+                    adm.onStoreChange = null
                     adm.reaction?.dispose()
                     adm.reaction = null
                 }

packages/mobx-react/README.md

@@ -61,6 +61,8 @@ When using component classes, `this.props` and `this.state` will be made observa
 
 `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.
+
 See the [MobX](https://mobx.js.org/react-integration.html#react-integration) documentation for more details.
 
 ```javascript

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

@@ -2,22 +2,7 @@
 
 exports[`#3492 should not cause warning by calling forceUpdate on uncommited components 1`] = `[MockFunction]`;
 
-exports[`Redeclaring an existing observer component as an observer should log a warning 1`] = `
-[MockFunction] {
-  "calls": Array [
-    Array [
-      "The provided component class (AlreadyObserver)
-                has already been declared as an observer component.",
-    ],
-  ],
-  "results": Array [
-    Object {
-      "type": "return",
-      "value": undefined,
-    },
-  ],
-}
-`;
+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__/hooks.test.tsx

@@ -93,6 +93,7 @@ test("computed properties result in double render when using observer instead of
     expect(seen).toEqual([
         "parent",
         0,
+        0,
         "parent",
         2,
         2 // should contain "2" only once! But with hooks, one update is scheduled based the fact that props change, the other because the observable source changed.