Fix mobxjs#3728: observables initialization should not update state v…

…ersion (mobxjs#3732)
pranavrajs · Aug 26, 2023 · 3ceeb86 · 3ceeb86
1 parent d813746
commit 3ceeb86
Show file tree

Hide file tree

Showing 26 changed files with 352 additions and 131 deletions.
diff --git a/.changeset/seven-poems-sneeze.md b/.changeset/seven-poems-sneeze.md
@@ -0,0 +1,5 @@
+---
+"mobx-react-lite": patch
+---
+
+fix: #3734: `isolateGlobalState: true` makes observer stop to react to store changes
diff --git a/.changeset/spotty-emus-hear.md b/.changeset/spotty-emus-hear.md
@@ -0,0 +1,7 @@
+---
+"mobx": patch
+---
+
+-   fix: #3728: Observable initialization updates state version.
+-   fix: Observable set initialization violates `enforceActions: "always"`.
+-   fix: Changing keys of observable object does not respect `enforceActions`.
diff --git a/packages/mobx-react-lite/__tests__/observer.test.tsx b/packages/mobx-react-lite/__tests__/observer.test.tsx
@@ -1107,3 +1107,24 @@ test("StrictMode #3671", async () => {
     )
     expect(container).toHaveTextContent("1")
 })
+
+test("`isolateGlobalState` shouldn't break reactivity #3734", async () => {
+    mobx.configure({ isolateGlobalState: true })
+
+    const o = mobx.observable({ x: 0 })
+
+    const Cmp = observer(() => o.x as any)
+
+    const { container, unmount } = render(<Cmp />)
+
+    expect(container).toHaveTextContent("0")
+    act(
+        mobx.action(() => {
+            o.x++
+        })
+    )
+    expect(container).toHaveTextContent("1")
+    unmount()
+
+    mobx._resetGlobalState()
+})
diff --git a/packages/mobx-react-lite/__tests__/strictAndConcurrentModeUsingFinalizationRegistry.test.tsx b/packages/mobx-react-lite/__tests__/strictAndConcurrentModeUsingFinalizationRegistry.test.tsx
@@ -42,9 +42,8 @@ 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))
+    await new Promise(resolve => setTimeout(resolve, 100))
     gc()
-    await new Promise(resolve => setTimeout(resolve, 0))
     // Can take a while (especially on CI) before gc actually calls the registry
     await waitFor(
         () => {
@@ -54,7 +53,7 @@ test("uncommitted components should not leak observations", async () => {
             expect(count2IsObserved).toBeFalsy()
         },
         {
-            timeout: 10_000,
+            timeout: 15_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(2)
+        expect(counterRender).toBe(1)
 
         act(() => {
             ;(container.querySelector("#inc")! as any).click()
         })
         expect(container.querySelector("span")!.innerHTML).toBe("11")
-        expect(counterRender).toBe(3)
+        expect(counterRender).toBe(2)
 
         act(() => {
             ;(container.querySelector("#incmultiplier")! as any).click()
         })
         expect(container.querySelector("span")!.innerHTML).toBe("22")
-        expect(counterRender).toBe(5) // TODO: should be 3
+        expect(counterRender).toBe(4) // One from props, second from updating local observable (setState during render)
     })
 })
 
@@ -271,7 +271,12 @@ describe("combining observer with props and stores", () => {
             store.x = 10
         })
 
-        expect(renderedValues).toEqual([10, 10, 15, 15, 20]) // TODO: should have one 15 less
+        expect(renderedValues).toEqual([
+            10,
+            15, // props change
+            15, // local observable change (setState during render)
+            20
+        ])
 
         // 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(2)
+        expect(counterRender).toBe(1)
 
         act(() => {
             ;(container.querySelector("#inc")! as any).click()
         })
         expect(container.querySelector("span")!.innerHTML).toBe("11")
-        expect(counterRender).toBe(3)
+        expect(counterRender).toBe(2)
 
         act(() => {
             ;(container.querySelector("#incmultiplier")! as any).click()
         })
         expect(container.querySelector("span")!.innerHTML).toBe("22")
-        expect(counterRender).toBe(5) // TODO: should be 3
+        expect(counterRender).toBe(4) // One from props, second from updating local observable with effect
     })
 })
 
@@ -337,7 +337,12 @@ describe("combining observer with props and stores", () => {
             store.x = 10
         })
 
-        expect(renderedValues).toEqual([10, 10, 15, 15, 20]) // TODO: should have one 15 less
+        expect(renderedValues).toEqual([
+            10,
+            15, // props change
+            15, // local observable change during render (localStore.y = props.y)
+            20
+        ])
 
         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(4)
+        expect(renderCount).toBe(3)
     })
 
     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(2)
+            expect(counterRender).toBe(1)
 
             act(() => {
                 ;(container.querySelector("#inc")! as any).click()
             })
             expect(container.querySelector("span")!.innerHTML).toBe("11")
-            expect(counterRender).toBe(3)
+            expect(counterRender).toBe(2)
 
             act(() => {
                 ;(container.querySelector("#incmultiplier")! as any).click()
             })
             expect(container.querySelector("span")!.innerHTML).toBe("22")
-            expect(counterRender).toBe(5) // TODO: should be 3
+            expect(counterRender).toBe(4) // One from props, second from updating local observable with effect
         })
     })
 })

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,7 @@ describe("is used to keep observable within component body", () => {
         fireEvent.click(div)
         expect(div.textContent).toBe("3-2")
 
-        // though render 4 times, mobx.observable only called once
-        expect(renderCount).toBe(4)
+        expect(renderCount).toBe(3)
     })
 
     it("actions can be used", () => {
@@ -424,19 +423,19 @@ describe("is used to keep observable within component body", () => {
             const { container } = render(<Parent />)
 
             expect(container.querySelector("span")!.innerHTML).toBe("10")
-            expect(counterRender).toBe(2)
+            expect(counterRender).toBe(1)
 
             act(() => {
                 ;(container.querySelector("#inc")! as any).click()
             })
             expect(container.querySelector("span")!.innerHTML).toBe("11")
-            expect(counterRender).toBe(3)
+            expect(counterRender).toBe(2)
 
             act(() => {
                 ;(container.querySelector("#incmultiplier")! as any).click()
             })
             expect(container.querySelector("span")!.innerHTML).toBe("22")
-            expect(counterRender).toBe(5) // TODO: should be 3
+            expect(counterRender).toBe(4) // One from props, second from updating source (setState during render)
         })
     })
 })
diff --git a/packages/mobx-react-lite/src/useObserver.ts b/packages/mobx-react-lite/src/useObserver.ts
@@ -24,10 +24,8 @@ type ObserverAdministration = {
     getSnapshot: Parameters<typeof React.useSyncExternalStore>[1]
 }
 
-const mobxGlobalState = _getGlobalState()
-
 // BC
-const globalStateVersionIsAvailable = typeof mobxGlobalState.stateVersion !== "undefined"
+const globalStateVersionIsAvailable = typeof _getGlobalState().stateVersion !== "undefined"
 
 function createReaction(adm: ObserverAdministration) {
     adm.reaction = new Reaction(`observer${adm.name}`, () => {
@@ -84,7 +82,7 @@ export function useObserver<T>(render: () => T, baseComponentName: string = "obs
             getSnapshot() {
                 // Do NOT access admRef here!
                 return globalStateVersionIsAvailable
-                    ? mobxGlobalState.stateVersion
+                    ? _getGlobalState().stateVersion
                     : adm.stateVersion
             }
         }

diff --git a/packages/mobx-react/__tests__/finalizationRegistry.tsx b/packages/mobx-react/__tests__/finalizationRegistry.tsx
@@ -8,12 +8,6 @@ import { observer } from "../src"
 
 afterEach(cleanup)
 
-function sleep(time: number) {
-    return new Promise<void>(res => {
-        setTimeout(res, time)
-    })
-}
-
 // TODO remove once https://github.com/mobxjs/mobx/pull/3620 is merged.
 declare class WeakRef<T> {
     constructor(object: T)
@@ -80,9 +74,12 @@ test("should not prevent GC of uncomitted components", async () => {
     expect(aConstructorCount).toBe(2)
     expect(aMountCount).toBe(1)
 
+    await Promise.resolve()
     gc()
-    await sleep(50)
-    expect(firstARef!.deref()).toBeUndefined()
+    await waitFor(() => expect(firstARef!.deref()).toBeUndefined(), {
+        timeout: 10_000,
+        interval: 150
+    })
 
     unmount()
 })
diff --git a/packages/mobx-react/__tests__/hooks.test.tsx b/packages/mobx-react/__tests__/hooks.test.tsx
@@ -93,10 +93,9 @@ 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.
+        2, // props changed
+        2 // observable source changed (setState during render)
     ])
     expect(container).toHaveTextContent("2")
 })
diff --git a/packages/mobx/__tests__/v5/base/extendObservable.js b/packages/mobx/__tests__/v5/base/extendObservable.js
@@ -7,7 +7,9 @@ import {
     isObservableProp,
     isComputedProp,
     isAction,
-    extendObservable
+    extendObservable,
+    observable,
+    reaction
 } from "../../../src/mobx"
 
 test("extendObservable should work", function () {
@@ -160,3 +162,15 @@ test("extendObservable should apply specified decorators", function () {
     expect(isAction(box.someFunc)).toBe(true)
     expect(box.someFunc()).toEqual(2)
 })
+
+test("extendObservable notifies about added keys", () => {
+    let reactionCalled = false
+    const o = observable({})
+    const disposeReaction = reaction(
+        () => Object.keys(o),
+        () => (reactionCalled = true)
+    )
+    extendObservable(o, { x: 0 })
+    expect(reactionCalled).toBe(true)
+    disposeReaction()
+})