From d4d5b29d309ead5eb5ec5857c9822623188497ce Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Mon, 14 Nov 2022 13:34:04 -0500 Subject: [PATCH 1/6] fix: handle encoding of dynamic params in descendant routes --- .../lib/components.tsx | 8 ++++ .../__tests__/special-characters-test.tsx | 39 +++++++++++++++++++ packages/react-router-dom/server.tsx | 25 +++++++++--- packages/react-router/lib/components.tsx | 1 + packages/react-router/lib/context.ts | 1 + packages/react-router/lib/hooks.tsx | 13 ++++++- packages/router/history.ts | 20 ++++++---- packages/router/router.ts | 18 ++++++++- 8 files changed, 107 insertions(+), 18 deletions(-) diff --git a/packages/react-router-dom-v5-compat/lib/components.tsx b/packages/react-router-dom-v5-compat/lib/components.tsx index d16824bd14..e629beb89b 100644 --- a/packages/react-router-dom-v5-compat/lib/components.tsx +++ b/packages/react-router-dom-v5-compat/lib/components.tsx @@ -81,6 +81,14 @@ export function StaticRouter({ createHref(to: To) { return typeof to === "string" ? to : createPath(to); }, + encodeLocation(to: To) { + let path = typeof to === "string" ? parsePath(to) : to; + return { + pathname: path.pathname || "", + search: path.search || "", + hash: path.hash || "", + }; + }, push(to: To) { throw new Error( `You cannot use navigator.push() on the server because it is a stateless ` + diff --git a/packages/react-router-dom/__tests__/special-characters-test.tsx b/packages/react-router-dom/__tests__/special-characters-test.tsx index ff66a95585..1208d3c661 100644 --- a/packages/react-router-dom/__tests__/special-characters-test.tsx +++ b/packages/react-router-dom/__tests__/special-characters-test.tsx @@ -221,6 +221,17 @@ describe("special character tests", () => { path="/reset" element={Link to path} /> + + } + /> + + } + /> } /> ); @@ -487,6 +498,34 @@ describe("special character tests", () => { } }); + it("handles special chars in descendant routes paths", async () => { + for (let charDef of specialChars) { + let { char, pathChar } = charDef; + + await testParamValues( + `/descendant/${char}/match`, + "Descendant Route", + { + pathname: `/descendant/${pathChar}/match`, + search: "", + hash: "", + }, + { param: char, "*": "match" } + ); + + await testParamValues( + `/descendant/foo${char}bar/match`, + "Descendant Route", + { + pathname: `/descendant/foo${pathChar}bar/match`, + search: "", + hash: "", + }, + { param: `foo${char}bar`, "*": "match" } + ); + } + }); + it("handles special chars in search params", async () => { for (let charDef of specialChars) { let { char, searchChar } = charDef; diff --git a/packages/react-router-dom/server.tsx b/packages/react-router-dom/server.tsx index e668894779..67e7b7beb5 100644 --- a/packages/react-router-dom/server.tsx +++ b/packages/react-router-dom/server.tsx @@ -1,5 +1,6 @@ import * as React from "react"; import type { + Path, RevalidationState, Router as RemixRouter, StaticHandlerContext, @@ -141,9 +142,8 @@ export function unstable_StaticRouterProvider({ function getStatelessNavigator() { return { - createHref(to: To) { - return typeof to === "string" ? to : createPath(to); - }, + createHref, + encodeLocation, push(to: To) { throw new Error( `You cannot use navigator.push() on the server because it is a stateless ` + @@ -230,9 +230,8 @@ export function unstable_createStaticRouter( revalidate() { throw msg("revalidate"); }, - createHref() { - throw msg("createHref"); - }, + createHref, + encodeLocation, getFetcher() { return IDLE_FETCHER; }, @@ -246,3 +245,17 @@ export function unstable_createStaticRouter( _internalActiveDeferreds: new Map(), }; } + +function createHref(to: To) { + return typeof to === "string" ? to : createPath(to); +} + +function encodeLocation(to: To): Path { + // Locations should already be encoded on the server, so just return as-is + let path = typeof to === "string" ? parsePath(to) : to; + return { + pathname: path.pathname || "", + search: path.search || "", + hash: path.hash || "", + }; +} diff --git a/packages/react-router/lib/components.tsx b/packages/react-router/lib/components.tsx index 94ad090092..bf890e92f4 100644 --- a/packages/react-router/lib/components.tsx +++ b/packages/react-router/lib/components.tsx @@ -69,6 +69,7 @@ export function RouterProvider({ let navigator = React.useMemo((): Navigator => { return { createHref: router.createHref, + encodeLocation: router.encodeLocation, go: (n) => router.navigate(n), push: (to, state, opts) => router.navigate(to, { diff --git a/packages/react-router/lib/context.ts b/packages/react-router/lib/context.ts index af7182557d..f64cd1ae09 100644 --- a/packages/react-router/lib/context.ts +++ b/packages/react-router/lib/context.ts @@ -107,6 +107,7 @@ export interface NavigateOptions { */ export interface Navigator { createHref: History["createHref"]; + encodeLocation: History["encodeLocation"]; go: History["go"]; push(to: To, state?: any, opts?: NavigateOptions): void; replace(to: To, state?: any, opts?: NavigateOptions): void; diff --git a/packages/react-router/lib/hooks.tsx b/packages/react-router/lib/hooks.tsx index d0a06f82e4..0768a4109c 100644 --- a/packages/react-router/lib/hooks.tsx +++ b/packages/react-router/lib/hooks.tsx @@ -310,6 +310,7 @@ export function useRoutes( `useRoutes() may be used only in the context of a component.` ); + let { navigator } = React.useContext(NavigationContext); let dataRouterStateContext = React.useContext(DataRouterStateContext); let { matches: parentMatches } = React.useContext(RouteContext); let routeMatch = parentMatches[parentMatches.length - 1]; @@ -401,11 +402,19 @@ export function useRoutes( matches.map((match) => Object.assign({}, match, { params: Object.assign({}, parentParams, match.params), - pathname: joinPaths([parentPathnameBase, match.pathname]), + pathname: joinPaths([ + parentPathnameBase, + // Re-encode pathnames that were decoded inside matchRoutes + navigator.encodeLocation(match.pathname).pathname, + ]), pathnameBase: match.pathnameBase === "/" ? parentPathnameBase - : joinPaths([parentPathnameBase, match.pathnameBase]), + : joinPaths([ + parentPathnameBase, + // Re-encode pathnames that were decoded inside matchRoutes + navigator.encodeLocation(match.pathnameBase).pathname, + ]), }) ), parentMatches, diff --git a/packages/router/history.ts b/packages/router/history.ts index 9af5d65ddd..56779c8f9d 100644 --- a/packages/router/history.ts +++ b/packages/router/history.ts @@ -127,12 +127,12 @@ export interface History { /** * Encode a location the same way window.history would do (no-op for memory - * history) so we ensure our PUSH/REPLAC e navigations for data routers + * history) so we ensure our PUSH/REPLACE navigations for data routers * behave the same as POP * - * @param location The incoming location from router.navigate() + * @param to Unencoded path */ - encodeLocation(location: Location): Location; + encodeLocation(to: To): Path; /** * Pushes a new location onto the history stack, increasing its length by one. @@ -268,8 +268,13 @@ export function createMemoryHistory( createHref(to) { return typeof to === "string" ? to : createPath(to); }, - encodeLocation(location) { - return location; + encodeLocation(to: To) { + let path = typeof to === "string" ? parsePath(to) : to; + return { + pathname: path.pathname || "", + search: path.search || "", + hash: path.hash || "", + }; }, push(to, state) { action = Action.Push; @@ -636,11 +641,10 @@ function getUrlBasedHistory( createHref(to) { return createHref(window, to); }, - encodeLocation(location) { + encodeLocation(to) { // Encode a Location the same way window.location would - let url = createURL(createPath(location)); + let url = createURL(typeof to === "string" ? to : createPath(to)); return { - ...location, pathname: url.pathname, search: url.search, hash: url.hash, diff --git a/packages/router/router.ts b/packages/router/router.ts index 51e02187de..8b2aecfa76 100644 --- a/packages/router/router.ts +++ b/packages/router/router.ts @@ -1,4 +1,4 @@ -import type { History, Location, To } from "./history"; +import type { History, Location, Path, To } from "./history"; import { Action as HistoryAction, createLocation, @@ -154,6 +154,16 @@ export interface Router { */ createHref(location: Location | URL): string; + /** + * @internal + * PRIVATE - DO NOT USE + * + * Utility function to URL encode a location according to the internal + * history implementation + * @param location + */ + encodeLocation(to: To): Path; + /** * @internal * PRIVATE - DO NOT USE @@ -773,7 +783,10 @@ export function createRouter(init: RouterInit): Router { // remains the same as POP and non-data-router usages. new URL() does all // the same encoding we'd get from a history.pushState/window.location read // without having to touch history - location = init.history.encodeLocation(location); + location = { + ...location, + ...init.history.encodeLocation(location), + }; let historyAction = (opts && opts.replace) === true || submission != null @@ -1825,6 +1838,7 @@ export function createRouter(init: RouterInit): Router { // Passthrough to history-aware createHref used by useHref so we get proper // hash-aware URLs in DOM paths createHref: (to: To) => init.history.createHref(to), + encodeLocation: (location: To) => init.history.encodeLocation(location), getFetcher, deleteFetcher, dispose, From b7812b379de06529d2bf70916e8ecf9a47310649 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Mon, 14 Nov 2022 15:06:44 -0500 Subject: [PATCH 2/6] add changeset --- .changeset/pretty-dolls-bathe.md | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 .changeset/pretty-dolls-bathe.md diff --git a/.changeset/pretty-dolls-bathe.md b/.changeset/pretty-dolls-bathe.md new file mode 100644 index 0000000000..7fe4bfd187 --- /dev/null +++ b/.changeset/pretty-dolls-bathe.md @@ -0,0 +1,6 @@ +--- +"react-router": patch +"react-router-dom": patch +--- + +Fix issues with encoded characters in descendant routes From e64b997d15cf058a444513d1c48101026d076896 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Tue, 15 Nov 2022 09:51:48 -0500 Subject: [PATCH 3/6] fix var name --- packages/router/router.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/router/router.ts b/packages/router/router.ts index 8b2aecfa76..06b7ba8f56 100644 --- a/packages/router/router.ts +++ b/packages/router/router.ts @@ -1838,7 +1838,7 @@ export function createRouter(init: RouterInit): Router { // Passthrough to history-aware createHref used by useHref so we get proper // hash-aware URLs in DOM paths createHref: (to: To) => init.history.createHref(to), - encodeLocation: (location: To) => init.history.encodeLocation(location), + encodeLocation: (to: To) => init.history.encodeLocation(to), getFetcher, deleteFetcher, dispose, From e29a004bfa3f2fbe23a8e83f8fc1ca2c985b0d7f Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Tue, 15 Nov 2022 09:53:13 -0500 Subject: [PATCH 4/6] fix jsdoc --- packages/router/router.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/router/router.ts b/packages/router/router.ts index 06b7ba8f56..3f11274690 100644 --- a/packages/router/router.ts +++ b/packages/router/router.ts @@ -158,9 +158,9 @@ export interface Router { * @internal * PRIVATE - DO NOT USE * - * Utility function to URL encode a location according to the internal + * Utility function to URL encode a destination path according to the internal * history implementation - * @param location + * @param to */ encodeLocation(to: To): Path; From 43de13c0921e19d922c6f6878db6bccd9e52dec4 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Thu, 17 Nov 2022 12:52:56 -0500 Subject: [PATCH 5/6] encode pathnames in NavLink check --- .../__tests__/nav-link-active-test.tsx | 90 +++++++++++++++++++ packages/react-router-dom/index.tsx | 3 +- 2 files changed, 92 insertions(+), 1 deletion(-) diff --git a/packages/react-router-dom/__tests__/nav-link-active-test.tsx b/packages/react-router-dom/__tests__/nav-link-active-test.tsx index f44dce59a9..ae20091fb8 100644 --- a/packages/react-router-dom/__tests__/nav-link-active-test.tsx +++ b/packages/react-router-dom/__tests__/nav-link-active-test.tsx @@ -8,6 +8,7 @@ import { JSDOM } from "jsdom"; import * as React from "react"; import * as TestRenderer from "react-test-renderer"; import { + BrowserRouter, MemoryRouter, Routes, Route, @@ -189,6 +190,37 @@ describe("NavLink", () => { expect(anchor.children[0]).toMatch("Home (current)"); }); + + it("matches when portions of the url are encoded", () => { + let renderer: TestRenderer.ReactTestRenderer; + + TestRenderer.act(() => { + renderer = TestRenderer.create( + + + + Matt + Matt + Michael + + } + /> + + + ); + }); + + let anchors = renderer.root.findAllByType("a"); + + expect(anchors.map((a) => a.props.className)).toEqual([ + "active", + "active", + "", + ]); + }); }); describe("when it matches a partial URL segment", () => { @@ -712,6 +744,64 @@ describe("NavLink using a data router", () => { await waitFor(() => screen.getByText("Baz page")); expect(screen.getByText("Link to Bar").className).toBe(""); }); + + it("applies the default 'active'/'pending' classNames when the url has encoded characters", async () => { + let barDfd = createDeferred(); + let bazDfd = createDeferred(); + let router = createBrowserRouter( + createRoutesFromElements( + }> + Foo page

} /> + barDfd.promise} + element={

Bar page

} + /> + bazDfd.promise} + element={

Baz page

} + /> + + ), + { + window: getWindow("/foo"), + } + ); + render(); + + function Layout() { + return ( + <> + Link to Foo + Link to Bar + Link to Baz + + + ); + } + + expect(screen.getByText("Link to Bar").className).toBe(""); + expect(screen.getByText("Link to Baz").className).toBe(""); + + fireEvent.click(screen.getByText("Link to Bar")); + expect(screen.getByText("Link to Bar").className).toBe("pending"); + expect(screen.getByText("Link to Baz").className).toBe(""); + + barDfd.resolve(null); + await waitFor(() => screen.getByText("Bar page")); + expect(screen.getByText("Link to Bar").className).toBe("active"); + expect(screen.getByText("Link to Baz").className).toBe(""); + + fireEvent.click(screen.getByText("Link to Baz")); + expect(screen.getByText("Link to Bar").className).toBe("active"); + expect(screen.getByText("Link to Baz").className).toBe("pending"); + + bazDfd.resolve(null); + await waitFor(() => screen.getByText("Baz page")); + expect(screen.getByText("Link to Bar").className).toBe(""); + expect(screen.getByText("Link to Baz").className).toBe("active"); + }); }); describe("NavLink under a Routes with a basename", () => { diff --git a/packages/react-router-dom/index.tsx b/packages/react-router-dom/index.tsx index 6cd01dc86f..3c9bc8283f 100644 --- a/packages/react-router-dom/index.tsx +++ b/packages/react-router-dom/index.tsx @@ -444,8 +444,9 @@ export const NavLink = React.forwardRef( let path = useResolvedPath(to, { relative: rest.relative }); let location = useLocation(); let routerState = React.useContext(DataRouterStateContext); + let { navigator } = React.useContext(NavigationContext); - let toPathname = path.pathname; + let toPathname = navigator.encodeLocation(path).pathname; let locationPathname = location.pathname; let nextLocationPathname = routerState && routerState.navigation && routerState.navigation.location From 1b2e28a46c36455c15bc41fc8da12a5761200630 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Thu, 17 Nov 2022 12:55:33 -0500 Subject: [PATCH 6/6] Bump bundle --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 46a937dd23..b29785bf16 100644 --- a/package.json +++ b/package.json @@ -116,7 +116,7 @@ "none": "14.5 kB" }, "packages/react-router-dom/dist/react-router-dom.production.min.js": { - "none": "10 kB" + "none": "10.5 kB" }, "packages/react-router-dom/dist/umd/react-router-dom.production.min.js": { "none": "16 kB"