Remove useSyncExternalStore in favor of useState (#10377)

remix-run · Apr 21, 2023 · 3efa5d0 · 3efa5d0
1 parent 2821817
commit 3efa5d0
Show file tree

Hide file tree

Showing 8 changed files with 38 additions and 250 deletions.
diff --git a/.changeset/remove-use-sync-external-store.md b/.changeset/remove-use-sync-external-store.md
@@ -0,0 +1,6 @@
+---
+"react-router": patch
+"react-router-dom": patch
+---
+
+Switched from `useSyncExternalStore` to `useState` for internal `@remix-run/router` router state syncing in `<RouterProvider>`. We found some [subtle bugs](https://codesandbox.io/s/use-sync-external-store-loop-9g7b81) where router state updates got propagated _before_ other normal `useState` updates, which could lead to footguns in `useEffect` calls.
diff --git a/package.json b/package.json
@@ -108,10 +108,10 @@
       "none": "45.8 kB"
     },
     "packages/react-router/dist/react-router.production.min.js": {
-      "none": "13.6 kB"
+      "none": "12.9 kB"
     },
     "packages/react-router/dist/umd/react-router.production.min.js": {
-      "none": "15.9 kB"
+      "none": "15.3 kB"
     },
     "packages/react-router-dom/dist/react-router-dom.production.min.js": {
       "none": "12 kB"

diff --git a/packages/react-router-dom/__tests__/special-characters-test.tsx b/packages/react-router-dom/__tests__/special-characters-test.tsx
@@ -828,19 +828,8 @@ describe("special character tests", () => {
     });
 
     describe("browser routers", () => {
-      let testWindow: Window;
-
-      beforeEach(() => {
-        // Need to use our own custom DOM in order to get a working history
-        const dom = new JSDOM(`<!DOCTYPE html><html><body></body></html>`, {
-          url: "https://remix.run/",
-        });
-        testWindow = dom.window as unknown as Window;
-        testWindow.history.pushState({}, "", "/");
-      });
-
       it("encodes characters in BrowserRouter", () => {
-        testWindow.history.replaceState(null, "", "/with space");
+        let testWindow = getWindow("/with space");
 
         let ctx = render(
           <BrowserRouter window={testWindow}>
@@ -857,7 +846,7 @@ describe("special character tests", () => {
       });
 
       it("encodes characters in BrowserRouter (navigate)", () => {
-        testWindow.history.replaceState(null, "", "/");
+        let testWindow = getWindow("/");
 
         function Start() {
           let navigate = useNavigate();
@@ -882,7 +871,7 @@ describe("special character tests", () => {
       });
 
       it("encodes characters in createBrowserRouter", () => {
-        testWindow.history.replaceState(null, "", "/with space");
+        let testWindow = getWindow("/with space");
 
         let router = createBrowserRouter(
           [{ path: "/with space", element: <ShowPath /> }],
@@ -897,7 +886,7 @@ describe("special character tests", () => {
       });
 
       it("encodes characters in createBrowserRouter (navigate)", () => {
-        testWindow.history.replaceState(null, "", "/with space");
+        let testWindow = getWindow("/");
 
         function Start() {
           let navigate = useNavigate();
@@ -923,19 +912,8 @@ describe("special character tests", () => {
     });
 
     describe("hash routers", () => {
-      let testWindow: Window;
-
-      beforeEach(() => {
-        // Need to use our own custom DOM in order to get a working history
-        const dom = new JSDOM(`<!DOCTYPE html><html><body></body></html>`, {
-          url: "https://remix.run/",
-        });
-        testWindow = dom.window as unknown as Window;
-        testWindow.history.pushState({}, "", "/");
-      });
-
       it("encodes characters in HashRouter", () => {
-        testWindow.history.replaceState(null, "", "/#/with space");
+        let testWindow = getWindow("/#/with space");
 
         let ctx = render(
           <HashRouter window={testWindow}>
@@ -953,7 +931,7 @@ describe("special character tests", () => {
       });
 
       it("encodes characters in HashRouter (navigate)", () => {
-        testWindow.history.replaceState(null, "", "/");
+        let testWindow = getWindow("/");
 
         function Start() {
           let navigate = useNavigate();
@@ -979,7 +957,7 @@ describe("special character tests", () => {
       });
 
       it("encodes characters in createHashRouter", () => {
-        testWindow.history.replaceState(null, "", "/#/with space");
+        let testWindow = getWindow("/#/with space");
 
         let router = createHashRouter(
           [{ path: "/with space", element: <ShowPath /> }],
@@ -995,7 +973,7 @@ describe("special character tests", () => {
       });
 
       it("encodes characters in createHashRouter (navigate)", () => {
-        testWindow.history.replaceState(null, "", "/");
+        let testWindow = getWindow("/");
 
         function Start() {
           let navigate = useNavigate();
@@ -1022,3 +1000,13 @@ describe("special character tests", () => {
     });
   });
 });
+
+function getWindow(initialPath) {
+  // Need to use our own custom DOM in order to get a working history
+  const dom = new JSDOM(`<!DOCTYPE html><html><body></body></html>`, {
+    url: "https://remix.run/",
+  });
+  let testWindow = dom.window as unknown as Window;
+  testWindow.history.pushState({}, "", initialPath);
+  return testWindow;
+}
diff --git a/packages/react-router/__tests__/data-memory-router-test.tsx b/packages/react-router/__tests__/data-memory-router-test.tsx
@@ -887,7 +887,7 @@ describe("createMemoryRouter", () => {
 
     spy.mockClear();
     fireEvent.click(screen.getByText("Link to Child"));
-    await new Promise((r) => setTimeout(r, 0));
+    await waitFor(() => screen.getByText("Child"));
 
     expect(getHtml(container)).toMatchInlineSnapshot(`
       "<div>

diff --git a/packages/react-router/lib/components.tsx b/packages/react-router/lib/components.tsx
@@ -5,7 +5,6 @@ import type {
   Location,
   MemoryHistory,
   Router as RemixRouter,
-  RouterState,
   To,
   LazyRouteFunction,
   RelativeRoutingType,
@@ -19,7 +18,6 @@ import {
   stripBasename,
   UNSAFE_warning as warning,
 } from "@remix-run/router";
-import { useSyncExternalStore as useSyncExternalStoreShim } from "./use-sync-external-store-shim";
 
 import type {
   DataRouteObject,
@@ -57,17 +55,17 @@ export function RouterProvider({
   fallbackElement,
   router,
 }: RouterProviderProps): React.ReactElement {
-  let getState = React.useCallback(() => router.state, [router]);
-
-  // Sync router state to our component state to force re-renders
-  let state: RouterState = useSyncExternalStoreShim(
-    router.subscribe,
-    getState,
-    // We have to provide this so React@18 doesn't complain during hydration,
-    // but we pass our serialized hydration data into the router so state here
-    // is already synced with what the server saw
-    getState
-  );
+  let [state, setState] = React.useState(router.state);
+
+  // Need to use a layout effect here so we are subscribed early enough to
+  // pick up on any render-driven redirects/navigations (useEffect/<Navigate>)
+  React.useLayoutEffect(() => {
+    return router.subscribe((newState) => {
+      if (newState !== state) {
+        setState(newState);
+      }
+    });
+  }, [router, state]);
 
   let navigator = React.useMemo((): Navigator => {
     return {

diff --git a/packages/react-router/lib/use-sync-external-store-shim/index.ts b/packages/react-router/lib/use-sync-external-store-shim/index.ts
diff --git a/packages/react-router/lib/use-sync-external-store-shim/useSyncExternalStoreShimClient.ts b/packages/react-router/lib/use-sync-external-store-shim/useSyncExternalStoreShimClient.ts
diff --git a/packages/react-router/lib/use-sync-external-store-shim/useSyncExternalStoreShimServer.ts b/packages/react-router/lib/use-sync-external-store-shim/useSyncExternalStoreShimServer.ts