fix: handle encoding of dynamic params in descendant routes (#9589)

brophdawg11 · web-flow · commit 729cd469084c · 2022-11-18T09:52:40.000-05:00
* fix: handle encoding of dynamic params in descendant routes

* add changeset

* fix var name

* fix jsdoc

* encode pathnames in NavLink check

* Bump bundle
diff --git a/.changeset/pretty-dolls-bathe.md 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
diff --git 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"
diff --git 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__/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(
+          <BrowserRouter window={getWindow("/users/matt brophy")}>
+            <Routes>
+              <Route
+                path="/users/:name"
+                element={
+                  <>
+                    <NavLink to=".">Matt</NavLink>
+                    <NavLink to="/users/matt brophy">Matt</NavLink>
+                    <NavLink to="/users/michael jackson">Michael</NavLink>
+                  </>
+                }
+              />
+            </Routes>
+          </BrowserRouter>
+        );
+      });
+
+      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(
+        <Route path="/" element={<Layout />}>
+          <Route path="foo" element={<p>Foo page</p>} />
+          <Route
+            path="bar/:param"
+            loader={() => barDfd.promise}
+            element={<p>Bar page</p>}
+          />
+          <Route
+            path="baz-✅"
+            loader={() => bazDfd.promise}
+            element={<p>Baz page</p>}
+          />
+        </Route>
+      ),
+      {
+        window: getWindow("/foo"),
+      }
+    );
+    render(<RouterProvider router={router} />);
+
+    function Layout() {
+      return (
+        <>
+          <NavLink to="/foo">Link to Foo</NavLink>
+          <NavLink to="/bar/matt brophy">Link to Bar</NavLink>
+          <NavLink to="/baz-✅">Link to Baz</NavLink>
+          <Outlet />
+        </>
+      );
+    }
+
+    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/__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={navigatePath}>Link to path</Link>}
           />
+          <Route
+            path="/descendant/:param/*"
+            element={
+              <Routes>
+                <Route
+                  path="match"
+                  element={<Comp heading="Descendant Route" />}
+                />
+              </Routes>
+            }
+          />
           <Route path="/*" element={<Comp heading="Root Splat Route" />} />
         </>
       );
@@ -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/index.tsx b/packages/react-router-dom/index.tsx
@@ -444,8 +444,9 @@ export const NavLink = React.forwardRef<HTMLAnchorElement, NavLinkProps>(
     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
diff --git 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
@@ -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
@@ -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
@@ -310,6 +310,7 @@ export function useRoutes(
     `useRoutes() may be used only in the context of a <Router> 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
@@ -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
@@ -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 destination path according to the internal
+   * history implementation
+   * @param to
+   */
+  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: (to: To) => init.history.encodeLocation(to),
     getFetcher,
     deleteFetcher,
     dispose,
