From 2f24a6d97d7970f72ed2c7af3e49c4a06b3a6839 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Mon, 4 Dec 2023 15:34:39 -0500 Subject: [PATCH 1/6] Add future.v7_relativeSplatPath flag This commit started with an initial revert of commit 9cfd99da2ff4c265935a1a104fad548596b6db14 --- packages/react-router-dom/index.tsx | 12 +- packages/react-router-dom/server.tsx | 18 +- .../__tests__/useResolvedPath-test.tsx | 310 +++++++++++++++++- packages/react-router/lib/components.tsx | 33 +- packages/react-router/lib/context.ts | 3 + packages/react-router/lib/hooks.tsx | 10 +- packages/router/index.ts | 2 +- packages/router/router.ts | 53 ++- packages/router/utils.ts | 19 ++ 9 files changed, 435 insertions(+), 25 deletions(-) diff --git a/packages/react-router-dom/index.tsx b/packages/react-router-dom/index.tsx index c3a1bd947c..9b21e4c09a 100644 --- a/packages/react-router-dom/index.tsx +++ b/packages/react-router-dom/index.tsx @@ -657,6 +657,9 @@ export function RouterProvider({ navigator, static: false, basename, + future: { + v7_relativeSplatPath: router.future.v7_relativeSplatPath, + }, }), [router, navigator, basename] ); @@ -749,6 +752,7 @@ export function BrowserRouter({ location={state.location} navigationType={state.action} navigator={history} + future={future} /> ); } @@ -799,6 +803,7 @@ export function HashRouter({ location={state.location} navigationType={state.action} navigator={history} + future={future} /> ); } @@ -845,6 +850,7 @@ function HistoryRouter({ location={state.location} navigationType={state.action} navigator={history} + future={future} /> ); } @@ -1543,10 +1549,8 @@ export function useFormAction( // object referenced by useMemo inside useResolvedPath let path = { ...useResolvedPath(action ? action : ".", { relative }) }; - // Previously we set the default action to ".". The problem with this is that - // `useResolvedPath(".")` excludes search params of the resolved URL. This is - // the intended behavior of when "." is specifically provided as - // the form action, but inconsistent w/ browsers when the action is omitted. + // If no action was specified, browsers will persist current search params + // when determining the path, so match that behavior // https://github.com/remix-run/remix/issues/927 let location = useLocation(); if (action == null) { diff --git a/packages/react-router-dom/server.tsx b/packages/react-router-dom/server.tsx index 99fc1426dc..42a926bb81 100644 --- a/packages/react-router-dom/server.tsx +++ b/packages/react-router-dom/server.tsx @@ -7,6 +7,7 @@ import type { CreateStaticHandlerOptions as RouterCreateStaticHandlerOptions, UNSAFE_RouteManifest as RouteManifest, RouterState, + FutureConfig as RouterFutureConfig, } from "@remix-run/router"; import { IDLE_BLOCKER, @@ -109,6 +110,9 @@ export function StaticRouterProvider({ static: true, staticContext: context, basename: context.basename || "/", + future: { + v7_relativeSplatPath: router.future.v7_relativeSplatPath, + }, }; let fetchersContext = new Map(); @@ -260,7 +264,11 @@ export function createStaticHandler( export function createStaticRouter( routes: RouteObject[], - context: StaticHandlerContext + context: StaticHandlerContext, + opts: { + // Only accept future flags that impact the server render + future?: Partial>; + } = {} ): RemixRouter { let manifest: RouteManifest = {}; let dataRoutes = convertRoutesToDataRoutes( @@ -288,6 +296,14 @@ export function createStaticRouter( get basename() { return context.basename; }, + get future() { + return { + v7_fetcherPersist: false, + v7_normalizeFormMethod: false, + v7_prependBasename: false, + v7_relativeSplatPath: opts.future?.v7_relativeSplatPath === true, + }; + }, get state() { return { historyAction: Action.Pop, diff --git a/packages/react-router/__tests__/useResolvedPath-test.tsx b/packages/react-router/__tests__/useResolvedPath-test.tsx index d6615e865f..12a9468987 100644 --- a/packages/react-router/__tests__/useResolvedPath-test.tsx +++ b/packages/react-router/__tests__/useResolvedPath-test.tsx @@ -1,7 +1,14 @@ import * as React from "react"; import * as TestRenderer from "react-test-renderer"; import type { Path } from "react-router"; -import { MemoryRouter, Routes, Route, useResolvedPath } from "react-router"; +import { + MemoryRouter, + Routes, + Route, + useResolvedPath, + useLocation, +} from "react-router"; +import { prettyDOM, render } from "@testing-library/react"; function ShowResolvedPath({ path }: { path: string | Path }) { return 
{JSON.stringify(useResolvedPath(path))}
; @@ -85,6 +92,9 @@ describe("useResolvedPath", () => { }); describe("in a splat route", () => { + // Note: This test asserts long-standing buggy behavior fixed by enabling + // the future.v7_relativeSplatPath flag. See: + // https://github.com/remix-run/react-router/issues/11052#issuecomment-1836589329 it("resolves . to the route path", () => { let renderer: TestRenderer.ReactTestRenderer; TestRenderer.act(() => { @@ -105,5 +115,303 @@ describe("useResolvedPath", () => { `); }); + + it("resolves .. to the parent route path", () => { + let renderer: TestRenderer.ReactTestRenderer; + TestRenderer.act(() => { + renderer = TestRenderer.create( + + + + } /> + + + + ); + }); + + expect(renderer.toJSON()).toMatchInlineSnapshot(` + 
+          {"pathname":"/users","search":"","hash":""}
+
+ `); + }); + + it("resolves . to the route path (descendant route)", () => { + let renderer: TestRenderer.ReactTestRenderer; + TestRenderer.act(() => { + renderer = TestRenderer.create( + + + + } /> + + } + /> + + + ); + }); + + expect(renderer.toJSON()).toMatchInlineSnapshot(` + 
+          {"pathname":"/users/mj","search":"","hash":""}
+
+ `); + }); + + it("resolves .. to the parent route path (descendant route)", () => { + let renderer: TestRenderer.ReactTestRenderer; + TestRenderer.act(() => { + renderer = TestRenderer.create( + + + + } /> + + } + /> + + + ); + }); + + expect(renderer.toJSON()).toMatchInlineSnapshot(` + 
+          {"pathname":"/users","search":"","hash":""}
+
+ `); + }); + }); + + describe("in a param route", () => { + it("resolves . to the route path", () => { + let renderer: TestRenderer.ReactTestRenderer; + TestRenderer.act(() => { + renderer = TestRenderer.create( + + + + } /> + + + + ); + }); + + expect(renderer.toJSON()).toMatchInlineSnapshot(` + 
+          {"pathname":"/users/mj","search":"","hash":""}
+
+ `); + }); + + it("resolves .. to the parent route", () => { + let renderer: TestRenderer.ReactTestRenderer; + TestRenderer.act(() => { + renderer = TestRenderer.create( + + + + } /> + + + + ); + }); + + expect(renderer.toJSON()).toMatchInlineSnapshot(` + 
+          {"pathname":"/users","search":"","hash":""}
+
+ `); + }); + }); + + // See: https://github.com/remix-run/react-router/issues/11052#issuecomment-1836589329 + describe("future.v7_relativeSplatPath", () => { + function App({ enableFlag }: { enableFlag: boolean }) { + let routeConfigs = [ + { + routes: ( + } + /> + ), + }, + { + routes: ( + } + /> + ), + }, + { + routes: ( + + + } + /> + + ), + }, + { + routes: ( + } + /> + ), + }, + { + routes: ( + + + } + /> + + ), + }, + ]; + + return ( + <> + {routeConfigs.map((config, idx) => ( + + {config.routes} + + ))} + + ); + } + + function Component({ desc }) { + return ( + <> + {`--- Routes: ${desc} ---`} + {`useLocation(): ${useLocation().pathname}`} + {`useResolvedPath('.'): ${useResolvedPath(".").pathname}`} + {`useResolvedPath('..'): ${useResolvedPath("..").pathname}`} + {`useResolvedPath('baz/qux'): ${useResolvedPath("baz/qux").pathname}`} + {`useResolvedPath('./baz/qux'): ${ + useResolvedPath("./baz/qux").pathname + }\n`} + + ); + } + + it("when disabled, resolves splat route relative paths differently than other routes", async () => { + let { container } = render(); + let html = getHtml(container); + html = html ? html.replace(/</g, "<").replace(/>/g, ">") : html; + expect(html).toMatchInlineSnapshot(` + "
+ --- Routes: --- + useLocation(): /foo/bar + useResolvedPath('.'): /foo/bar + useResolvedPath('..'): / + useResolvedPath('baz/qux'): /foo/bar/baz/qux + useResolvedPath('./baz/qux'): /foo/bar/baz/qux + + --- Routes: --- + useLocation(): /foo/bar + useResolvedPath('.'): /foo/bar + useResolvedPath('..'): / + useResolvedPath('baz/qux'): /foo/bar/baz/qux + useResolvedPath('./baz/qux'): /foo/bar/baz/qux + + --- Routes: --- + useLocation(): /foo/bar + useResolvedPath('.'): /foo/bar + useResolvedPath('..'): /foo + useResolvedPath('baz/qux'): /foo/bar/baz/qux + useResolvedPath('./baz/qux'): /foo/bar/baz/qux + + --- Routes: --- + useLocation(): /foo/bar + useResolvedPath('.'): /foo + useResolvedPath('..'): / + useResolvedPath('baz/qux'): /foo/baz/qux + useResolvedPath('./baz/qux'): /foo/baz/qux + + --- Routes: --- + useLocation(): /foo/bar + useResolvedPath('.'): /foo + useResolvedPath('..'): /foo + useResolvedPath('baz/qux'): /foo/baz/qux + useResolvedPath('./baz/qux'): /foo/baz/qux + +
" + `); + }); + + it("when enabled, resolves splat route relative paths differently than other routes", async () => { + let { container } = render(); + let html = getHtml(container); + html = html ? html.replace(/</g, "<").replace(/>/g, ">") : html; + expect(html).toMatchInlineSnapshot(` + "
+ --- Routes: --- + useLocation(): /foo/bar + useResolvedPath('.'): /foo/bar + useResolvedPath('..'): / + useResolvedPath('baz/qux'): /foo/bar/baz/qux + useResolvedPath('./baz/qux'): /foo/bar/baz/qux + + --- Routes: --- + useLocation(): /foo/bar + useResolvedPath('.'): /foo/bar + useResolvedPath('..'): / + useResolvedPath('baz/qux'): /foo/bar/baz/qux + useResolvedPath('./baz/qux'): /foo/bar/baz/qux + + --- Routes: --- + useLocation(): /foo/bar + useResolvedPath('.'): /foo/bar + useResolvedPath('..'): /foo + useResolvedPath('baz/qux'): /foo/bar/baz/qux + useResolvedPath('./baz/qux'): /foo/bar/baz/qux + + --- Routes: --- + useLocation(): /foo/bar + useResolvedPath('.'): /foo/bar + useResolvedPath('..'): / + useResolvedPath('baz/qux'): /foo/bar/baz/qux + useResolvedPath('./baz/qux'): /foo/bar/baz/qux + + --- Routes: --- + useLocation(): /foo/bar + useResolvedPath('.'): /foo/bar + useResolvedPath('..'): /foo + useResolvedPath('baz/qux'): /foo/bar/baz/qux + useResolvedPath('./baz/qux'): /foo/bar/baz/qux + +
" + `); + }); }); }); + +function getHtml(container: HTMLElement) { + return prettyDOM(container, undefined, { + highlight: false, + }); +} diff --git a/packages/react-router/lib/components.tsx b/packages/react-router/lib/components.tsx index 3c4a95e2f2..2d81447a53 100644 --- a/packages/react-router/lib/components.tsx +++ b/packages/react-router/lib/components.tsx @@ -14,7 +14,7 @@ import { AbortedDeferredError, Action as NavigationType, createMemoryHistory, - UNSAFE_getPathContributingMatches as getPathContributingMatches, + UNSAFE_getResolveToMatches as getResolveToMatches, UNSAFE_invariant as invariant, parsePath, resolveTo, @@ -51,13 +51,16 @@ import { } from "./hooks"; export interface FutureConfig { + v7_relativeSplatPath: boolean; v7_startTransition: boolean; } export interface RouterProviderProps { fallbackElement?: React.ReactNode; router: RemixRouter; - future?: Partial; + // Only accept future flags relevant to rendering behavior + // routing flags should be accessed via router.future + future?: Partial>; } /** @@ -137,6 +140,9 @@ export function RouterProvider({ navigator, static: false, basename, + future: { + v7_relativeSplatPath: router.future.v7_relativeSplatPath, + }, }), [router, navigator, basename] ); @@ -233,6 +239,7 @@ export function MemoryRouter({ location={state.location} navigationType={state.action} navigator={history} + future={future} /> ); } @@ -266,8 +273,10 @@ export function Navigate({ ` may be used only in the context of a component.` ); + let { future, static: isStatic } = React.useContext(NavigationContext); + warning( - !React.useContext(NavigationContext).static, + !isStatic, ` must not be used on the initial render in a . ` + `This is a no-op, but you should modify your code so the is ` + `only ever rendered in response to some user interaction or state change.` @@ -281,7 +290,7 @@ export function Navigate({ // StrictMode they navigate to the same place let path = resolveTo( to, - getPathContributingMatches(matches).map((match) => match.pathnameBase), + getResolveToMatches(matches, future.v7_relativeSplatPath), locationPathname, relative === "path" ); @@ -368,6 +377,9 @@ export interface RouterProps { navigationType?: NavigationType; navigator: Navigator; static?: boolean; + future?: { + v7_relativeSplatPath?: boolean; + }; } /** @@ -386,6 +398,7 @@ export function Router({ navigationType = NavigationType.Pop, navigator, static: staticProp = false, + future, }: RouterProps): React.ReactElement | null { invariant( !useInRouterContext(), @@ -397,8 +410,16 @@ export function Router({ // the enforcement of trailing slashes throughout the app let basename = basenameProp.replace(/^\/*/, "/"); let navigationContext = React.useMemo( - () => ({ basename, navigator, static: staticProp }), - [basename, navigator, staticProp] + () => ({ + basename, + navigator, + static: staticProp, + future: { + v7_relativeSplatPath: false, + ...future, + }, + }), + [basename, future, navigator, staticProp] ); if (typeof locationProp === "string") { diff --git a/packages/react-router/lib/context.ts b/packages/react-router/lib/context.ts index 3e47ac2d05..63b0f5f864 100644 --- a/packages/react-router/lib/context.ts +++ b/packages/react-router/lib/context.ts @@ -120,6 +120,9 @@ interface NavigationContextObject { basename: string; navigator: Navigator; static: boolean; + future: { + v7_relativeSplatPath: boolean; + }; } export const NavigationContext = React.createContext( diff --git a/packages/react-router/lib/hooks.tsx b/packages/react-router/lib/hooks.tsx index 390553abee..a945658704 100644 --- a/packages/react-router/lib/hooks.tsx +++ b/packages/react-router/lib/hooks.tsx @@ -18,7 +18,7 @@ import { IDLE_BLOCKER, Action as NavigationType, UNSAFE_convertRouteMatchToUiMatch as convertRouteMatchToUiMatch, - UNSAFE_getPathContributingMatches as getPathContributingMatches, + UNSAFE_getResolveToMatches as getResolveToMatches, UNSAFE_invariant as invariant, isRouteErrorResponse, joinPaths, @@ -193,12 +193,12 @@ function useNavigateUnstable(): NavigateFunction { ); let dataRouterContext = React.useContext(DataRouterContext); - let { basename, navigator } = React.useContext(NavigationContext); + let { basename, future, navigator } = React.useContext(NavigationContext); let { matches } = React.useContext(RouteContext); let { pathname: locationPathname } = useLocation(); let routePathnamesJson = JSON.stringify( - getPathContributingMatches(matches).map((match) => match.pathnameBase) + getResolveToMatches(matches, future.v7_relativeSplatPath) ); let activeRef = React.useRef(false); @@ -309,11 +309,11 @@ export function useResolvedPath( to: To, { relative }: { relative?: RelativeRoutingType } = {} ): Path { + let { future } = React.useContext(NavigationContext); let { matches } = React.useContext(RouteContext); let { pathname: locationPathname } = useLocation(); - let routePathnamesJson = JSON.stringify( - getPathContributingMatches(matches).map((match) => match.pathnameBase) + getResolveToMatches(matches, future.v7_relativeSplatPath) ); return React.useMemo( diff --git a/packages/router/index.ts b/packages/router/index.ts index cbacea8bf8..060360d34a 100644 --- a/packages/router/index.ts +++ b/packages/router/index.ts @@ -87,7 +87,7 @@ export { ErrorResponseImpl as UNSAFE_ErrorResponseImpl, convertRoutesToDataRoutes as UNSAFE_convertRoutesToDataRoutes, convertRouteMatchToUiMatch as UNSAFE_convertRouteMatchToUiMatch, - getPathContributingMatches as UNSAFE_getPathContributingMatches, + getResolveToMatches as UNSAFE_getResolveToMatches, } from "./utils"; export { diff --git a/packages/router/router.ts b/packages/router/router.ts index 21453256af..491bc8afc6 100644 --- a/packages/router/router.ts +++ b/packages/router/router.ts @@ -40,6 +40,7 @@ import { convertRouteMatchToUiMatch, convertRoutesToDataRoutes, getPathContributingMatches, + getResolveToMatches, immutableRouteKeys, isRouteErrorResponse, joinPaths, @@ -64,6 +65,14 @@ export interface Router { */ get basename(): RouterInit["basename"]; + /** + * @internal + * PRIVATE - DO NOT USE + * + * Return the router future flags + */ + get future(): FutureConfig; + /** * @internal * PRIVATE - DO NOT USE @@ -346,6 +355,7 @@ export interface FutureConfig { v7_fetcherPersist: boolean; v7_normalizeFormMethod: boolean; v7_prependBasename: boolean; + v7_relativeSplatPath: boolean; } /** @@ -770,6 +780,7 @@ export function createRouter(init: RouterInit): Router { v7_fetcherPersist: false, v7_normalizeFormMethod: false, v7_prependBasename: false, + v7_relativeSplatPath: false, ...init.future, }; // Cleanup function for history @@ -1231,6 +1242,7 @@ export function createRouter(init: RouterInit): Router { basename, future.v7_prependBasename, to, + future.v7_relativeSplatPath, opts?.fromRouteId, opts?.relative ); @@ -1545,7 +1557,8 @@ export function createRouter(init: RouterInit): Router { matches, manifest, mapRouteProperties, - basename + basename, + future.v7_relativeSplatPath ); if (request.signal.aborted) { @@ -1824,6 +1837,7 @@ export function createRouter(init: RouterInit): Router { basename, future.v7_prependBasename, href, + future.v7_relativeSplatPath, routeId, opts?.relative ); @@ -1930,7 +1944,8 @@ export function createRouter(init: RouterInit): Router { requestMatches, manifest, mapRouteProperties, - basename + basename, + future.v7_relativeSplatPath ); if (fetchRequest.signal.aborted) { @@ -2173,7 +2188,8 @@ export function createRouter(init: RouterInit): Router { matches, manifest, mapRouteProperties, - basename + basename, + future.v7_relativeSplatPath ); // Deferred isn't supported for fetcher loads, await everything and treat it @@ -2369,7 +2385,8 @@ export function createRouter(init: RouterInit): Router { matches, manifest, mapRouteProperties, - basename + basename, + future.v7_relativeSplatPath ) ), ...fetchersToLoad.map((f) => { @@ -2381,7 +2398,8 @@ export function createRouter(init: RouterInit): Router { f.matches, manifest, mapRouteProperties, - basename + basename, + future.v7_relativeSplatPath ); } else { let error: ErrorResult = { @@ -2723,6 +2741,9 @@ export function createRouter(init: RouterInit): Router { get basename() { return basename; }, + get future() { + return future; + }, get state() { return state; }, @@ -2764,6 +2785,13 @@ export function createRouter(init: RouterInit): Router { export const UNSAFE_DEFERRED_SYMBOL = Symbol("deferred"); +/** + * Future flags to toggle new feature behavior + */ +export interface StaticHandlerFutureConfig { + v7_relativeSplatPath: boolean; +} + export interface CreateStaticHandlerOptions { basename?: string; /** @@ -2771,6 +2799,7 @@ export interface CreateStaticHandlerOptions { */ detectErrorBoundary?: DetectErrorBoundaryFunction; mapRouteProperties?: MapRoutePropertiesFunction; + future?: Partial; } export function createStaticHandler( @@ -2796,6 +2825,11 @@ export function createStaticHandler( } else { mapRouteProperties = defaultMapRouteProperties; } + // Config driven behavior flags + let future: StaticHandlerFutureConfig = { + v7_relativeSplatPath: false, + ...(opts ? opts.future : null), + }; let dataRoutes = convertRoutesToDataRoutes( routes, @@ -3058,6 +3092,7 @@ export function createStaticHandler( manifest, mapRouteProperties, basename, + future.v7_relativeSplatPath, { isStaticRequest: true, isRouteRequest, requestContext } ); @@ -3226,6 +3261,7 @@ export function createStaticHandler( manifest, mapRouteProperties, basename, + future.v7_relativeSplatPath, { isStaticRequest: true, isRouteRequest, requestContext } ) ), @@ -3316,6 +3352,7 @@ function normalizeTo( basename: string, prependBasename: boolean, to: To | null, + v7_relativeSplatPath: boolean, fromRouteId?: string, relative?: RelativeRoutingType ) { @@ -3340,7 +3377,7 @@ function normalizeTo( // Resolve the relative path let path = resolveTo( to ? to : ".", - getPathContributingMatches(contextualMatches).map((m) => m.pathnameBase), + getResolveToMatches(contextualMatches, v7_relativeSplatPath), stripBasename(location.pathname, basename) || location.pathname, relative === "path" ); @@ -3830,6 +3867,7 @@ async function callLoaderOrAction( manifest: RouteManifest, mapRouteProperties: MapRoutePropertiesFunction, basename: string, + v7_relativeSplatPath: boolean, opts: { isStaticRequest?: boolean; isRouteRequest?: boolean; @@ -3943,7 +3981,8 @@ async function callLoaderOrAction( matches.slice(0, matches.indexOf(match) + 1), basename, true, - location + location, + v7_relativeSplatPath ); } else if (!opts.isStaticRequest) { // Strip off the protocol+origin for same-origin + same-basename absolute diff --git a/packages/router/utils.ts b/packages/router/utils.ts index ccbe5537b7..dd86fcd4f1 100644 --- a/packages/router/utils.ts +++ b/packages/router/utils.ts @@ -1145,6 +1145,25 @@ export function getPathContributingMatches< ); } +// Return the array of pathnames for the current route matches - used to +// generate the routePathnames input for resolveTo() +export function getResolveToMatches< + T extends AgnosticRouteMatch = AgnosticRouteMatch +>(matches: T[], v7_relativeSplatPath: boolean) { + let pathMatches = getPathContributingMatches(matches); + + // When v7_relativeSplatPath is enabled, use the full pathname for the leaf + // match so we include splat values for "." links. See: + // https://github.com/remix-run/react-router/issues/11052#issuecomment-1836589329 + if (v7_relativeSplatPath) { + return pathMatches.map((match, idx) => + idx === matches.length - 1 ? match.pathname : match.pathnameBase + ); + } + + return pathMatches.map((match) => match.pathnameBase); +} + /** * @private */ From 403fa3c1169a5b600836a63b40eed65742e5b26b Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Mon, 4 Dec 2023 17:10:00 -0500 Subject: [PATCH 2/6] Add StaticRouter future flag --- packages/react-router-dom-v5-compat/index.ts | 1 + packages/react-router-dom/index.tsx | 1 + packages/react-router-dom/server.tsx | 4 ++++ packages/react-router-native/index.tsx | 1 + 4 files changed, 7 insertions(+) diff --git a/packages/react-router-dom-v5-compat/index.ts b/packages/react-router-dom-v5-compat/index.ts index a9e2a15c41..6de7e9dee8 100644 --- a/packages/react-router-dom-v5-compat/index.ts +++ b/packages/react-router-dom-v5-compat/index.ts @@ -59,6 +59,7 @@ export type { FormEncType, FormMethod, FormProps, + FutureConfig, GetScrollRestorationKeyFunction, Hash, HashRouterProps, diff --git a/packages/react-router-dom/index.tsx b/packages/react-router-dom/index.tsx index 9b21e4c09a..951287c97d 100644 --- a/packages/react-router-dom/index.tsx +++ b/packages/react-router-dom/index.tsx @@ -104,6 +104,7 @@ export type { DataRouteObject, ErrorResponse, Fetcher, + FutureConfig, Hash, IndexRouteObject, IndexRouteProps, diff --git a/packages/react-router-dom/server.tsx b/packages/react-router-dom/server.tsx index 42a926bb81..a17aaec594 100644 --- a/packages/react-router-dom/server.tsx +++ b/packages/react-router-dom/server.tsx @@ -25,6 +25,7 @@ import { } from "react-router"; import type { DataRouteObject, + FutureConfig, Location, RouteObject, To, @@ -43,6 +44,7 @@ export interface StaticRouterProps { basename?: string; children?: React.ReactNode; location: Partial | string; + future?: Partial; } /** @@ -53,6 +55,7 @@ export function StaticRouter({ basename, children, location: locationProp = "/", + future, }: StaticRouterProps) { if (typeof locationProp === "string") { locationProp = parsePath(locationProp); @@ -75,6 +78,7 @@ export function StaticRouter({ location={location} navigationType={action} navigator={staticNavigator} + future={future} static={true} /> ); diff --git a/packages/react-router-native/index.tsx b/packages/react-router-native/index.tsx index 6c52f5efde..eb27d56bfe 100644 --- a/packages/react-router-native/index.tsx +++ b/packages/react-router-native/index.tsx @@ -29,6 +29,7 @@ export type { DataRouteObject, ErrorResponse, Fetcher, + FutureConfig, Hash, IndexRouteObject, IndexRouteProps, From 312c948536cf9fc272d2f0b563ff51e926d2432d Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Mon, 4 Dec 2023 17:46:42 -0500 Subject: [PATCH 3/6] Add changeset --- .changeset/relative-splat-path.md | 138 ++++++++++++++++++++++++++++++ 1 file changed, 138 insertions(+) create mode 100644 .changeset/relative-splat-path.md diff --git a/.changeset/relative-splat-path.md b/.changeset/relative-splat-path.md new file mode 100644 index 0000000000..5de9b68600 --- /dev/null +++ b/.changeset/relative-splat-path.md @@ -0,0 +1,138 @@ +--- +"react-router-dom-v5-compat": minor +"react-router-native": minor +"react-router-dom": minor +"react-router": minor +"@remix-run/router": minor +--- + +Add a new `future.v7_relativeSplatPath` flag to implenent a breaking bug fix to relative routing when inside a splat route. + +This fix was originally added in [#10983](https://github.com/remix-run/react-router/issues/10983) and was later reverted in [#11078](https://github.com/remix-run/react-router/issues/110788) because it was determined that a large number of existing applications were relying on the buggy behavior (see [#11052](https://github.com/remix-run/react-router/issues/11052)) + +**The Bug** +The buggy behavior is that without this flag, the default behavior when resolving relative paths is to _ignore_ any splat (`*`) portion of the current route path. + +**The Background** +This decision was originally made thinking that it would make the concept of nested different sections of your apps in `` easier if relative routing would _replace_ the current splat: + +```jsx + + + } /> + } /> + + +``` + +Any paths like `/dashboard`, `/dashboard/team`, `/dashboard/projects` will match the `Dashboard` route. The dashboard component itself can then render nested ``: + +```jsx +function Dashboard() { + return ( +
+

Dashboard

+ + + + } /> + } /> + } /> + +
+ ); +} +``` + +Now, all links and route paths are relative to the router above them. This makes code splitting and compartmentalizing your app really easy. You could render the `Dashboard` as its own independent app, or embed it into your large app without making any changes to it. + +**The Problem** + +The problem is that this concept of ignoring part of a pth breaks a lot of other assumptions in React Router - namely that `"."` always means the current location pathname for that route. When we ignore the splat portion, we start getting invalid paths when using `"."`: + +```jsx +// If we are on URL /dashboard/team, and we want to link to /dashboard/team: +function DashboardTeam() { + // ❌ This is broken and results in + return A broken link to the Current URL; + + // ✅ This is fixed but super unintuitive since we're already at /dashboard/team! + return A broken link to the Current URL; +} +``` + +We've also introduced an issue that we can no longer move our `DashboardTeam` component around our route hierarchy easily - since it behaves differently if we're underneath a non-splat route, such as `/dashboard/:widget`. Now, our `"."` links will, properly point to ourself _inclusive of the dynamic param value_ so behavior will break from it's corresponding usage in a `/dashboard/*` route. + +Even worse, consider a nested splat route configuration: + +```jsx + + + + } /> + + + +``` + +Now, a `` and a `` inside the `Dashboard` component go to the same place! That is definitely not correct! + +Another common issue arose in Data Routers (and Remix) where any `
` should post to it's own route `action` if you the user doesn't specify a form action: + +```jsx +let router = createBrowserRouter({ + path: "/dashboard", + children: [ + { + path: "*", + action: dashboardAction, + Component() { + // ❌ This form is broken! It throws a 405 error when it submits because + // it tries to submit to /dashboard (without the splat value) and the parent + // `/dashboard` route doesn't have an action + return ...
; + }, + }, + ], +}); +``` + +This is just a compounded issue from the above because the default location for a `Form` to submit to is itself (`"."`) - and if we ignore the splat portion, that now resolves to the parent route. + +**The Solution** +If you are leveraging this behavior, it's recommended to enable the future flag, move your splat to it's own route, and leverage `../` for any links to "sibling" pages: + +```jsx + + + + } /> + + + + +function Dashboard() { + return ( +
+

Dashboard

+ + + + } /> + } /> + } /> + +
+ ); +} +``` + +This way, `.` means "the full current pathname for my route" in all cases (including static, dynamic, and splat routes) and `..` always means "my parents pathname". From cdf40cfb09cf63b1f8bcedd768394fa5254bbfa0 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Mon, 4 Dec 2023 18:22:57 -0500 Subject: [PATCH 4/6] Bump bundle --- package.json | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/package.json b/package.json index efb420939f..add36b5a32 100644 --- a/package.json +++ b/package.json @@ -110,19 +110,19 @@ }, "filesize": { "packages/router/dist/router.umd.min.js": { - "none": "49.8 kB" + "none": "50.1 kB" }, "packages/react-router/dist/react-router.production.min.js": { - "none": "14.5 kB" + "none": "14.6 kB" }, "packages/react-router/dist/umd/react-router.production.min.js": { - "none": "16.9 kB" + "none": "17.0 kB" }, "packages/react-router-dom/dist/react-router-dom.production.min.js": { - "none": "16.7 kB" + "none": "16.8 kB" }, "packages/react-router-dom/dist/umd/react-router-dom.production.min.js": { - "none": "22.9 kB" + "none": "23.0 kB" } } } From 51f5f7f9449a1d57f7dd327a3ec738ab63bad1b6 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Tue, 5 Dec 2023 10:41:34 -0500 Subject: [PATCH 5/6] Add example of useResolvedPath relative:path with the flag enabled --- .../react-router/__tests__/useResolvedPath-test.tsx | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/packages/react-router/__tests__/useResolvedPath-test.tsx b/packages/react-router/__tests__/useResolvedPath-test.tsx index 12a9468987..a26a5c867d 100644 --- a/packages/react-router/__tests__/useResolvedPath-test.tsx +++ b/packages/react-router/__tests__/useResolvedPath-test.tsx @@ -310,6 +310,9 @@ describe("useResolvedPath", () => { {`useLocation(): ${useLocation().pathname}`} {`useResolvedPath('.'): ${useResolvedPath(".").pathname}`} {`useResolvedPath('..'): ${useResolvedPath("..").pathname}`} + {`useResolvedPath('..', { relative: 'path' }): ${ + useResolvedPath("..", { relative: "path" }).pathname + }`} {`useResolvedPath('baz/qux'): ${useResolvedPath("baz/qux").pathname}`} {`useResolvedPath('./baz/qux'): ${ useResolvedPath("./baz/qux").pathname @@ -328,6 +331,7 @@ describe("useResolvedPath", () => { useLocation(): /foo/bar useResolvedPath('.'): /foo/bar useResolvedPath('..'): / + useResolvedPath('..', { relative: 'path' }): /foo useResolvedPath('baz/qux'): /foo/bar/baz/qux useResolvedPath('./baz/qux'): /foo/bar/baz/qux @@ -335,6 +339,7 @@ describe("useResolvedPath", () => { useLocation(): /foo/bar useResolvedPath('.'): /foo/bar useResolvedPath('..'): / + useResolvedPath('..', { relative: 'path' }): /foo useResolvedPath('baz/qux'): /foo/bar/baz/qux useResolvedPath('./baz/qux'): /foo/bar/baz/qux @@ -342,6 +347,7 @@ describe("useResolvedPath", () => { useLocation(): /foo/bar useResolvedPath('.'): /foo/bar useResolvedPath('..'): /foo + useResolvedPath('..', { relative: 'path' }): /foo useResolvedPath('baz/qux'): /foo/bar/baz/qux useResolvedPath('./baz/qux'): /foo/bar/baz/qux @@ -349,6 +355,7 @@ describe("useResolvedPath", () => { useLocation(): /foo/bar useResolvedPath('.'): /foo useResolvedPath('..'): / + useResolvedPath('..', { relative: 'path' }): / useResolvedPath('baz/qux'): /foo/baz/qux useResolvedPath('./baz/qux'): /foo/baz/qux @@ -356,6 +363,7 @@ describe("useResolvedPath", () => { useLocation(): /foo/bar useResolvedPath('.'): /foo useResolvedPath('..'): /foo + useResolvedPath('..', { relative: 'path' }): / useResolvedPath('baz/qux'): /foo/baz/qux useResolvedPath('./baz/qux'): /foo/baz/qux @@ -373,6 +381,7 @@ describe("useResolvedPath", () => { useLocation(): /foo/bar useResolvedPath('.'): /foo/bar useResolvedPath('..'): / + useResolvedPath('..', { relative: 'path' }): /foo useResolvedPath('baz/qux'): /foo/bar/baz/qux useResolvedPath('./baz/qux'): /foo/bar/baz/qux @@ -380,6 +389,7 @@ describe("useResolvedPath", () => { useLocation(): /foo/bar useResolvedPath('.'): /foo/bar useResolvedPath('..'): / + useResolvedPath('..', { relative: 'path' }): /foo useResolvedPath('baz/qux'): /foo/bar/baz/qux useResolvedPath('./baz/qux'): /foo/bar/baz/qux @@ -387,6 +397,7 @@ describe("useResolvedPath", () => { useLocation(): /foo/bar useResolvedPath('.'): /foo/bar useResolvedPath('..'): /foo + useResolvedPath('..', { relative: 'path' }): /foo useResolvedPath('baz/qux'): /foo/bar/baz/qux useResolvedPath('./baz/qux'): /foo/bar/baz/qux @@ -394,6 +405,7 @@ describe("useResolvedPath", () => { useLocation(): /foo/bar useResolvedPath('.'): /foo/bar useResolvedPath('..'): / + useResolvedPath('..', { relative: 'path' }): /foo useResolvedPath('baz/qux'): /foo/bar/baz/qux useResolvedPath('./baz/qux'): /foo/bar/baz/qux @@ -401,6 +413,7 @@ describe("useResolvedPath", () => { useLocation(): /foo/bar useResolvedPath('.'): /foo/bar useResolvedPath('..'): /foo + useResolvedPath('..', { relative: 'path' }): /foo useResolvedPath('baz/qux'): /foo/bar/baz/qux useResolvedPath('./baz/qux'): /foo/bar/baz/qux From 576be9e7fde2d8a87df79ce367788342e8e699f9 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Tue, 5 Dec 2023 11:32:20 -0500 Subject: [PATCH 6/6] Add future flag to docs --- .changeset/relative-splat-path.md | 2 +- docs/components/form.md | 3 ++ docs/components/link.md | 3 ++ docs/guides/api-development-strategy.md | 14 +++--- docs/hooks/use-href.md | 10 ++--- docs/hooks/use-navigate.md | 3 ++ docs/hooks/use-resolved-path.md | 59 +++++++++++++++++++++++++ docs/hooks/use-submit.md | 3 ++ 8 files changed, 85 insertions(+), 12 deletions(-) diff --git a/.changeset/relative-splat-path.md b/.changeset/relative-splat-path.md index 5de9b68600..bcaa9c8d4a 100644 --- a/.changeset/relative-splat-path.md +++ b/.changeset/relative-splat-path.md @@ -42,7 +42,7 @@ function Dashboard() { } /> } /> } /> - + ); } diff --git a/docs/components/form.md b/docs/components/form.md index 65336f3b09..d5626d753a 100644 --- a/docs/components/form.md +++ b/docs/components/form.md @@ -109,6 +109,8 @@ If you need to post to a different route, then add an action prop: - [Index Search Param][indexsearchparam] (index vs parent route disambiguation) +Please see the [Splat Paths][relativesplatpath] section on the `useResolvedPath` docs for a note on the behavior of the `future.v7_relativeSplatPath` future flag for relative `useNavigate()` behavior within splat routes + ## `method` This determines the [HTTP verb](https://developer.mozilla.org/en-US/docs/Web/HTTP/Methods) to be used. The same as plain HTML [form method][htmlform-method], except it also supports "put", "patch", and "delete" in addition to "get" and "post". The default is "get". @@ -394,3 +396,4 @@ You can access those values from the `request.url` [history-state]: https://developer.mozilla.org/en-US/docs/Web/API/History/state [use-view-transition-state]: ../hooks//use-view-transition-state [view-transitions]: https://developer.mozilla.org/en-US/docs/Web/API/View_Transitions_API +[relativesplatpath]: ../hooks/use-resolved-path#splat-paths diff --git a/docs/components/link.md b/docs/components/link.md index b32e02271c..5b61e1c6b6 100644 --- a/docs/components/link.md +++ b/docs/components/link.md @@ -63,6 +63,8 @@ A relative `` value (that does not begin with `/`) resolves relative to `` with a `..` behaves differently from a normal `` when the current URL ends with `/`. `` ignores the trailing slash, and removes one URL segment for each `..`. But an `` value handles `..` differently when the current URL ends with `/` vs when it does not. +Please see the [Splat Paths][relativesplatpath] section on the `useResolvedPath` docs for a note on the behavior of the `future.v7_relativeSplatPath` future flag for relative `` behavior within splat routes + ## `relative` By default, links are relative to the route hierarchy (`relative="route"`), so `..` will go up one `Route` level. Occasionally, you may find that you have matching URL patterns that do not make sense to be nested, and you'd prefer to use relative _path_ routing. You can opt into this behavior with `relative="path"`: @@ -201,3 +203,4 @@ function ImageLink(to) { [view-transitions]: https://developer.mozilla.org/en-US/docs/Web/API/View_Transitions_API [picking-a-router]: ../routers/picking-a-router [navlink]: ./nav-link +[relativesplatpath]: ../hooks/use-resolved-path#splat-paths diff --git a/docs/guides/api-development-strategy.md b/docs/guides/api-development-strategy.md index d8c75d242c..1944448301 100644 --- a/docs/guides/api-development-strategy.md +++ b/docs/guides/api-development-strategy.md @@ -63,12 +63,13 @@ const router = createBrowserRouter(routes, { }); ``` -| Flag | Description | -| ----------------------------------------- | --------------------------------------------------------------------- | -| `v7_fetcherPersist` | Delay active fetcher cleanup until they return to an `idle` state | -| `v7_normalizeFormMethod` | Normalize `useNavigation().formMethod` to be an uppercase HTTP Method | -| [`v7_partialHydration`][partialhydration] | Support partial hydration for Server-rendered apps | -| `v7_prependBasename` | Prepend the router basename to navigate/fetch paths | +| Flag | Description | +| ------------------------------------------- | --------------------------------------------------------------------- | +| `v7_fetcherPersist` | Delay active fetcher cleanup until they return to an `idle` state | +| `v7_normalizeFormMethod` | Normalize `useNavigation().formMethod` to be an uppercase HTTP Method | +| [`v7_partialHydration`][partialhydration] | Support partial hydration for Server-rendered apps | +| `v7_prependBasename` | Prepend the router basename to navigate/fetch paths | +| [`v7_relativeSplatPath`][relativesplatpath] | Fix buggy relative path resolution in splat routes | ### React Router Future Flags @@ -96,3 +97,4 @@ These flags apply to both Data and non-Data Routers and are passed to the render [picking-a-router]: ../routers/picking-a-router [starttransition]: https://react.dev/reference/react/startTransition [partialhydration]: ../routers/create-browser-router#partial-hydration-data +[relativesplatpath]: ../hooks/use-resolved-path#splat-paths diff --git a/docs/hooks/use-href.md b/docs/hooks/use-href.md index 680add0450..842e2e889d 100644 --- a/docs/hooks/use-href.md +++ b/docs/hooks/use-href.md @@ -18,8 +18,8 @@ declare function useHref( The `useHref` hook returns a URL that may be used to link to the given `to` location, even outside of React Router. -> **Tip:** -> -> You may be interested in taking a look at the source for the `` -> component in `react-router-dom` to see how it uses `useHref` internally to -> determine its own `href` value. +You may be interested in taking a look at the source for the `` component in `react-router-dom` to see how it uses `useHref` internally to determine its own `href` value + +Please see the [Splat Paths][relativesplatpath] section on the `useResolvedPath` docs for a note on the behavior of the `future.v7_relativeSplatPath` future flag for relative `useHref()` behavior within splat routes + +[relativesplatpath]: ../hooks/use-resolved-path#splat-paths diff --git a/docs/hooks/use-navigate.md b/docs/hooks/use-navigate.md index d8e25cabd8..4493b90228 100644 --- a/docs/hooks/use-navigate.md +++ b/docs/hooks/use-navigate.md @@ -54,6 +54,8 @@ The `navigate` function has two signatures: - Either pass a `To` value (same type as ``) with an optional second `options` argument (similar to the props you can pass to [``][link]), or - Pass the delta you want to go in the history stack. For example, `navigate(-1)` is equivalent to hitting the back button +Please see the [Splat Paths][relativesplatpath] section on the `useResolvedPath` docs for a note on the behavior of the `future.v7_relativeSplatPath` future flag for relative `useNavigate()` behavior within splat routes + ## `options.replace` Specifying `replace: true` will cause the navigation to replace the current entry in the history stack instead of adding a new one. @@ -119,3 +121,4 @@ The `unstable_viewTransition` option enables a [View Transition][view-transition [picking-a-router]: ../routers/picking-a-router [flush-sync]: https://react.dev/reference/react-dom/flushSync [start-transition]: https://react.dev/reference/react/startTransition +[relativesplatpath]: ../hooks/use-resolved-path#splat-paths diff --git a/docs/hooks/use-resolved-path.md b/docs/hooks/use-resolved-path.md index fcf4138809..9f9cbeea86 100644 --- a/docs/hooks/use-resolved-path.md +++ b/docs/hooks/use-resolved-path.md @@ -22,5 +22,64 @@ This is useful when building links from relative values. For example, check out See [resolvePath][resolvepath] for more information. +## Splat Paths + +The original logic for `useResolvedPath` behaved differently for splat paths which in hindsight was incorrect/buggy behavior. This was fixed in [`6.19.0`][release-6.19.0] but it was determined that a large number of existing applications [relied on this behavior][revert-comment] so the fix was reverted in [`6.20.1`][release-6.20.1] and re-introduced in [`6.21.0`][release-6.21.0] behind a `future.v7_relativeSplatPath` [future flag][future-flag]. This will become the default behavior in React Router v7, so it is recommended to update your applications at your convenience to be better prepared for the eventual v7 upgrade. + +It should be noted that this is the foundation for all relative routing in React Router, so this applies to the following relative path code flows as well: + +- `` +- `useNavigate()` +- `useHref()` +- `
` +- `useSubmit()` +- Relative path `redirect` responses returned from loaders and actions + +### Behavior without the flag + +When this flag is not enabled, the default behavior is that when resolving relative paths inside of a [splat route (`*`)][splat], the splat portion of the path is ignored. So, given a route tree such as: + +```jsx + + + } /> + + +``` + +If you are currently at URL `/dashboard/teams`, `useResolvedPath("projects")` inside the `Dashboard` component would resolve to `/dashboard/projects` because the "current" location we are relative to would be considered `/dashboard` _without the "teams" splat value_. + +This makes for a slight convenience in routing between "sibling" splat routes (`/dashboard/teams`, `/dashboard/projects`, etc.), however it causes other inconsistencies such as: + +- `useResolvedPath(".")` no longer resolves to the current location for that route, it actually resolved you "up" to `/dashboard` from `/dashboard/teams` +- If you changed your route definition to use a dynamic parameter (``), then any resolved paths inside the `Dashboard` component would break since the dynamic param value is not ignored like the splat value + +And then it gets worse if you define the splat route as a child: + +```jsx + + + + } /> + + + +``` + +- Now, `useResolvedPath(".")` and `useResolvedPath("..")` resolve to the exact same path inside `` +- If you were using a Data Router and defined an `action` on the splat route, you'd get a 405 error on `` submissions because they (by default) submit to `"."` which would resolve to the parent `/dashboard` route which doesn't have an `action`. + +### Behavior with the flag + +When you enable the flag, this "bug" is fixed so that path resolution is consistent across all route types, `useResolvedPath(".")` always resolves to the current pathname for the contextual route. This includes any dynamic param or splat param values. + +If you want to navigate between "sibling" routes within a splat route, it is suggested you move your splat route to it's own child (`} />`) and use `useResolvedPath("../teams")` and `useResolvedPath("../projects")` parent-relative paths to navigate to sibling routes. + [navlink]: ../components/nav-link [resolvepath]: ../utils/resolve-path +[release-6.19.0]: https://github.com/remix-run/react-router/blob/main/CHANGELOG.md#v6190 +[release-6.20.1]: https://github.com/remix-run/react-router/blob/main/CHANGELOG.md#v6201 +[release-6.21.0]: https://github.com/remix-run/react-router/blob/main/CHANGELOG.md#v6210 +[revert-comment]: https://github.com/remix-run/react-router/issues/11052#issuecomment-1836589329 +[future-flag]: ../guides/api-development-strategy +[splat]: ../route/route#splats diff --git a/docs/hooks/use-submit.md b/docs/hooks/use-submit.md index 5208632166..83e0a14650 100644 --- a/docs/hooks/use-submit.md +++ b/docs/hooks/use-submit.md @@ -150,6 +150,8 @@ submit(null, { ; ``` +Please see the [Splat Paths][relativesplatpath] section on the `useResolvedPath` docs for a note on the behavior of the `future.v7_relativeSplatPath` future flag for relative `useSubmit()` `action` behavior within splat routes + Because submissions are navigations, the options may also contain the other navigation related props from [``][form] such as: - `fetcherKey` @@ -170,3 +172,4 @@ The `unstable_flushSync` option tells React Router DOM to wrap the initial state [form]: ../components/form [flush-sync]: https://react.dev/reference/react-dom/flushSync [start-transition]: https://react.dev/reference/react/startTransition +[relativesplatpath]: ../hooks/use-resolved-path#splat-paths