Better React 18 support (#3590)

.changeset/brown-seals-worry.md

@@ -0,0 +1,5 @@
+---
+"mobx": minor
+---
+
+Better support for React 18: Mobx now keeps track of a global state version, which updates with each mutation.

.changeset/early-terms-bow.md

@@ -0,0 +1,6 @@
+---
+"mobx-react-lite": major
+---
+
+Components now use `useSyncExternalStore`, which should prevent tearing - you have to update mobx, otherwise it should behave as previously.<br>
+Improved displayName/name handling as discussed in #3438.<br>

.changeset/wise-waves-jam.md

@@ -0,0 +1,12 @@
+---
+"mobx-react": major
+---
+
+Functional components now use `useSyncExternalStore`, which should prevent tearing - you have to update mobx, otherwise it should behave as previously.<br>
+Improved displayName/name handling of functional components as discussed in #3438.<br>
+Reactions of uncommited class components are now correctly disposed, fixes #3492.<br>
+Reactions don't notify uncommited class components, avoiding the warning, fixes #3492.<br>
+Removed symbol "polyfill" and replaced with actual Symbols.<br>
+Removed `this.render` replacement detection + warning. `this.render` is no longer configurable/writable (possibly BC).<br>
+Class component instance is no longer exposed as `component[$mobx]["reactcomponent"]` (possibly BC).<br>
+Deprecated `disposeOnUnmount`, it's not compatible with remounting.<br>

.vscode/settings.json

@@ -1,9 +1,11 @@
 {
     "[typescript]": {
+        "editor.defaultFormatter": "esbenp.prettier-vscode",
         "editor.formatOnSave": true,
         "editor.formatOnPaste": false
     },
     "[javascript]": {
+        "editor.defaultFormatter": "esbenp.prettier-vscode",
         "editor.formatOnSave": true,
         "editor.formatOnPaste": false
     },

packages/mobx-react-lite/__tests__/api.test.ts

@@ -18,7 +18,8 @@ test("correct api should be exposed", function () {
             "useObserver",
             "isObserverBatched",
             "observerBatching",
-            "useStaticRendering"
+            "useStaticRendering",
+            "_observerFinalizationRegistry"
         ].sort()
     )
 })

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

@@ -618,12 +618,15 @@ it("should hoist known statics only", () => {
     expect(wrapped.render).toBe(undefined)
 })
 
-it("should have the correct displayName", () => {
-    const TestComponent = observer(function MyComponent() {
+it("should inherit original name/displayName #3438", () => {
+    function Name() {
         return null
-    })
+    }
+    Name.displayName = "DisplayName"
+    const TestComponent = observer(Name)
 
-    expect((TestComponent as any).type.displayName).toBe("MyComponent")
+    expect((TestComponent as any).type.name).toBe("Name")
+    expect((TestComponent as any).type.displayName).toBe("DisplayName")
 })
 
 test("parent / childs render in the right order", done => {

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

@@ -1,4 +1,4 @@
-import { cleanup, render } from "@testing-library/react"
+import { cleanup, render, waitFor } from "@testing-library/react"
 import * as mobx from "mobx"
 import * as React from "react"
 import { useObserver } from "../src/useObserver"
@@ -43,10 +43,17 @@ test("uncommitted components should not leak observations", async () => {
 
     // Allow gc to kick in in case to let finalization registry cleanup
     gc()
-    await sleep(50)
-
-    // count1 should still be being observed by Component1,
-    // but count2 should have had its reaction cleaned up.
-    expect(count1IsObserved).toBeTruthy()
-    expect(count2IsObserved).toBeFalsy()
+    // Can take a while (especially on CI) before gc actually calls the registry
+    await waitFor(
+        () => {
+            // count1 should still be being observed by Component1,
+            // but count2 should have had its reaction cleaned up.
+            expect(count1IsObserved).toBeTruthy()
+            expect(count2IsObserved).toBeFalsy()
+        },
+        {
+            timeout: 2000,
+            interval: 200
+        }
+    )
 })

packages/mobx-react-lite/package.json

@@ -38,7 +38,7 @@
     "homepage": "https://mobx.js.org",
     "dependencies": {},
     "peerDependencies": {
-        "mobx": "^6.1.0",
+        "mobx": "^6.9.0",
         "react": "^16.8.0 || ^17 || ^18"
     },
     "peerDependenciesMeta": {
@@ -51,7 +51,8 @@
     },
     "devDependencies": {
         "mobx": "^6.8.0",
-        "expose-gc": "^1.0.0"
+        "expose-gc": "^1.0.0",
+        "use-sync-external-store": "^1.2.0"
     },
     "keywords": [
         "mobx",

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

@@ -16,6 +16,7 @@ export { useLocalObservable } from "./useLocalObservable"
 export { useLocalStore } from "./useLocalStore"
 export { useAsObservableSource } from "./useAsObservableSource"
 
+export { observerFinalizationRegistry as _observerFinalizationRegistry }
 export const clearTimers = observerFinalizationRegistry["finalizeAllImmediately"] ?? (() => {})
 
 export function useObserver<T>(fn: () => T, baseComponentName: string = "observed"): T {

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

@@ -104,11 +104,13 @@ export function observer<P extends object, TRef = {}>(
         return useObserver(() => render(props, ref), baseComponentName)
     }
 
-    // Don't set `displayName` for anonymous components,
-    // so the `displayName` can be customized by user, see #3192.
-    if (baseComponentName !== "") {
-        ;(observerComponent as React.FunctionComponent).displayName = baseComponentName
-    }
+    // Inherit original name and displayName, see #3438
+    ;(observerComponent as React.FunctionComponent).displayName = baseComponent.displayName
+    Object.defineProperty(observerComponent, "name", {
+        value: baseComponent.name,
+        writable: true,
+        configurable: true
+    })
 
     // Support legacy context: `contextTypes` must be applied before `memo`
     if ((baseComponent as any).contextTypes) {
@@ -136,7 +138,7 @@ export function observer<P extends object, TRef = {}>(
             set() {
                 throw new Error(
                     `[mobx-react-lite] \`${
-                        this.displayName || this.type?.displayName || "Component"
+                        this.displayName || this.type?.displayName || this.type?.name || "Component"
                     }.contextTypes\` must be set before applying \`observer\`.`
                 )
             }

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

@@ -1,129 +1,115 @@
-import { Reaction } from "mobx"
+import { Reaction, _getGlobalState } from "mobx"
 import React from "react"
 import { printDebugValue } from "./utils/printDebugValue"
-import { observerFinalizationRegistry } from "./utils/observerFinalizationRegistry"
 import { isUsingStaticRendering } from "./staticRendering"
+import { observerFinalizationRegistry } from "./utils/observerFinalizationRegistry"
+import { useSyncExternalStore } from "use-sync-external-store/shim"
 
-function observerComponentNameFor(baseComponentName: string) {
-    return `observer${baseComponentName}`
-}
-
+// Do not store `admRef` (even as part of a closure!) on this object,
+// otherwise it will prevent GC and therefore reaction disposal via FinalizationRegistry.
 type ObserverAdministration = {
-    /** The Reaction created during first render, which may be leaked */
-    reaction: Reaction | null
-
-    /**
-     * Whether the component has yet completed mounting (for us, whether
-     * its useEffect has run)
-     */
-    mounted: boolean
-
-    /**
-     * Whether the observables that the component is tracking changed between
-     * the first render and the first useEffect.
-     */
-    changedBeforeMount: boolean
+    reaction: Reaction | null // also serves as disposed flag
+    forceUpdate: 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,
+    // but we can use `useSyncExternalStore` API.
+    stateVersion: any
+    name: string
+    // These don't depend on state/props, therefore we can keep them here instead of `useCallback`
+    subscribe: Parameters<typeof React.useSyncExternalStore>[0]
+    getSnapshot: Parameters<typeof React.useSyncExternalStore>[1]
 }
 
-/**
- * We use class to make it easier to detect in heap snapshots by name
- */
-class ObjectToBeRetainedByReact {}
+const mobxGlobalState = _getGlobalState()
+
+// BC
+const globalStateVersionIsAvailable = typeof mobxGlobalState.globalVersion !== "undefined"
 
-function objectToBeRetainedByReactFactory() {
-    return new ObjectToBeRetainedByReact()
+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".
+        // If state changes in between initial render and mount,
+        // `useSyncExternalStore` should handle that by checking the state version and issuing update.
+        adm.forceUpdate?.()
+    })
 }
 
-export function useObserver<T>(fn: () => T, baseComponentName: string = "observed"): T {
+export function useObserver<T>(render: () => T, baseComponentName: string = "observed"): T {
     if (isUsingStaticRendering()) {
-        return fn()
+        return render()
     }
 
-    const [objectRetainedByReact] = React.useState(objectToBeRetainedByReactFactory)
-    // Force update, see #2982
-    const [, setState] = React.useState()
-    const forceUpdate = () => setState([] as any)
-
-    // StrictMode/ConcurrentMode/Suspense may mean that our component is
-    // rendered and abandoned multiple times, so we need to track leaked
-    // Reactions.
     const admRef = React.useRef<ObserverAdministration | null>(null)
 
     if (!admRef.current) {
         // First render
-        admRef.current = {
+        const adm: ObserverAdministration = {
             reaction: null,
-            mounted: false,
-            changedBeforeMount: false
+            forceUpdate: null,
+            stateVersion: Symbol(),
+            name: baseComponentName,
+            subscribe(onStoreChange: () => void) {
+                // Do NOT access admRef here!
+                observerFinalizationRegistry.unregister(adm)
+                adm.forceUpdate = onStoreChange
+                if (!adm.reaction) {
+                    // We've lost our reaction and therefore all subscriptions.
+                    // We have to recreate reaction and schedule re-render to recreate subscriptions,
+                    // even if state did not change.
+                    createReaction(adm)
+                    adm.forceUpdate()
+                }
+
+                return () => {
+                    // Do NOT access admRef here!
+                    adm.forceUpdate = null
+                    adm.reaction?.dispose()
+                    adm.reaction = null
+                }
+            },
+            getSnapshot() {
+                // Do NOT access admRef here!
+                return globalStateVersionIsAvailable
+                    ? mobxGlobalState.stateVersion
+                    : adm.stateVersion
+            }
         }
+
+        admRef.current = adm
     }
 
     const adm = admRef.current!
 
     if (!adm.reaction) {
-        // First render or component was not committed and reaction was disposed by registry
-        adm.reaction = new Reaction(observerComponentNameFor(baseComponentName), () => {
-            // Observable has changed, meaning we want to re-render
-            // BUT if we're a component that hasn't yet got to the useEffect()
-            // stage, we might be a component that _started_ to render, but
-            // got dropped, and we don't want to make state changes then.
-            // (It triggers warnings in StrictMode, for a start.)
-            if (adm.mounted) {
-                // We have reached useEffect(), so we're mounted, and can trigger an update
-                forceUpdate()
-            } else {
-                // We haven't yet reached useEffect(), so we'll need to trigger a re-render
-                // when (and if) useEffect() arrives.
-                adm.changedBeforeMount = true
-            }
-        })
-
-        observerFinalizationRegistry.register(objectRetainedByReact, adm, adm)
+        // First render or reaction was disposed by registry before subscribe
+        createReaction(adm)
+        // StrictMode/ConcurrentMode/Suspense may mean that our component is
+        // rendered and abandoned multiple times, so we need to track leaked
+        // Reactions.
+        observerFinalizationRegistry.register(admRef, adm, adm)
     }
 
-    React.useDebugValue(adm.reaction, printDebugValue)
-
-    React.useEffect(() => {
-        observerFinalizationRegistry.unregister(adm)
-
-        adm.mounted = true
+    React.useDebugValue(adm.reaction!, printDebugValue)
 
-        if (adm.reaction) {
-            if (adm.changedBeforeMount) {
-                // Got a change before mount, force an update
-                adm.changedBeforeMount = false
-                forceUpdate()
-            }
-        } else {
-            // The reaction we set up in our render has been disposed.
-            // This can be due to bad timings of renderings, e.g. our
-            // component was paused for a _very_ long time, and our
-            // reaction got cleaned up
-
-            // Re-create the reaction
-            adm.reaction = new Reaction(observerComponentNameFor(baseComponentName), () => {
-                // We've definitely already been mounted at this point
-                forceUpdate()
-            })
-            forceUpdate()
-        }
-
-        return () => {
-            adm.reaction!.dispose()
-            adm.reaction = null
-            adm.mounted = false
-            adm.changedBeforeMount = false
-        }
-    }, [])
+    useSyncExternalStore(
+        // Both of these must be stable, otherwise it would keep resubscribing every render.
+        adm.subscribe,
+        adm.getSnapshot
+    )
 
     // render the original component, but have the
     // reaction track the observables, so that rendering
     // can be invalidated (see above) once a dependency changes
-    let rendering!: T
+    let renderResult!: T
     let exception
-    adm.reaction.track(() => {
+    adm.reaction!.track(() => {
         try {
-            rendering = fn()
+            renderResult = render()
         } catch (e) {
             exception = e
         }
@@ -133,5 +119,5 @@ export function useObserver<T>(fn: () => T, baseComponentName: string = "observe
         throw exception // re-throw any exceptions caught during rendering
     }
 
-    return rendering
+    return renderResult
 }