Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove useSyncExternalStore in favor of useState #10377

Merged
merged 4 commits into from
Apr 21, 2023
Merged

Remove useSyncExternalStore in favor of useState #10377

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7a5fb11
Remove useSyncExternalStore in favor of useState
brophdawg11 Apr 19, 2023
d234044
Drop bundle
brophdawg11 Apr 20, 2023
8c3431b
Merge branch 'dev' into brophdawg11/remove-uses
brophdawg11 Apr 21, 2023
f3fd217
Merge branch 'dev' into brophdawg11/remove-uses
brophdawg11 Apr 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 6 additions & 0 deletions .changeset/remove-use-sync-external-store.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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.
4 changes: 2 additions & 2 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,10 +108,10 @@
"none": "45.8 kB"
},
"packages/react-router/dist/react-router.production.min.js": {
"none": "13.3 kB"
"none": "12.7 kB"
},
"packages/react-router/dist/umd/react-router.production.min.js": {
"none": "15.6 kB"
"none": "15.0 kB"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a bonus we drop the use-sync-external-store/shim and shave a few bytes

},
"packages/react-router-dom/dist/react-router-dom.production.min.js": {
"none": "12 kB"
Expand Down
48 changes: 18 additions & 30 deletions packages/react-router-dom/__tests__/special-characters-test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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({}, "", "/");
});
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reusing the same window instance across these tests was. ... not smart on my part :)


it("encodes characters in BrowserRouter", () => {
testWindow.history.replaceState(null, "", "/with space");
let testWindow = getWindow("/with space");

let ctx = render(
<BrowserRouter window={testWindow}>
Expand All @@ -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();
Expand All @@ -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 /> }],
Expand All @@ -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();
Expand All @@ -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}>
Expand All @@ -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();
Expand All @@ -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 /> }],
Expand All @@ -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();
Expand All @@ -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;
}
2 changes: 1 addition & 1 deletion packages/react-router/__tests__/data-memory-router-test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down
24 changes: 11 additions & 13 deletions packages/react-router/lib/components.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import type {
Location,
MemoryHistory,
Router as RemixRouter,
RouterState,
To,
LazyRouteFunction,
RelativeRoutingType,
Expand All @@ -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,
Expand Down Expand Up @@ -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 {
Expand Down
32 changes: 0 additions & 32 deletions packages/react-router/lib/use-sync-external-store-shim/index.ts
View file
Open in desktop

This file was deleted.

152 changes: 0 additions & 152 deletions packages/react-router/lib/use-sync-external-store-shim/useSyncExternalStoreShimClient.ts
View file
Open in desktop

This file was deleted.

20 changes: 0 additions & 20 deletions packages/react-router/lib/use-sync-external-store-shim/useSyncExternalStoreShimServer.ts
View file
Open in desktop

This file was deleted.