From fd0e45b82fccb5086f33e8beba7af1f877e193b9 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Thu, 1 Jun 2023 11:47:43 -0400 Subject: [PATCH 1/7] Strip basename from location provided to getKey --- .changeset/strip-basename-getkey.md | 5 ++++ packages/router/__tests__/router-test.ts | 31 ++++++++++++++++++++++++ packages/router/router.ts | 31 +++++++++++++++--------- 3 files changed, 56 insertions(+), 11 deletions(-) create mode 100644 .changeset/strip-basename-getkey.md diff --git a/.changeset/strip-basename-getkey.md b/.changeset/strip-basename-getkey.md new file mode 100644 index 0000000000..f9c4be368d --- /dev/null +++ b/.changeset/strip-basename-getkey.md @@ -0,0 +1,5 @@ +--- +"@remix-run/router": patch +--- + +Fix `basename` not being stripped from the location provided to `` diff --git a/packages/router/__tests__/router-test.ts b/packages/router/__tests__/router-test.ts index 536e089367..8eebd33551 100644 --- 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("strips 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 = { "/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("/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 index 23a57f5a9c..cf8a1f52a3 100644 --- a/packages/router/router.ts +++ b/packages/router/router.ts @@ -2430,7 +2430,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 @@ -2450,15 +2450,27 @@ export function createRouter(init: RouterInit): Router { }; } + function getScrollKey(location: Location, matches: AgnosticDataRouteMatch[]) { + if (getScrollRestorationKey) { + let loc = { + ...location, + pathname: + stripBasename(location.pathname, basename) || location.pathname, + }; + let userMatches = matches.map((m) => + createUseMatchesMatch(m, state.loaderData) + ); + return getScrollRestorationKey(loc, userMatches) || 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(); } } @@ -2467,11 +2479,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; From 88130775abd0b0e468fcaa21e46f35de43984489 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Fri, 2 Jun 2023 12:14:44 -0400 Subject: [PATCH 2/7] Remove basename stripping to RR layer --- .../__tests__/scroll-restoration-test.tsx | 85 +++++++++++++++++++ packages/react-router-dom/index.tsx | 14 ++- packages/router/__tests__/router-test.ts | 6 +- packages/router/router.ts | 12 +-- 4 files changed, 105 insertions(+), 12 deletions(-) create mode 100644 packages/react-router-dom/__tests__/scroll-restoration-test.tsx diff --git a/packages/react-router-dom/__tests__/scroll-restoration-test.tsx b/packages/react-router-dom/__tests__/scroll-restoration-test.tsx new file mode 100644 index 0000000000..af0e196033 --- /dev/null +++ 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("renders the first route that matches the URL", () => { + let getKey = jest.fn(() => "mykey"); + let testWindow = getWindowImpl("/base"); + window.scrollTo = () => {}; + + let router = createBrowserRouter( + [ + { + path: "/", + Component() { + return ( + <> + + + + ); + }, + children: [ + { + index: true, + Component() { + return /page; + }, + }, + { + path: "page", + Component() { + return

Page

; + }, + }, + ], + }, + ], + { basename: "/base", window: testWindow } + ); + let { container } = render(); + + 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(``, { 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 index 7495692dab..a68d44b9ef 100644 --- 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(); @@ -1258,9 +1259,20 @@ function useScrollRestoration({ savedScrollPositions, () => window.scrollY, getKey + ? (location, matches) => + getKey( + { + ...location, + pathname: + stripBasename(location.pathname, basename) || + location.pathname, + }, + matches + ) + : undefined ); 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 index 8eebd33551..0aec9256b1 100644 --- a/packages/router/__tests__/router-test.ts +++ b/packages/router/__tests__/router-test.ts @@ -6959,7 +6959,7 @@ describe("a router", () => { expect(t.router.state.preventScrollReset).toBe(false); }); - it("strips the basename from the location provided to getKey", async () => { + it("does not strip the basename from the location provided to getKey", async () => { let t = setup({ routes: SCROLL_ROUTES, basename: "/base", @@ -6971,7 +6971,7 @@ describe("a router", () => { }, }); - let positions = { "/tasks": 100 }; + let positions = { "/base/tasks": 100 }; let activeScrollPosition = 0; let pathname; t.router.enableScrollRestoration( @@ -6985,7 +6985,7 @@ describe("a router", () => { let nav1 = await t.navigate("/base/tasks"); await nav1.loaders.tasks.resolve("TASKS"); - expect(pathname).toBe("/tasks"); + expect(pathname).toBe("/base/tasks"); expect(t.router.state.restoreScrollPosition).toBe(100); expect(t.router.state.preventScrollReset).toBe(false); }); diff --git a/packages/router/router.ts b/packages/router/router.ts index cf8a1f52a3..94b175c238 100644 --- a/packages/router/router.ts +++ b/packages/router/router.ts @@ -2452,15 +2452,11 @@ export function createRouter(init: RouterInit): Router { function getScrollKey(location: Location, matches: AgnosticDataRouteMatch[]) { if (getScrollRestorationKey) { - let loc = { - ...location, - pathname: - stripBasename(location.pathname, basename) || location.pathname, - }; - let userMatches = matches.map((m) => - createUseMatchesMatch(m, state.loaderData) + let key = getScrollRestorationKey( + location, + matches.map((m) => createUseMatchesMatch(m, state.loaderData)) ); - return getScrollRestorationKey(loc, userMatches) || location.key; + return key || location.key; } return location.key; } From bcf7e90687a17248eee06400a66100e8bf8e2ebd Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Fri, 2 Jun 2023 13:53:51 -0400 Subject: [PATCH 3/7] Bump bundle --- package.json | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/package.json b/package.json index 933e5eb919..6fee45572c 100644 --- 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": "17.9 kB" + "none": "18.1 kB" } } } From de77321d82cc36bc563360c910160c3a94c6ec83 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Fri, 2 Jun 2023 14:40:37 -0400 Subject: [PATCH 4/7] Add comment --- packages/react-router-dom/index.tsx | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/react-router-dom/index.tsx b/packages/react-router-dom/index.tsx index a68d44b9ef..2b273a1bff 100644 --- a/packages/react-router-dom/index.tsx +++ b/packages/react-router-dom/index.tsx @@ -1261,6 +1261,7 @@ function useScrollRestoration({ getKey ? (location, matches) => getKey( + // Strip the basename to match useLocation { ...location, pathname: From 330c65fa8b277dde694da5d5424e996ce88bf723 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Tue, 6 Jun 2023 16:15:13 -0400 Subject: [PATCH 5/7] Fix test description --- packages/react-router-dom/__tests__/scroll-restoration-test.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/react-router-dom/__tests__/scroll-restoration-test.tsx b/packages/react-router-dom/__tests__/scroll-restoration-test.tsx index af0e196033..f30ae3d213 100644 --- a/packages/react-router-dom/__tests__/scroll-restoration-test.tsx +++ b/packages/react-router-dom/__tests__/scroll-restoration-test.tsx @@ -11,7 +11,7 @@ import { } from "react-router-dom"; describe(`ScrollRestoration`, () => { - it("renders the first route that matches the URL", () => { + it("removes the basename from the location provided to getKey", () => { let getKey = jest.fn(() => "mykey"); let testWindow = getWindowImpl("/base"); window.scrollTo = () => {}; From 485e475b70dace103779ad55ab678c2de24fe1da Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Tue, 6 Jun 2023 16:16:08 -0400 Subject: [PATCH 6/7] update changeset --- .changeset/strip-basename-getkey.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/strip-basename-getkey.md b/.changeset/strip-basename-getkey.md index f9c4be368d..7b3b292191 100644 --- a/.changeset/strip-basename-getkey.md +++ b/.changeset/strip-basename-getkey.md @@ -2,4 +2,4 @@ "@remix-run/router": patch --- -Fix `basename` not being stripped from the location provided to `` +Strip `basename` from the `location` provided to `` to match the `useLocation` behavior From 797fcb166e263ffa33c03ca2f8b1142eb8be41e5 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Tue, 6 Jun 2023 16:19:19 -0400 Subject: [PATCH 7/7] Code cleanup --- packages/react-router-dom/index.tsx | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/packages/react-router-dom/index.tsx b/packages/react-router-dom/index.tsx index 2b273a1bff..665355056b 100644 --- a/packages/react-router-dom/index.tsx +++ b/packages/react-router-dom/index.tsx @@ -1255,13 +1255,11 @@ function useScrollRestoration({ // Enable scroll restoration in the router // eslint-disable-next-line react-hooks/rules-of-hooks React.useLayoutEffect(() => { - let disableScrollRestoration = router?.enableScrollRestoration( - savedScrollPositions, - () => window.scrollY, - getKey + let getKeyWithoutBasename: GetScrollRestorationKeyFunction | undefined = + getKey && basename !== "/" ? (location, matches) => getKey( - // Strip the basename to match useLocation + // Strip the basename to match useLocation() { ...location, pathname: @@ -1270,7 +1268,11 @@ function useScrollRestoration({ }, matches ) - : undefined + : getKey; + let disableScrollRestoration = router?.enableScrollRestoration( + savedScrollPositions, + () => window.scrollY, + getKeyWithoutBasename ); return () => disableScrollRestoration && disableScrollRestoration(); }, [router, basename, getKey]);