From 532397fc9e7bfdb819bfef2ff2a1bdf26eb78afd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bernt=20R=C3=B8skar=20Brenna?= Date: Mon, 3 Oct 2022 19:58:30 +0200 Subject: [PATCH 1/8] failing test for bug 9392 --- .../__tests__/link-href-test.tsx | 45 ++++++++++++++++++- 1 file changed, 44 insertions(+), 1 deletion(-) diff --git a/packages/react-router-dom/__tests__/link-href-test.tsx b/packages/react-router-dom/__tests__/link-href-test.tsx index 9ec368fc70..f0be7b5fa2 100644 --- a/packages/react-router-dom/__tests__/link-href-test.tsx +++ b/packages/react-router-dom/__tests__/link-href-test.tsx @@ -1,6 +1,15 @@ import * as React from "react"; import * as TestRenderer from "react-test-renderer"; -import { MemoryRouter, Routes, Route, Link, Outlet } from "react-router-dom"; +import { + MemoryRouter, + Routes, + Route, + Link, + Outlet, + HashRouter, + createHashRouter, + RouterProvider +} from "react-router-dom"; describe(" href", () => { describe("in a static route", () => { @@ -679,4 +688,38 @@ describe(" href", () => { ); }); }); + + describe("when using ", () => { + test("rendered hrefs contain #", () => { + let renderer: TestRenderer.ReactTestRenderer; + TestRenderer.act(() => { + renderer = TestRenderer.create( + + + } /> + + + ); + }); + expect(renderer.root.findByType("a").props.href).toEqual("#/about"); + }); + }) + + describe("when using createHashRouter", () => { + test("rendered hrefs contain #", () => { + let renderer: TestRenderer.ReactTestRenderer; + TestRenderer.act(() => { + const router = createHashRouter([ + { + path: "/", + element: + } + ]); + renderer = TestRenderer.create( + + ); + }); + expect(renderer.root.findByType("a").props.href).toEqual("#/about"); + }); + }); }); From d32332476ee20718083b99d98dc9e2ce004b8f9a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bernt=20R=C3=B8skar=20Brenna?= Date: Mon, 3 Oct 2022 20:03:57 +0200 Subject: [PATCH 2/8] missing semicolon --- packages/react-router-dom/__tests__/link-href-test.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/react-router-dom/__tests__/link-href-test.tsx b/packages/react-router-dom/__tests__/link-href-test.tsx index f0be7b5fa2..01fd6905d4 100644 --- a/packages/react-router-dom/__tests__/link-href-test.tsx +++ b/packages/react-router-dom/__tests__/link-href-test.tsx @@ -703,7 +703,7 @@ describe(" href", () => { }); expect(renderer.root.findByType("a").props.href).toEqual("#/about"); }); - }) + }); describe("when using createHashRouter", () => { test("rendered hrefs contain #", () => { From 5f22be04f5c67e27ec5d011f8169c126ca80f5ad Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bernt=20R=C3=B8skar=20Brenna?= Date: Mon, 3 Oct 2022 20:07:23 +0200 Subject: [PATCH 3/8] CLA --- contributors.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/contributors.yml b/contributors.yml index 205fe06b05..1f2681fc79 100644 --- a/contributors.yml +++ b/contributors.yml @@ -19,6 +19,7 @@ - chensokheng - chrisngobanh - christopherchudzicki +- codeape2 - coryhouse - cvbuelow - david-crespo From 31af3cdb7e04c315f94719aee69e9df9e6861f0c Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Wed, 5 Oct 2022 16:40:49 -0700 Subject: [PATCH 4/8] fix: update createHref to be history-aware --- .../__tests__/link-href-test.tsx | 98 ++++++++++++++++++- packages/router/router.ts | 4 +- 2 files changed, 100 insertions(+), 2 deletions(-) diff --git a/packages/react-router-dom/__tests__/link-href-test.tsx b/packages/react-router-dom/__tests__/link-href-test.tsx index 9ec368fc70..41cd6028ee 100644 --- a/packages/react-router-dom/__tests__/link-href-test.tsx +++ b/packages/react-router-dom/__tests__/link-href-test.tsx @@ -1,6 +1,18 @@ import * as React from "react"; +import { createRoot } from "react-dom/client"; import * as TestRenderer from "react-test-renderer"; -import { MemoryRouter, Routes, Route, Link, Outlet } from "react-router-dom"; +import { + MemoryRouter, + Routes, + Route, + Link, + Outlet, + BrowserRouter, + HashRouter, + createBrowserRouter, + RouterProvider, + createHashRouter, +} from "react-router-dom"; describe(" href", () => { describe("in a static route", () => { @@ -679,4 +691,88 @@ describe(" href", () => { ); }); }); + + it("inside a browser router", () => { + let node: HTMLDivElement = document.createElement("div"); + document.body.appendChild(node); + TestRenderer.act(() => { + createRoot(node).render( + + + Link} + /> + + + ); + }); + + expect(node.innerHTML).toMatchInlineSnapshot( + `"Link"` + ); + + document.body.removeChild(node); + node = null!; + }); + + it("inside a hash router", () => { + let node: HTMLDivElement = document.createElement("div"); + document.body.appendChild(node); + TestRenderer.act(() => { + createRoot(node).render( + + + Link} + /> + + + ); + }); + + expect(node.innerHTML).toMatchInlineSnapshot( + `"Link"` + ); + + document.body.removeChild(node); + node = null!; + }); + + it("inside a data browser router", () => { + let node: HTMLDivElement = document.createElement("div"); + document.body.appendChild(node); + TestRenderer.act(() => { + let router = createBrowserRouter([ + { path: "/", element: Link }, + ]); + createRoot(node).render(); + }); + + expect(node.innerHTML).toMatchInlineSnapshot( + `"Link"` + ); + + document.body.removeChild(node); + node = null!; + }); + + it("inside a data hash router", () => { + let node: HTMLDivElement = document.createElement("div"); + document.body.appendChild(node); + TestRenderer.act(() => { + let router = createHashRouter([ + { path: "/", element: Link }, + ]); + createRoot(node).render(); + }); + + expect(node.innerHTML).toMatchInlineSnapshot( + `"Link"` + ); + + document.body.removeChild(node); + node = null!; + }); }); diff --git a/packages/router/router.ts b/packages/router/router.ts index 2e7e495c38..398b6c1e84 100644 --- a/packages/router/router.ts +++ b/packages/router/router.ts @@ -1739,7 +1739,9 @@ export function createRouter(init: RouterInit): Router { navigate, fetch, revalidate, - createHref, + // 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), getFetcher, deleteFetcher, dispose, From aa98e9d486f2894423a32481e40030ce80235cfc Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Thu, 6 Oct 2022 08:29:45 -0700 Subject: [PATCH 5/8] fix dup test names --- packages/react-router-dom/__tests__/link-href-test.tsx | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/react-router-dom/__tests__/link-href-test.tsx b/packages/react-router-dom/__tests__/link-href-test.tsx index 7081e4df82..36a5d9ed6d 100644 --- a/packages/react-router-dom/__tests__/link-href-test.tsx +++ b/packages/react-router-dom/__tests__/link-href-test.tsx @@ -692,7 +692,7 @@ describe(" href", () => { }); describe("when using a browser router", () => { - it("renders proper ", () => { + it("renders proper for BrowserRouter", () => { let renderer: TestRenderer.ReactTestRenderer; TestRenderer.act(() => { renderer = TestRenderer.create( @@ -708,7 +708,7 @@ describe(" href", () => { ); }); - it("renders proper ", () => { + it("renders proper for createBrowserRouter", () => { let renderer: TestRenderer.ReactTestRenderer; TestRenderer.act(() => { let router = createBrowserRouter([ @@ -726,7 +726,7 @@ describe(" href", () => { }); describe("when using a hash router", () => { - it("renders proper ", () => { + it("renders proper for HashRouter", () => { let renderer: TestRenderer.ReactTestRenderer; TestRenderer.act(() => { renderer = TestRenderer.create( @@ -742,7 +742,7 @@ describe(" href", () => { ); }); - it("renders proper ", () => { + it("renders proper for createHashRouter", () => { let renderer: TestRenderer.ReactTestRenderer; TestRenderer.act(() => { let router = createHashRouter([ From a977d6ff6f7f2e653b491c468309213734587ce5 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Thu, 13 Oct 2022 10:37:15 -0400 Subject: [PATCH 6/8] rename internal creteHref -> createServerHref for clarity --- packages/router/router.ts | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/packages/router/router.ts b/packages/router/router.ts index 398b6c1e84..c9f20fbe06 100644 --- a/packages/router/router.ts +++ b/packages/router/router.ts @@ -1880,7 +1880,7 @@ export function unstable_createStaticHandler( ): Promise | Response> { let result: DataResult; if (!actionMatch.route.action) { - let href = createHref(new URL(request.url)); + let href = createServerHref(new URL(request.url)); result = getMethodNotAllowedResult(href); } else { result = await callLoaderOrAction( @@ -2138,7 +2138,7 @@ function normalizeNavigateOptions( path, submission: { formMethod: opts.formMethod, - formAction: createHref(parsePath(path)), + formAction: createServerHref(parsePath(path)), formEncType: (opts && opts.formEncType) || "application/x-www-form-urlencoded", formData: opts.formData, @@ -2698,7 +2698,7 @@ function getNotFoundMatches(routes: AgnosticDataRouteObject[]): { } function getMethodNotAllowedResult(path: Location | string): ErrorResult { - let href = typeof path === "string" ? path : createHref(path); + let href = typeof path === "string" ? path : createServerHref(path); console.warn( "You're trying to submit to a route that does not have an action. To " + "fix this, please add an `action` function to the route for " + @@ -2725,7 +2725,7 @@ function findRedirect(results: DataResult[]): RedirectResult | undefined { } // Create an href to represent a "server" URL without the hash -function createHref(location: Partial | Location | URL) { +function createServerHref(location: Partial | Location | URL) { return (location.pathname || "") + (location.search || ""); } @@ -2850,7 +2850,8 @@ function createURL(location: Location | string): URL { typeof window !== "undefined" && typeof window.location !== "undefined" ? window.location.origin : "unknown://unknown"; - let href = typeof location === "string" ? location : createHref(location); + let href = + typeof location === "string" ? location : createServerHref(location); return new URL(href, base); } //#endregion From b99a3a05f607a2f34018746574e655f1f3dbe715 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Thu, 13 Oct 2022 10:49:52 -0400 Subject: [PATCH 7/8] Remove hash-specific hrefs from shared tests --- .../__tests__/data-browser-router-test.tsx | 293 +++++++----------- 1 file changed, 114 insertions(+), 179 deletions(-) diff --git a/packages/react-router-dom/__tests__/data-browser-router-test.tsx b/packages/react-router-dom/__tests__/data-browser-router-test.tsx index e421e8599c..aeffc33c19 100644 --- a/packages/react-router-dom/__tests__/data-browser-router-test.tsx +++ b/packages/react-router-dom/__tests__/data-browser-router-test.tsx @@ -433,7 +433,7 @@ function testDomRouter( it("handles link navigations when using a basename", async () => { let testWindow = getWindow("/base/name/foo"); - let { container } = render( + render( Link to Foo Link to Bar - +
+ +
); } assertLocation(testWindow, "/base/name/foo"); - expect(getHtml(container)).toMatchInlineSnapshot(` - "" - `); - expect(screen.getByText("Foo Heading")).toBeDefined(); + fireEvent.click(screen.getByText("Link to Bar")); await waitFor(() => screen.getByText("Bar Heading")); assertLocation(testWindow, "/base/name/bar"); @@ -508,8 +491,10 @@ function testDomRouter( return (
Link to Bar -

{navigation.state}

- +
+

{navigation.state}

+ +
); } @@ -522,62 +507,50 @@ function testDomRouter( return

{data?.message}

; } - expect(getHtml(container)).toMatchInlineSnapshot(` - "
-
- - Link to Bar - + expect(getHtml(container.querySelector("#output"))) + .toMatchInlineSnapshot(` + "

idle

Foo

-
-
" - `); +
" + `); fireEvent.click(screen.getByText("Link to Bar")); - expect(getHtml(container)).toMatchInlineSnapshot(` - "
-
- - Link to Bar - + expect(getHtml(container.querySelector("#output"))) + .toMatchInlineSnapshot(` + "

loading

Foo

-
-
" - `); +
" + `); barDefer.resolve({ message: "Bar Loader" }); await waitFor(() => screen.getByText("idle")); - expect(getHtml(container)).toMatchInlineSnapshot(` - "
-
- - Link to Bar - + expect(getHtml(container.querySelector("#output"))) + .toMatchInlineSnapshot(` + "

idle

Bar Loader

-
-
" - `); +
" + `); }); it("handles link navigations with preventScrollReset", async () => { @@ -3664,8 +3637,10 @@ function testDomRouter(
Link to Foo Link to Bar -

{navigation.state}

- +
+

{navigation.state}

+ +
); } @@ -3687,83 +3662,56 @@ function testDomRouter( return

Bar Error:{error.message}

; } - expect(getHtml(container)).toMatchInlineSnapshot(` - "
-
- - Link to Foo - - - Link to Bar - -

- idle -

-

- Foo: - hydrated from foo -

-
+ expect(getHtml(container.querySelector("#output"))) + .toMatchInlineSnapshot(` + "
+

+ idle +

+

+ Foo: + hydrated from foo +

" `); fireEvent.click(screen.getByText("Link to Bar")); barDefer.reject(new Error("Kaboom!")); await waitFor(() => screen.getByText("idle")); - expect(getHtml(container)).toMatchInlineSnapshot(` - "
-
- - Link to Foo - - - Link to Bar - -

- idle -

-

- Bar Error: - Kaboom! -

-
+ expect(getHtml(container.querySelector("#output"))) + .toMatchInlineSnapshot(` + "
+

+ idle +

+

+ Bar Error: + Kaboom! +

" `); fireEvent.click(screen.getByText("Link to Foo")); fooDefer.reject(new Error("Kaboom!")); await waitFor(() => screen.getByText("idle")); - expect(getHtml(container)).toMatchInlineSnapshot(` - "
-
- - Link to Foo - - - Link to Bar - -

- idle -

-

- Foo Error: - Kaboom! -

-
+ expect(getHtml(container.querySelector("#output"))) + .toMatchInlineSnapshot(` + "
+

+ idle +

+

+ Foo Error: + Kaboom! +

" - `); + `); }); it("renders navigation errors on parent elements", async () => { @@ -3803,8 +3751,10 @@ function testDomRouter(
Link to Foo Link to Bar -

{navigation.state}

- +
+

{navigation.state}

+ +
); } @@ -3825,27 +3775,18 @@ function testDomRouter( return

Bar:{data?.message}

; } - expect(getHtml(container)).toMatchInlineSnapshot(` - "
-
- - Link to Foo - - - Link to Bar - -

- idle -

-

- Foo: - hydrated from foo -

-
+ expect(getHtml(container.querySelector("#output"))) + .toMatchInlineSnapshot(` + "
+

+ idle +

+

+ Foo: + hydrated from foo +

" `); @@ -3893,8 +3834,10 @@ function testDomRouter( return (
Link to Bar -

{navigation.state}

- +
+

{navigation.state}

+ +
); } @@ -3908,43 +3851,35 @@ function testDomRouter( return

Bar Error:{error.message}

; } - expect(getHtml(container)).toMatchInlineSnapshot(` - "
-
- - Link to Bar - -

- idle -

-

- Foo -

-
+ expect(getHtml(container.querySelector("#output"))) + .toMatchInlineSnapshot(` + "
+

+ idle +

+

+ Foo +

" `); fireEvent.click(screen.getByText("Link to Bar")); barDefer.reject(new Error("Kaboom!")); await waitFor(() => screen.getByText("idle")); - expect(getHtml(container)).toMatchInlineSnapshot(` - "
-
- - Link to Bar - -

- idle -

-

- Bar Error: - Kaboom! -

-
+ expect(getHtml(container.querySelector("#output"))) + .toMatchInlineSnapshot(` + "
+

+ idle +

+

+ Bar Error: + Kaboom! +

" `); }); From 3054115903aa4ac6407cb36d7f8a77b524e83044 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Thu, 13 Oct 2022 10:50:58 -0400 Subject: [PATCH 8/8] add chngeset --- .changeset/funny-hotels-repeat.md | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 .changeset/funny-hotels-repeat.md diff --git a/.changeset/funny-hotels-repeat.md b/.changeset/funny-hotels-repeat.md new file mode 100644 index 0000000000..c44688ec77 --- /dev/null +++ b/.changeset/funny-hotels-repeat.md @@ -0,0 +1,6 @@ +--- +"react-router-dom": patch +"@remix-run/router": patch +--- + +Fix hrefs generated for createHashRouter