diff --git a/.changeset/remove-use-sync-external-store.md b/.changeset/remove-use-sync-external-store.md new file mode 100644 index 0000000000..5760c0e4b1 --- /dev/null +++ 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 ``. 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 index 43ac0c3dd9..227697dde6 100644 --- 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 index 9c6f714fd8..0f6a5ddb6e 100644 --- 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(``, { - 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( @@ -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: }], @@ -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(``, { - 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( @@ -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: }], @@ -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(``, { + 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 index b84537cf71..303581da20 100644 --- 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(` "
diff --git a/packages/react-router/lib/components.tsx b/packages/react-router/lib/components.tsx index e5c9949dc8..0c2186c830 100644 --- 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/) + 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 deleted file mode 100644 index 0ab83f7d95..0000000000 --- a/packages/react-router/lib/use-sync-external-store-shim/index.ts +++ /dev/null @@ -1,32 +0,0 @@ -/** - * Inlined into the react-router repo since use-sync-external-store does not - * provide a UMD-compatible package, so we need this to be able to distribute - * UMD react-router bundles - */ - -/** - * Copyright (c) Facebook, Inc. and its affiliates. - * - * This source code is licensed under the MIT license found in the - * LICENSE file in the root directory of this source tree. - * - * @flow - */ - -import * as React from "react"; - -import { useSyncExternalStore as client } from "./useSyncExternalStoreShimClient"; -import { useSyncExternalStore as server } from "./useSyncExternalStoreShimServer"; - -const canUseDOM: boolean = !!( - typeof window !== "undefined" && - typeof window.document !== "undefined" && - typeof window.document.createElement !== "undefined" -); -const isServerEnvironment = !canUseDOM; -const shim = isServerEnvironment ? server : client; - -export const useSyncExternalStore = - "useSyncExternalStore" in React - ? ((module) => module.useSyncExternalStore)(React) - : shim; 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 deleted file mode 100644 index c2311dccb6..0000000000 --- a/packages/react-router/lib/use-sync-external-store-shim/useSyncExternalStoreShimClient.ts +++ /dev/null @@ -1,152 +0,0 @@ -/** - * Copyright (c) Facebook, Inc. and its affiliates. - * - * This source code is licensed under the MIT license found in the - * LICENSE file in the root directory of this source tree. - */ - -import * as React from "react"; - -/** - * inlined Object.is polyfill to avoid requiring consumers ship their own - * https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/is - */ -function isPolyfill(x: any, y: any) { - return ( - (x === y && (x !== 0 || 1 / x === 1 / y)) || (x !== x && y !== y) // eslint-disable-line no-self-compare - ); -} - -const is: (x: any, y: any) => boolean = - typeof Object.is === "function" ? Object.is : isPolyfill; - -// Intentionally not using named imports because Rollup uses dynamic -// dispatch for CommonJS interop named imports. -const { useState, useEffect, useLayoutEffect, useDebugValue } = React; - -let didWarnOld18Alpha = false; -let didWarnUncachedGetSnapshot = false; - -// Disclaimer: This shim breaks many of the rules of React, and only works -// because of a very particular set of implementation details and assumptions -// -- change any one of them and it will break. The most important assumption -// is that updates are always synchronous, because concurrent rendering is -// only available in versions of React that also have a built-in -// useSyncExternalStore API. And we only use this shim when the built-in API -// does not exist. -// -// Do not assume that the clever hacks used by this hook also work in general. -// The point of this shim is to replace the need for hacks by other libraries. -export function useSyncExternalStore( - subscribe: (fn: () => void) => () => void, - getSnapshot: () => T, - // Note: The shim does not use getServerSnapshot, because pre-18 versions of - // React do not expose a way to check if we're hydrating. So users of the shim - // will need to track that themselves and return the correct value - // from `getSnapshot`. - getServerSnapshot?: () => T -): T { - if (__DEV__) { - if (!didWarnOld18Alpha) { - if ("startTransition" in React) { - didWarnOld18Alpha = true; - console.error( - "You are using an outdated, pre-release alpha of React 18 that " + - "does not support useSyncExternalStore. The " + - "use-sync-external-store shim will not work correctly. Upgrade " + - "to a newer pre-release." - ); - } - } - } - - // Read the current snapshot from the store on every render. Again, this - // breaks the rules of React, and only works here because of specific - // implementation details, most importantly that updates are - // always synchronous. - const value = getSnapshot(); - if (__DEV__) { - if (!didWarnUncachedGetSnapshot) { - const cachedValue = getSnapshot(); - if (!is(value, cachedValue)) { - console.error( - "The result of getSnapshot should be cached to avoid an infinite loop" - ); - didWarnUncachedGetSnapshot = true; - } - } - } - - // Because updates are synchronous, we don't queue them. Instead we force a - // re-render whenever the subscribed state changes by updating an some - // arbitrary useState hook. Then, during render, we call getSnapshot to read - // the current value. - // - // Because we don't actually use the state returned by the useState hook, we - // can save a bit of memory by storing other stuff in that slot. - // - // To implement the early bailout, we need to track some things on a mutable - // object. Usually, we would put that in a useRef hook, but we can stash it in - // our useState hook instead. - // - // To force a re-render, we call forceUpdate({inst}). That works because the - // new object always fails an equality check. - const [{ inst }, forceUpdate] = useState({ inst: { value, getSnapshot } }); - - // Track the latest getSnapshot function with a ref. This needs to be updated - // in the layout phase so we can access it during the tearing check that - // happens on subscribe. - useLayoutEffect(() => { - inst.value = value; - inst.getSnapshot = getSnapshot; - - // Whenever getSnapshot or subscribe changes, we need to check in the - // commit phase if there was an interleaved mutation. In concurrent mode - // this can happen all the time, but even in synchronous mode, an earlier - // effect may have mutated the store. - if (checkIfSnapshotChanged(inst)) { - // Force a re-render. - forceUpdate({ inst }); - } - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [subscribe, value, getSnapshot]); - - useEffect(() => { - // Check for changes right before subscribing. Subsequent changes will be - // detected in the subscription handler. - if (checkIfSnapshotChanged(inst)) { - // Force a re-render. - forceUpdate({ inst }); - } - const handleStoreChange = () => { - // TODO: Because there is no cross-renderer API for batching updates, it's - // up to the consumer of this library to wrap their subscription event - // with unstable_batchedUpdates. Should we try to detect when this isn't - // the case and print a warning in development? - - // The store changed. Check if the snapshot changed since the last time we - // read from the store. - if (checkIfSnapshotChanged(inst)) { - // Force a re-render. - forceUpdate({ inst }); - } - }; - // Subscribe to the store and return a clean-up function. - return subscribe(handleStoreChange); - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [subscribe]); - - useDebugValue(value); - return value; -} - -function checkIfSnapshotChanged(inst: any) { - const latestGetSnapshot = inst.getSnapshot; - const prevValue = inst.value; - try { - const nextValue = latestGetSnapshot(); - return !is(prevValue, nextValue); - } catch (error) { - return true; - } -} 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 deleted file mode 100644 index 9a8050d23d..0000000000 --- a/packages/react-router/lib/use-sync-external-store-shim/useSyncExternalStoreShimServer.ts +++ /dev/null @@ -1,20 +0,0 @@ -/** - * Copyright (c) Facebook, Inc. and its affiliates. - * - * This source code is licensed under the MIT license found in the - * LICENSE file in the root directory of this source tree. - * - * @flow - */ - -export function useSyncExternalStore( - subscribe: (fn: () => void) => () => void, - getSnapshot: () => T, - getServerSnapshot?: () => T -): T { - // Note: The shim does not use getServerSnapshot, because pre-18 versions of - // React do not expose a way to check if we're hydrating. So users of the shim - // will need to track that themselves and return the correct value - // from `getSnapshot`. - return getSnapshot(); -}