Strip basename from location provided to getKey (#10550)

brophdawg11 · web-flow · commit 08e2f76fb3b5 · 2023-06-06T16:23:22.000-04:00
diff --git a/.changeset/strip-basename-getkey.md b/.changeset/strip-basename-getkey.md
@@ -0,0 +1,5 @@
+---
+"@remix-run/router": patch
+---
+
+Strip `basename` from the `location` provided to `<ScrollRestoration getKey>` to match the `useLocation` behavior
diff --git a/package.json b/package.json
@@ -118,10 +118,10 @@
       "none": "15.8 kB"
     },
     "packages/react-router-dom/dist/react-router-dom.production.min.js": {
-      "none": "12.0 kB"
+      "none": "12.1 kB"
     },
     "packages/react-router-dom/dist/umd/react-router-dom.production.min.js": {
-      "none": "18.0 kB"
+      "none": "18.1 kB"
     }
   }
 }
diff --git a/packages/react-router-dom/__tests__/scroll-restoration-test.tsx b/packages/react-router-dom/__tests__/scroll-restoration-test.tsx
@@ -0,0 +1,85 @@
+import { JSDOM } from "jsdom";
+import * as React from "react";
+import { render, prettyDOM, fireEvent, screen } from "@testing-library/react";
+import "@testing-library/jest-dom";
+import {
+  Link,
+  Outlet,
+  RouterProvider,
+  ScrollRestoration,
+  createBrowserRouter,
+} from "react-router-dom";
+
+describe(`ScrollRestoration`, () => {
+  it("removes the basename from the location provided to getKey", () => {
+    let getKey = jest.fn(() => "mykey");
+    let testWindow = getWindowImpl("/base");
+    window.scrollTo = () => {};
+
+    let router = createBrowserRouter(
+      [
+        {
+          path: "/",
+          Component() {
+            return (
+              <>
+                <Outlet />
+                <ScrollRestoration getKey={getKey} />
+              </>
+            );
+          },
+          children: [
+            {
+              index: true,
+              Component() {
+                return <Link to="/page">/page</Link>;
+              },
+            },
+            {
+              path: "page",
+              Component() {
+                return <h1>Page</h1>;
+              },
+            },
+          ],
+        },
+      ],
+      { basename: "/base", window: testWindow }
+    );
+    let { container } = render(<RouterProvider router={router} />);
+
+    expect(getKey.mock.calls.length).toBe(1);
+    // @ts-expect-error
+    expect(getKey.mock.calls[0][0].pathname).toBe("/"); // restore
+
+    expect(getHtml(container)).toMatch("/page");
+    fireEvent.click(screen.getByText("/page"));
+    expect(getHtml(container)).toMatch("Page");
+
+    expect(getKey.mock.calls.length).toBe(3);
+    // @ts-expect-error
+    expect(getKey.mock.calls[1][0].pathname).toBe("/"); // save
+    // @ts-expect-error
+    expect(getKey.mock.calls[2][0].pathname).toBe("/page"); // restore
+  });
+});
+
+function getWindowImpl(initialUrl: string): Window {
+  // Need to use our own custom DOM in order to get a working history
+  const dom = new JSDOM(`<!DOCTYPE html>`, { url: "http://localhost/" });
+  dom.window.history.replaceState(null, "", initialUrl);
+  return dom.window as unknown as Window;
+}
+
+function getHtml(container: HTMLElement) {
+  return prettyDOM(container, undefined, {
+    highlight: false,
+    theme: {
+      comment: null,
+      content: null,
+      prop: null,
+      tag: null,
+      value: null,
+    },
+  });
+}
diff --git a/packages/react-router-dom/index.tsx b/packages/react-router-dom/index.tsx
@@ -1208,6 +1208,7 @@ function useScrollRestoration({
   let { restoreScrollPosition, preventScrollReset } = useDataRouterState(
     DataRouterStateHook.UseScrollRestoration
   );
+  let { basename } = React.useContext(NavigationContext);
   let location = useLocation();
   let matches = useMatches();
   let navigation = useNavigation();
@@ -1254,13 +1255,27 @@ function useScrollRestoration({
     // Enable scroll restoration in the router
     // eslint-disable-next-line react-hooks/rules-of-hooks
     React.useLayoutEffect(() => {
+      let getKeyWithoutBasename: GetScrollRestorationKeyFunction | undefined =
+        getKey && basename !== "/"
+          ? (location, matches) =>
+              getKey(
+                // Strip the basename to match useLocation()
+                {
+                  ...location,
+                  pathname:
+                    stripBasename(location.pathname, basename) ||
+                    location.pathname,
+                },
+                matches
+              )
+          : getKey;
       let disableScrollRestoration = router?.enableScrollRestoration(
         savedScrollPositions,
         () => window.scrollY,
-        getKey
+        getKeyWithoutBasename
       );
       return () => disableScrollRestoration && disableScrollRestoration();
-    }, [router, getKey]);
+    }, [router, basename, getKey]);
 
     // Restore scrolling when state.restoreScrollPosition changes
     // eslint-disable-next-line react-hooks/rules-of-hooks
diff --git a/packages/router/__tests__/router-test.ts b/packages/router/__tests__/router-test.ts
@@ -6959,6 +6959,37 @@ describe("a router", () => {
         expect(t.router.state.preventScrollReset).toBe(false);
       });
 
+      it("does not strip the basename from the location provided to getKey", async () => {
+        let t = setup({
+          routes: SCROLL_ROUTES,
+          basename: "/base",
+          initialEntries: ["/base"],
+          hydrationData: {
+            loaderData: {
+              index: "INDEX_DATA",
+            },
+          },
+        });
+
+        let positions = { "/base/tasks": 100 };
+        let activeScrollPosition = 0;
+        let pathname;
+        t.router.enableScrollRestoration(
+          positions,
+          () => activeScrollPosition,
+          (l) => {
+            pathname = l.pathname;
+            return l.pathname;
+          }
+        );
+
+        let nav1 = await t.navigate("/base/tasks");
+        await nav1.loaders.tasks.resolve("TASKS");
+        expect(pathname).toBe("/base/tasks");
+        expect(t.router.state.restoreScrollPosition).toBe(100);
+        expect(t.router.state.preventScrollReset).toBe(false);
+      });
+
       it("restores scroll on GET submissions", async () => {
         let t = setup({
           routes: SCROLL_ROUTES,
diff --git a/packages/router/router.ts b/packages/router/router.ts
@@ -2444,7 +2444,7 @@ export function createRouter(init: RouterInit): Router {
   ) {
     savedScrollPositions = positions;
     getScrollPosition = getPosition;
-    getScrollRestorationKey = getKey || ((location) => location.key);
+    getScrollRestorationKey = getKey || null;
 
     // Perform initial hydration scroll restoration, since we miss the boat on
     // the initial updateState() because we've not yet rendered <ScrollRestoration/>
@@ -2464,15 +2464,23 @@ export function createRouter(init: RouterInit): Router {
     };
   }
 
+  function getScrollKey(location: Location, matches: AgnosticDataRouteMatch[]) {
+    if (getScrollRestorationKey) {
+      let key = getScrollRestorationKey(
+        location,
+        matches.map((m) => createUseMatchesMatch(m, state.loaderData))
+      );
+      return key || location.key;
+    }
+    return location.key;
+  }
+
   function saveScrollPosition(
     location: Location,
     matches: AgnosticDataRouteMatch[]
   ): void {
-    if (savedScrollPositions && getScrollRestorationKey && getScrollPosition) {
-      let userMatches = matches.map((m) =>
-        createUseMatchesMatch(m, state.loaderData)
-      );
-      let key = getScrollRestorationKey(location, userMatches) || location.key;
+    if (savedScrollPositions && getScrollPosition) {
+      let key = getScrollKey(location, matches);
       savedScrollPositions[key] = getScrollPosition();
     }
   }
@@ -2481,11 +2489,8 @@ export function createRouter(init: RouterInit): Router {
     location: Location,
     matches: AgnosticDataRouteMatch[]
   ): number | null {
-    if (savedScrollPositions && getScrollRestorationKey && getScrollPosition) {
-      let userMatches = matches.map((m) =>
-        createUseMatchesMatch(m, state.loaderData)
-      );
-      let key = getScrollRestorationKey(location, userMatches) || location.key;
+    if (savedScrollPositions) {
+      let key = getScrollKey(location, matches);
       let y = savedScrollPositions[key];
       if (typeof y === "number") {
         return y;

-Original file line number
+Diff line change
@@ @@ -0,0 +1,5 @@ @@
 +---
 +"@remix-run/router": patch
 +---
++
 +Strip `basename` from the `location` provided to `<ScrollRestoration getKey>` to match the `useLocation` behavior
Original file line number	Diff line number	Diff line change
`@@ -118,10 +118,10 @@`
`118`	`118`	`"none": "15.8 kB"`
`119`	`119`	`},`
`120`	`120`	`"packages/react-router-dom/dist/react-router-dom.production.min.js": {`
`121`		`- "none": "12.0 kB"`
	`121`	`+ "none": "12.1 kB"`
`122`	`122`	`},`
`123`	`123`	`"packages/react-router-dom/dist/umd/react-router-dom.production.min.js": {`
`124`		`- "none": "18.0 kB"`
	`124`	`+ "none": "18.1 kB"`
`125`	`125`	`}`
`126`	`126`	`}`
`127`	`127`	`}`