From 5d24f4501dd05f88623fc1664b8acebb06a75fd6 Mon Sep 17 00:00:00 2001 From: Leszek Kobus Date: Thu, 12 May 2022 10:00:03 +0200 Subject: [PATCH 1/4] #8312 and also #6226 --- .../__tests__/useNavigate-test.tsx | 57 +++++++++++++++++++ packages/react-router/lib/hooks.tsx | 4 +- 2 files changed, 60 insertions(+), 1 deletion(-) diff --git a/packages/react-router/__tests__/useNavigate-test.tsx b/packages/react-router/__tests__/useNavigate-test.tsx index 8502fea9b8..66bef275c1 100644 --- a/packages/react-router/__tests__/useNavigate-test.tsx +++ b/packages/react-router/__tests__/useNavigate-test.tsx @@ -7,6 +7,7 @@ import { useNavigate, useLocation, } from "react-router"; +import { joinPaths } from "../lib/router"; describe("useNavigate", () => { it("transitions to the new location", () => { @@ -97,4 +98,60 @@ describe("useNavigate", () => { `); }); }); + + describe("when basename is NOT '/' and 'to' is search query only", () => { + /** + * This is simulation of using navigate('?param=ParamValue') when Router basename is NOT '/'. + */ + const routerBasename = "/foo/bar"; + const to = "?param=ParamValue"; + const defaultPathname = '/'; + + function SimulateUseNavigate(props) { + const { + basename, + to, + pathNameCheck = true, // This is swich between using joinPaths or simple basename + } = props; + // joinPaths will always add trailing slash to the basename + // If pathname is the default ('/'), basename value should decide about trailing slash + const pathBasename = pathNameCheck ? basename : joinPaths([basename, defaultPathname]); + + return ( +
+ {pathBasename + to} +
+ ); + } + + it("should not add trailing slash to the basename", () => { + let renderer: TestRenderer.ReactTestRenderer; + TestRenderer.act(() => { + renderer = TestRenderer.create( + + ); + }); + + expect(renderer.toJSON()).toMatchInlineSnapshot(` +
+ ${routerBasename + to} +
+ `); + }); + + it("should add trailing slash to the basename", () => { + let renderer: TestRenderer.ReactTestRenderer; + TestRenderer.act(() => { + renderer = TestRenderer.create( + + ); + }); + + expect(renderer.toJSON()).toMatchInlineSnapshot(` +
+ ${routerBasename + '/' + to} +
+ `); + }); + }); }); diff --git a/packages/react-router/lib/hooks.tsx b/packages/react-router/lib/hooks.tsx index e81c674760..125c245986 100644 --- a/packages/react-router/lib/hooks.tsx +++ b/packages/react-router/lib/hooks.tsx @@ -179,7 +179,9 @@ export function useNavigate(): NavigateFunction { ); if (basename !== "/") { - path.pathname = joinPaths([basename, path.pathname]); + // joinPaths will always add trailing slash to the basename + // If pathname is the default ('/'), basename value should decide about trailing slash + path.pathname = path.pathname === "/" ? basename : joinPaths([basename, path.pathname]); } (!!options.replace ? navigator.replace : navigator.push)( From 95702b5242115718621061579d77dcc1d8d6e4c8 Mon Sep 17 00:00:00 2001 From: Leszek Kobus Date: Thu, 12 May 2022 10:13:09 +0200 Subject: [PATCH 2/4] cla sign --- contributors.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/contributors.yml b/contributors.yml index 7e9d8a5445..9bb444bc66 100644 --- a/contributors.yml +++ b/contributors.yml @@ -53,3 +53,4 @@ - turansky - underager - vijaypushkin +- senseibarni From ef6f6b2c5272276832f8478f3736104415dc4f6f Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Fri, 10 Jun 2022 16:29:17 -0400 Subject: [PATCH 3/4] fix: properly handle trailing slashes in Navigate and Link --- .../__tests__/trailing-slashes-test.tsx | 423 ++++++++++++++++++ .../__tests__/useNavigate-test.tsx | 61 +-- packages/react-router/lib/components.tsx | 4 +- packages/react-router/lib/hooks.tsx | 32 +- packages/router/utils.ts | 9 +- 5 files changed, 459 insertions(+), 70 deletions(-) create mode 100644 packages/react-router-dom/__tests__/trailing-slashes-test.tsx diff --git a/packages/react-router-dom/__tests__/trailing-slashes-test.tsx b/packages/react-router-dom/__tests__/trailing-slashes-test.tsx new file mode 100644 index 0000000000..7fda9c6b1c --- /dev/null +++ b/packages/react-router-dom/__tests__/trailing-slashes-test.tsx @@ -0,0 +1,423 @@ +import { JSDOM } from "jsdom"; +import * as React from "react"; +import * as ReactDOM from "react-dom"; +import { act } from "react-dom/test-utils"; + +import { + BrowserRouter, + Routes, + Route, + Navigate, + Link, + Outlet, +} from "react-router-dom"; + +describe("trailing slashes", () => { + let node: HTMLDivElement; + beforeEach(() => { + node = document.createElement("div"); + document.body.appendChild(node); + }); + + afterEach(() => { + document.body.removeChild(node); + node = null!; + }); + + describe(" element", () => { + describe("with a basename that does not contain a trailing slash", () => { + test("lets root links control trailing slashes (index route)", () => { + let window = getWindowImpl("/app"); + act(() => { + ReactDOM.render( + + + + + + + } + /> + + , + node + ); + }); + + expect(node.querySelectorAll("a")).toMatchInlineSnapshot(` + NodeList [ + , + , + ] + `); + }); + + test('lets root links control trailing slashes (path="")', () => { + let window = getWindowImpl("/app"); + act(() => { + ReactDOM.render( + + + + + + + } + /> + + , + node + ); + }); + + expect(node.querySelectorAll("a")).toMatchInlineSnapshot(` + NodeList [ + , + , + ] + `); + }); + + test("lets nested link control trailing slashes", () => { + let window = getWindowImpl("/app/parent/child"); + act(() => { + ReactDOM.render( + + + + + + + + } + > + + + + + + + + } + > + + + + + + + } + /> + + + + , + node + ); + }); + + expect(node.querySelectorAll("a")).toMatchInlineSnapshot(` + NodeList [ + , + , + , + , + , + , + , + , + , + , + ] + `); + }); + }); + + describe("with a basename that contains a trailing slash", () => { + test("always contains trailing slashes on root links (index route)", () => { + let window = getWindowImpl("/app/"); + act(() => { + ReactDOM.render( + + + + + + + } + /> + + , + node + ); + }); + + expect(node.querySelectorAll("a")).toMatchInlineSnapshot(` + NodeList [ + , + , + ] + `); + }); + + test('always contains trailing slashes on root links (path="" route)', () => { + let window = getWindowImpl("/app/"); + act(() => { + ReactDOM.render( + + + + + + + } + /> + + , + node + ); + }); + + expect(node.querySelectorAll("a")).toMatchInlineSnapshot(` + NodeList [ + , + , + ] + `); + }); + + test("always contains root trailing slashes in nested routes", () => { + let window = getWindowImpl("/app/parent/child"); + act(() => { + ReactDOM.render( + + + + + + + + } + > + + + + + + + + } + > + + + + + + + } + /> + + + + , + node + ); + }); + + expect(node.querySelectorAll("a")).toMatchInlineSnapshot(` + NodeList [ + , + , + , + , + , + , + , + , + , + , + ] + `); + }); + }); + + describe("in a element", () => { + it("should not include trailing slash", () => { + let window = getWindowImpl("/foo/bar"); + spyOn(window.history, "pushState").and.callThrough(); + + expect(window.location.href).toBe("https://remix.run/foo/bar"); + + act(() => { + ReactDOM.render( + + + 👋} /> + } /> + + , + node + ); + }); + + expect(window.location.href).toBe("https://remix.run/foo"); + }); + + it("should include trailing slash from the Navigate", () => { + let window = getWindowImpl("/foo/bar"); + spyOn(window.history, "pushState").and.callThrough(); + + expect(window.location.href).toBe("https://remix.run/foo/bar"); + + act(() => { + ReactDOM.render( + + + 👋} /> + } /> + + , + node + ); + }); + + expect(window.location.href).toBe("https://remix.run/foo/"); + }); + + it("should include trailing slash from the basename", () => { + let window = getWindowImpl("/foo/bar"); + spyOn(window.history, "pushState").and.callThrough(); + + expect(window.location.href).toBe("https://remix.run/foo/bar"); + + act(() => { + ReactDOM.render( + + + 👋} /> + } /> + + , + node + ); + }); + + expect(window.location.href).toBe("https://remix.run/foo/"); + }); + + it("should include trailing slash from both", () => { + let window = getWindowImpl("/foo/bar"); + spyOn(window.history, "pushState").and.callThrough(); + + expect(window.location.href).toBe("https://remix.run/foo/bar"); + + act(() => { + ReactDOM.render( + + + 👋} /> + } /> + + , + node + ); + }); + + expect(window.location.href).toBe("https://remix.run/foo/"); + }); + }); + }); +}); + +function getWindowImpl(initialUrl: string, isHash = false): Window { + // Need to use our own custom DOM in order to get a working history + const dom = new JSDOM(``, { url: "https://remix.run/" }); + dom.window.history.replaceState(null, "", (isHash ? "#" : "") + initialUrl); + return dom.window as unknown as Window; +} diff --git a/packages/react-router/__tests__/useNavigate-test.tsx b/packages/react-router/__tests__/useNavigate-test.tsx index 67532ac770..ac5bd16ccd 100644 --- a/packages/react-router/__tests__/useNavigate-test.tsx +++ b/packages/react-router/__tests__/useNavigate-test.tsx @@ -7,7 +7,6 @@ import { useNavigate, useLocation, } from "react-router"; -import { joinPaths } from "../lib/router"; describe("useNavigate", () => { it("navigates to the new location", () => { @@ -69,7 +68,7 @@ describe("useNavigate", () => { } function ShowLocationState() { - return

location.state: {JSON.stringify(useLocation().state)}

; + return

location.state:{JSON.stringify(useLocation().state)}

; } let renderer: TestRenderer.ReactTestRenderer; @@ -92,66 +91,10 @@ describe("useNavigate", () => { expect(renderer.toJSON()).toMatchInlineSnapshot(`

- location.state: + location.state: {"from":"home"}

`); }); }); - - describe("when basename is NOT '/' and 'to' is search query only", () => { - /** - * This is simulation of using navigate('?param=ParamValue') when Router basename is NOT '/'. - */ - const routerBasename = "/foo/bar"; - const to = "?param=ParamValue"; - const defaultPathname = '/'; - - function SimulateUseNavigate(props) { - const { - basename, - to, - pathNameCheck = true, // This is swich between using joinPaths or simple basename - } = props; - // joinPaths will always add trailing slash to the basename - // If pathname is the default ('/'), basename value should decide about trailing slash - const pathBasename = pathNameCheck ? basename : joinPaths([basename, defaultPathname]); - - return ( -
- {pathBasename + to} -
- ); - } - - it("should not add trailing slash to the basename", () => { - let renderer: TestRenderer.ReactTestRenderer; - TestRenderer.act(() => { - renderer = TestRenderer.create( - - ); - }); - - expect(renderer.toJSON()).toMatchInlineSnapshot(` -
- ${routerBasename + to} -
- `); - }); - - it("should add trailing slash to the basename", () => { - let renderer: TestRenderer.ReactTestRenderer; - TestRenderer.act(() => { - renderer = TestRenderer.create( - - ); - }); - - expect(renderer.toJSON()).toMatchInlineSnapshot(` -
- ${routerBasename + '/' + to} -
- `); - }); - }); }); diff --git a/packages/react-router/lib/components.tsx b/packages/react-router/lib/components.tsx index 02fc92a204..953776481b 100644 --- a/packages/react-router/lib/components.tsx +++ b/packages/react-router/lib/components.tsx @@ -323,7 +323,9 @@ export function Router({ ` You should never have more than one in your app.` ); - let basename = normalizePathname(basenameProp); + // Preserve trailing slashes on basename, so we can let the user control + // the enforcement of trailing slashes throughout the app + let basename = basenameProp.replace(/^\/*/, "/"); let navigationContext = React.useMemo( () => ({ basename, navigator, static: staticProp }), [basename, navigator, staticProp] diff --git a/packages/react-router/lib/hooks.tsx b/packages/react-router/lib/hooks.tsx index 9211fc03fe..3afb43eccb 100644 --- a/packages/react-router/lib/hooks.tsx +++ b/packages/react-router/lib/hooks.tsx @@ -2,6 +2,7 @@ import * as React from "react"; import { isRouteErrorResponse, Location, + normalizePathname, ParamParseKey, Params, Path, @@ -54,11 +55,24 @@ export function useHref(to: To): string { let joinedPathname = pathname; if (basename !== "/") { let toPathname = getToPathname(to); - let endsWithSlash = toPathname != null && toPathname.endsWith("/"); - joinedPathname = - pathname === "/" - ? basename + (endsWithSlash ? "/" : "") - : joinPaths([basename, pathname]); + + // Only append a trailing slash to the raw basename if the basename doesn't + // already have one and this wasn't specifically a route to "". This + // allows folks to control the trailing slash behavior when using a basename + let appendSlash = + !basename.endsWith("/") && + to !== "" && + (to as Path)?.pathname !== "" && + toPathname != null && + toPathname.endsWith("/"); + + if (pathname !== "/") { + joinedPathname = joinPaths([basename, pathname]); + } else if (appendSlash) { + joinedPathname = basename + "/"; + } else { + joinedPathname = basename; + } } return navigator.createHref({ pathname: joinedPathname, search, hash }); @@ -194,9 +208,11 @@ export function useNavigate(): NavigateFunction { ); if (basename !== "/") { - // joinPaths will always add trailing slash to the basename - // If pathname is the default ('/'), basename value should decide about trailing slash - path.pathname = path.pathname === "/" ? basename : joinPaths([basename, path.pathname]); + // If this was a blank path, just use the basename directly + let isBlankPath = to === "" || (to as Path).pathname === ""; + path.pathname = isBlankPath + ? basename + : joinPaths([basename, path.pathname]); } (!!options.replace ? navigator.replace : navigator.push)( diff --git a/packages/router/utils.ts b/packages/router/utils.ts index dec4143134..77ad209f50 100644 --- a/packages/router/utils.ts +++ b/packages/router/utils.ts @@ -578,13 +578,18 @@ export function stripBasename( return null; } - let nextChar = pathname.charAt(basename.length); + // We want to leave trailing slash behavior in the user's control, so if they + // specify a basename with a trailing slash, we should support it + let startIndex = basename.endsWith("/") + ? basename.length - 1 + : basename.length; + let nextChar = pathname.charAt(startIndex); if (nextChar && nextChar !== "/") { // pathname does not start with basename/ return null; } - return pathname.slice(basename.length) || "/"; + return pathname.slice(startIndex) || "/"; } /** From 093228636fcf2eb102b13396998eb0070fcfe593 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Mon, 13 Jun 2022 09:55:36 -0400 Subject: [PATCH 4/4] fix: handle useSearchParams use case with undefined to.pathname --- .../__tests__/trailing-slashes-test.tsx | 379 ++++++++++-------- packages/react-router/lib/hooks.tsx | 6 +- 2 files changed, 209 insertions(+), 176 deletions(-) diff --git a/packages/react-router-dom/__tests__/trailing-slashes-test.tsx b/packages/react-router-dom/__tests__/trailing-slashes-test.tsx index 7fda9c6b1c..1681166145 100644 --- a/packages/react-router-dom/__tests__/trailing-slashes-test.tsx +++ b/packages/react-router-dom/__tests__/trailing-slashes-test.tsx @@ -10,6 +10,8 @@ import { Navigate, Link, Outlet, + useSearchParams, + useLocation, } from "react-router-dom"; describe("trailing slashes", () => { @@ -48,15 +50,15 @@ describe("trailing slashes", () => { }); expect(node.querySelectorAll("a")).toMatchInlineSnapshot(` - NodeList [ - , - , - ] - `); + NodeList [ + , + , + ] + `); }); test('lets root links control trailing slashes (path="")', () => { @@ -81,15 +83,15 @@ describe("trailing slashes", () => { }); expect(node.querySelectorAll("a")).toMatchInlineSnapshot(` - NodeList [ - , - , - ] - `); + NodeList [ + , + , + ] + `); }); test("lets nested link control trailing slashes", () => { @@ -140,39 +142,39 @@ describe("trailing slashes", () => { }); expect(node.querySelectorAll("a")).toMatchInlineSnapshot(` - NodeList [ - , - , - , - , - , - , - , - , - , - , - ] - `); + NodeList [ + , + , + , + , + , + , + , + , + , + , + ] + `); }); }); @@ -199,15 +201,15 @@ describe("trailing slashes", () => { }); expect(node.querySelectorAll("a")).toMatchInlineSnapshot(` - NodeList [ - , - , - ] - `); + NodeList [ + , + , + ] + `); }); test('always contains trailing slashes on root links (path="" route)', () => { @@ -232,15 +234,15 @@ describe("trailing slashes", () => { }); expect(node.querySelectorAll("a")).toMatchInlineSnapshot(` - NodeList [ - , - , - ] - `); + NodeList [ + , + , + ] + `); }); test("always contains root trailing slashes in nested routes", () => { @@ -291,126 +293,155 @@ describe("trailing slashes", () => { }); expect(node.querySelectorAll("a")).toMatchInlineSnapshot(` - NodeList [ - , - , - , - , - , - , - , - , - , - , - ] - `); + NodeList [ + , + , + , + , + , + , + , + , + , + , + ] + `); }); }); + }); - describe("in a element", () => { - it("should not include trailing slash", () => { - let window = getWindowImpl("/foo/bar"); - spyOn(window.history, "pushState").and.callThrough(); - - expect(window.location.href).toBe("https://remix.run/foo/bar"); - - act(() => { - ReactDOM.render( - - - 👋} /> - } /> - - , - node - ); - }); - - expect(window.location.href).toBe("https://remix.run/foo"); + describe("in a element", () => { + it("should not include trailing slash on empty string path", () => { + let window = getWindowImpl("/foo/bar"); + spyOn(window.history, "pushState").and.callThrough(); + + expect(window.location.href).toBe("https://remix.run/foo/bar"); + + act(() => { + ReactDOM.render( + + + 👋} /> + } /> + + , + node + ); }); - it("should include trailing slash from the Navigate", () => { - let window = getWindowImpl("/foo/bar"); - spyOn(window.history, "pushState").and.callThrough(); - - expect(window.location.href).toBe("https://remix.run/foo/bar"); - - act(() => { - ReactDOM.render( - - - 👋} /> - } /> - - , - node - ); - }); + expect(window.location.href).toBe("https://remix.run/foo"); + }); - expect(window.location.href).toBe("https://remix.run/foo/"); + it("should not include trailing slash via useSearchParams", () => { + let window = getWindowImpl("/foo"); + spyOn(window.history, "pushState").and.callThrough(); + + expect(window.location.href).toBe("https://remix.run/foo"); + + function SetSearchParams() { + let [, setSearchParams] = useSearchParams(); + React.useEffect( + () => setSearchParams({ key: "value" }), + [setSearchParams] + ); + return

👋

; + } + + act(() => { + ReactDOM.render( + + + } /> + + , + node + ); }); - it("should include trailing slash from the basename", () => { - let window = getWindowImpl("/foo/bar"); - spyOn(window.history, "pushState").and.callThrough(); - - expect(window.location.href).toBe("https://remix.run/foo/bar"); - - act(() => { - ReactDOM.render( - - - 👋} /> - } /> - - , - node - ); - }); + expect(window.location.href).toBe("https://remix.run/foo?key=value"); + }); - expect(window.location.href).toBe("https://remix.run/foo/"); + it("should include trailing slash from the Navigate", () => { + let window = getWindowImpl("/foo/bar"); + spyOn(window.history, "pushState").and.callThrough(); + + expect(window.location.href).toBe("https://remix.run/foo/bar"); + + act(() => { + ReactDOM.render( + + + 👋} /> + } /> + + , + node + ); }); - it("should include trailing slash from both", () => { - let window = getWindowImpl("/foo/bar"); - spyOn(window.history, "pushState").and.callThrough(); + expect(window.location.href).toBe("https://remix.run/foo/"); + }); - expect(window.location.href).toBe("https://remix.run/foo/bar"); + it("should include trailing slash from the basename", () => { + let window = getWindowImpl("/foo/bar"); + spyOn(window.history, "pushState").and.callThrough(); + + expect(window.location.href).toBe("https://remix.run/foo/bar"); + + act(() => { + ReactDOM.render( + + + 👋} /> + } /> + + , + node + ); + }); - act(() => { - ReactDOM.render( - - - 👋} /> - } /> - - , - node - ); - }); + expect(window.location.href).toBe("https://remix.run/foo/"); + }); - expect(window.location.href).toBe("https://remix.run/foo/"); + it("should include trailing slash from both", () => { + let window = getWindowImpl("/foo/bar"); + spyOn(window.history, "pushState").and.callThrough(); + + expect(window.location.href).toBe("https://remix.run/foo/bar"); + + act(() => { + ReactDOM.render( + + + 👋} /> + } /> + + , + node + ); }); + + expect(window.location.href).toBe("https://remix.run/foo/"); }); }); }); diff --git a/packages/react-router/lib/hooks.tsx b/packages/react-router/lib/hooks.tsx index 3afb43eccb..5e4cb7fc26 100644 --- a/packages/react-router/lib/hooks.tsx +++ b/packages/react-router/lib/hooks.tsx @@ -208,8 +208,10 @@ export function useNavigate(): NavigateFunction { ); if (basename !== "/") { - // If this was a blank path, just use the basename directly - let isBlankPath = to === "" || (to as Path).pathname === ""; + // If this was a blank path, just use the basename directly, this gives + // the user control over trailing slash behavior + let toPath = typeof to === "string" ? parsePath(to) : to; + let isBlankPath = toPath.pathname == null || toPath.pathname === ""; path.pathname = isBlankPath ? basename : joinPaths([basename, path.pathname]);