Fix react-strict mode support, remove auto observability of this.prop…

…s / this.state / this.context in class components (#3718) * wip * changeset * wip * fix tests * style * wip * wip * wip * wip * wip * wip * wip * wip * fix comment * Added docs * Fixed constructor -> componentDidMount in docs * Fixed failing unit tests? * slightly more robust gc --------- Co-authored-by: Jan Plaček <[email protected]>
mobxjs · Jul 12, 2023 · 473cb3f · 473cb3f
1 parent 27efa3c
commit 473cb3f
Show file tree

Hide file tree

Showing 16 changed files with 491 additions and 334 deletions.
diff --git a/.changeset/flat-pumas-cross.md b/.changeset/flat-pumas-cross.md
@@ -0,0 +1,8 @@
+---
+"mobx-react-lite": patch
+"mobx-react": major
+---
+
+-   Fixed `observer` in `StrictMode` #3671
+-   **[BREAKING CHANGE]** 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. For details see https://github.com/mobxjs/mobx/blob/main/packages/mobx-react/README.md#note-on-using-props-and-state-in-derivations
+-   Extending or applying `observer` classes is now explicitly forbidden
diff --git a/docs/react-integration.md b/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>
 

diff --git a/packages/mobx-react-lite/__tests__/observer.test.tsx b/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")
+})
diff --git a/packages/mobx-react-lite/__tests__/strictAndConcurrentModeUsingFinalizationRegistry.test.tsx b/packages/mobx-react-lite/__tests__/strictAndConcurrentModeUsingFinalizationRegistry.test.tsx
@@ -14,6 +14,7 @@ expect(observerFinalizationRegistry).toBeInstanceOf(globalThis.FinalizationRegis
 afterEach(cleanup)
 
 test("uncommitted components should not leak observations", async () => {
+    jest.setTimeout(30_000)
     const store = mobx.observable({ count1: 0, count2: 0 })
 
     // Track whether counts are observed
@@ -41,7 +42,9 @@ test("uncommitted components should not leak observations", async () => {
     )
 
     // Allow gc to kick in in case to let finalization registry cleanup
+    await new Promise(resolve => setTimeout(resolve, 0))
     gc()
+    await new Promise(resolve => setTimeout(resolve, 0))
     // Can take a while (especially on CI) before gc actually calls the registry
     await waitFor(
         () => {
@@ -51,7 +54,7 @@ test("uncommitted components should not leak observations", async () => {
             expect(count2IsObserved).toBeFalsy()
         },
         {
-            timeout: 2000,
+            timeout: 10_000,
             interval: 200
         }
     )

diff --git a/packages/mobx-react-lite/__tests__/useAsObservableSource.deprecated.test.tsx b/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

diff --git a/packages/mobx-react-lite/__tests__/useAsObservableSource.test.tsx b/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")
     })

diff --git a/packages/mobx-react-lite/__tests__/useLocalObservable.test.tsx b/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
         })
     })
 })

diff --git a/packages/mobx-react-lite/__tests__/useLocalStore.deprecated.test.tsx b/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
         })
     })
 })
diff --git a/packages/mobx-react-lite/src/useLocalStore.ts b/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]
 }
diff --git a/packages/mobx-react-lite/src/useObserver.ts b/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
                 }