From dc5dddfc1e37e61167b1bcb53c6dcf64927401f7 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Thu, 1 Dec 2022 12:53:33 -0500 Subject: [PATCH 01/26] Allow uppercase
methods and fix submitted method override --- packages/react-router-dom/dom.ts | 2 +- packages/react-router-dom/index.tsx | 9 ++++++++- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/packages/react-router-dom/dom.ts b/packages/react-router-dom/dom.ts index dd5ad3e63c..e6fd5aa73f 100644 --- a/packages/react-router-dom/dom.ts +++ b/packages/react-router-dom/dom.ts @@ -245,5 +245,5 @@ export function getFormSubmissionInfo( let { protocol, host } = window.location; let url = new URL(action, `${protocol}//${host}`); - return { url, method, encType, formData }; + return { url, method: method.toLowerCase(), encType, formData }; } diff --git a/packages/react-router-dom/index.tsx b/packages/react-router-dom/index.tsx index 40bbb968f7..63e391422d 100644 --- a/packages/react-router-dom/index.tsx +++ b/packages/react-router-dom/index.tsx @@ -650,7 +650,14 @@ const FormImpl = React.forwardRef( let submitter = (event as unknown as HTMLSubmitEvent).nativeEvent .submitter as HTMLFormSubmitter | null; - submit(submitter || event.currentTarget, { method, replace, relative }); + let submitMethod = + (submitter?.formMethod as FormMethod | undefined) || method; + + submit(submitter || event.currentTarget, { + method: submitMethod, + replace, + relative, + }); }; return ( From e2f0cca37a2ebe9a4fb67541a386af42d0386f59 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Thu, 1 Dec 2022 13:46:27 -0500 Subject: [PATCH 02/26] Remove ?? operator and add lint rules --- packages/react-router-dom/.eslintrc | 3 ++- packages/react-router-dom/index.tsx | 3 +-- packages/react-router/.eslintrc | 3 ++- packages/router/.eslintrc | 6 +++++- 4 files changed, 10 insertions(+), 5 deletions(-) diff --git a/packages/react-router-dom/.eslintrc b/packages/react-router-dom/.eslintrc index a64f72e629..7258667c7e 100644 --- a/packages/react-router-dom/.eslintrc +++ b/packages/react-router-dom/.eslintrc @@ -7,6 +7,7 @@ "__DEV__": true }, "rules": { - "strict": 0 + "strict": 0, + "no-restricted-syntax": ["error", "LogicalExpression[operator='??']"] } } diff --git a/packages/react-router-dom/index.tsx b/packages/react-router-dom/index.tsx index 63e391422d..231645af77 100644 --- a/packages/react-router-dom/index.tsx +++ b/packages/react-router-dom/index.tsx @@ -923,10 +923,9 @@ export function useFormAction( invariant(routeContext, "useFormAction must be used inside a RouteContext"); let [match] = routeContext.matches.slice(-1); - let resolvedAction = action ?? "."; // Shallow clone path so we can modify it below, otherwise we modify the // object referenced by useMemo inside useResolvedPath - let path = { ...useResolvedPath(resolvedAction, { relative }) }; + let path = { ...useResolvedPath(action ? action : ".", { relative }) }; // Previously we set the default action to ".". The problem with this is that // `useResolvedPath(".")` excludes search params and the hash of the resolved diff --git a/packages/react-router/.eslintrc b/packages/react-router/.eslintrc index a64f72e629..7258667c7e 100644 --- a/packages/react-router/.eslintrc +++ b/packages/react-router/.eslintrc @@ -7,6 +7,7 @@ "__DEV__": true }, "rules": { - "strict": 0 + "strict": 0, + "no-restricted-syntax": ["error", "LogicalExpression[operator='??']"] } } diff --git a/packages/router/.eslintrc b/packages/router/.eslintrc index 5482700d18..7258667c7e 100644 --- a/packages/router/.eslintrc +++ b/packages/router/.eslintrc @@ -3,7 +3,11 @@ "browser": true, "commonjs": true }, + "globals": { + "__DEV__": true + }, "rules": { - "strict": 0 + "strict": 0, + "no-restricted-syntax": ["error", "LogicalExpression[operator='??']"] } } From 3fce26498c6088655515092cce9cce97fa971120 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Thu, 1 Dec 2022 15:33:15 -0500 Subject: [PATCH 03/26] Add Error serialization --- packages/react-router-dom/index.tsx | 4 ++++ packages/react-router-dom/server.tsx | 6 ++++++ 2 files changed, 10 insertions(+) diff --git a/packages/react-router-dom/index.tsx b/packages/react-router-dom/index.tsx index 231645af77..7de0c57cc3 100644 --- a/packages/react-router-dom/index.tsx +++ b/packages/react-router-dom/index.tsx @@ -252,6 +252,10 @@ function deserializeErrors( val.data, val.internal === true ); + } else if (val && val.__type === "Error") { + let error = new Error(val.message); + error.stack = val.stack; + serialized[key] = error; } else { serialized[key] = val; } diff --git a/packages/react-router-dom/server.tsx b/packages/react-router-dom/server.tsx index f0cfbff171..3aa3802591 100644 --- a/packages/react-router-dom/server.tsx +++ b/packages/react-router-dom/server.tsx @@ -157,6 +157,12 @@ function serializeErrors( // deserializeErrors in react-router-dom/index.tsx :) if (isRouteErrorResponse(val)) { serialized[key] = { ...val, __type: "RouteErrorResponse" }; + } else if (val instanceof Error) { + serialized[key] = { + message: val.message, + stack: val.stack, + __type: "Error", + }; } else { serialized[key] = val; } From cdbaa27fed6c4e11b9271fe4fae52c3a8e4643d6 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Thu, 1 Dec 2022 17:27:40 -0500 Subject: [PATCH 04/26] Mark loader-less routes with null loaderData during SSR --- packages/router/router.ts | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/packages/router/router.ts b/packages/router/router.ts index 40609b9c2d..7d2ba097d5 100644 --- a/packages/router/router.ts +++ b/packages/router/router.ts @@ -2252,7 +2252,10 @@ export function unstable_createStaticHandler( if (matchesToLoad.length === 0) { return { matches, - loaderData: {}, + loaderData: matches.reduce( + (acc, m) => Object.assign(acc, { [m.route.id]: null }), + {} + ), errors: pendingActionError || null, statusCode: 200, loaderHeaders: {}, @@ -2280,7 +2283,9 @@ export function unstable_createStaticHandler( // Can't do anything with these without the Remix side of things, so just // cancel them for now - results.forEach((result) => { + let executedLoaders = new Set(); + results.forEach((result, i) => { + executedLoaders.add(matchesToLoad[i].route.id); if (isDeferredResult(result)) { result.deferredData.cancel(); } @@ -2294,6 +2299,16 @@ export function unstable_createStaticHandler( pendingActionError ); + // Add a null for any routes that didn't have loaders so we know they've been + // previously "executed" and qualify as a revalidation on subsequent + // invocations. This is mostly needed for SSr scenarios where there may be + // no server loader but there may be a client loaders to fetch code split bundles + matches.forEach((match) => { + if (!executedLoaders.has(match.route.id)) { + context.loaderData[match.route.id] = null; + } + }); + return { ...context, matches, From e5d0803c60cae0f63b165d51a4e8c72e6010cba5 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Fri, 2 Dec 2022 18:13:33 -0500 Subject: [PATCH 05/26] Support fetch action redirects in useTransition --- packages/router/router.ts | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/packages/router/router.ts b/packages/router/router.ts index 7d2ba097d5..5b306030ec 100644 --- a/packages/router/router.ts +++ b/packages/router/router.ts @@ -1327,7 +1327,7 @@ export function createRouter(init: RouterInit): Router { state.fetchers.set(key, loadingFetcher); updateState({ fetchers: new Map(state.fetchers) }); - return startRedirectNavigation(state, actionResult); + return startRedirectNavigation(state, actionResult, false, true); } // Process any non-redirect errors thrown @@ -1587,13 +1587,23 @@ export function createRouter(init: RouterInit): Router { async function startRedirectNavigation( state: RouterState, redirect: RedirectResult, - replace?: boolean + replace?: boolean, + isFetchActionRedirect?: boolean ) { if (redirect.revalidate) { isRevalidationRequired = true; } - let redirectLocation = createLocation(state.location, redirect.location); + let redirectLocation = createLocation( + state.location, + redirect.location, + // TODO: This can be removed once we get rid of useTransition in Remix v2 + isFetchActionRedirect + ? { + isFetchActionRedirect: true, + } + : undefined + ); invariant( redirectLocation, "Expected a location on the redirect navigation" From 34ee73302f5690fbddaf9aef504f7386cf2e54df Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Thu, 8 Dec 2022 12:26:54 -0500 Subject: [PATCH 06/26] Add _isRedirect state for useTransition back compat --- packages/router/router.ts | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/packages/router/router.ts b/packages/router/router.ts index 913e8d5dbe..677de52e3f 100644 --- a/packages/router/router.ts +++ b/packages/router/router.ts @@ -1624,11 +1624,10 @@ export function createRouter(init: RouterInit): Router { state.location, redirect.location, // TODO: This can be removed once we get rid of useTransition in Remix v2 - isFetchActionRedirect - ? { - isFetchActionRedirect: true, - } - : undefined + { + _isRedirect: true, + ...(isFetchActionRedirect ? { _isFetchActionRedirect: true } : {}), + } ); invariant( redirectLocation, From f188645373ce3400bf982f4be3593758162e95e8 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Thu, 8 Dec 2022 17:58:53 -0500 Subject: [PATCH 07/26] Updates for fetcher type and redirect replace logic --- packages/router/__tests__/router-test.ts | 108 +++++++++++++++++++++-- packages/router/router.ts | 38 ++++++-- 2 files changed, 132 insertions(+), 14 deletions(-) diff --git a/packages/router/__tests__/router-test.ts b/packages/router/__tests__/router-test.ts index f8939b6378..d64fff465b 100644 --- a/packages/router/__tests__/router-test.ts +++ b/packages/router/__tests__/router-test.ts @@ -2469,6 +2469,35 @@ describe("a router", () => { expect(t.router.state.location.pathname).toEqual("/foo"); }); + it("navigates correctly using POP navigations across actions to new locations", async () => { + let t = initializeTmTest(); + + // Navigate to /foo + let A = await t.navigate("/foo"); + await A.loaders.foo.resolve("FOO"); + expect(t.router.state.location.pathname).toEqual("/foo"); + + // Navigate to /bar + let B = await t.navigate("/bar"); + await B.loaders.bar.resolve("BAR"); + expect(t.router.state.location.pathname).toEqual("/bar"); + + // Post to /baz (should not replace) + let C = await t.navigate("/baz", { + formMethod: "post", + formData: createFormData({ key: "value" }), + }); + await C.actions.baz.resolve("BAZ ACTION"); + await C.loaders.root.resolve("ROOT"); + await C.loaders.baz.resolve("BAZ"); + expect(t.router.state.location.pathname).toEqual("/baz"); + + // POP to /bar + let D = await t.navigate(-1); + await D.loaders.bar.resolve("BAR"); + expect(t.router.state.location.pathname).toEqual("/bar"); + }); + it("navigates correctly using POP navigations across action errors", async () => { let t = initializeTmTest(); @@ -2617,6 +2646,67 @@ describe("a router", () => { expect(t.router.state.historyAction).toEqual("POP"); expect(t.router.state.location.pathname).toEqual("/foo"); }); + + it("should respect explicit replace:false on non-redirected actions to new locations", async () => { + // start at / (history stack: [/]) + let t = initializeTmTest(); + + // Link to /foo (history stack: [/, /foo]) + let A = await t.navigate("/foo"); + await A.loaders.root.resolve("ROOT"); + await A.loaders.foo.resolve("FOO"); + expect(t.router.state.historyAction).toEqual("PUSH"); + expect(t.router.state.location.pathname).toEqual("/foo"); + + // POST /bar (history stack: [/, /foo, /bar]) + let B = await t.navigate("/bar", { + formMethod: "post", + formData: createFormData({ gosh: "dang" }), + replace: false, + }); + await B.actions.bar.resolve("BAR"); + await B.loaders.root.resolve("ROOT"); + await B.loaders.bar.resolve("BAR"); + expect(t.router.state.historyAction).toEqual("PUSH"); + expect(t.router.state.location.pathname).toEqual("/bar"); + + // POP /foo (history stack: [GET /, GET /foo]) + let C = await t.navigate(-1); + await C.loaders.foo.resolve("FOO"); + expect(t.router.state.historyAction).toEqual("POP"); + expect(t.router.state.location.pathname).toEqual("/foo"); + }); + + it("should respect explicit replace:false on non-redirected actions to the same location", async () => { + // start at / (history stack: [/]) + let t = initializeTmTest(); + + // Link to /foo (history stack: [/, /foo]) + let A = await t.navigate("/foo"); + await A.loaders.root.resolve("ROOT"); + await A.loaders.foo.resolve("FOO"); + expect(t.router.state.historyAction).toEqual("PUSH"); + expect(t.router.state.location.pathname).toEqual("/foo"); + + // POST /foo (history stack: [/, /foo, /foo]) + let B = await t.navigate("/foo", { + formMethod: "post", + formData: createFormData({ gosh: "dang" }), + replace: false, + }); + await B.actions.foo.resolve("FOO2 ACTION"); + await B.loaders.root.resolve("ROOT2"); + await B.loaders.foo.resolve("FOO2"); + expect(t.router.state.historyAction).toEqual("PUSH"); + expect(t.router.state.location.pathname).toEqual("/foo"); + + // POP /foo (history stack: [/, /foo]) + let C = await t.navigate(-1); + await C.loaders.root.resolve("ROOT3"); + await C.loaders.foo.resolve("FOO3"); + expect(t.router.state.historyAction).toEqual("POP"); + expect(t.router.state.location.pathname).toEqual("/foo"); + }); }); describe("submission navigations", () => { @@ -6280,7 +6370,7 @@ describe("a router", () => { await N.loaders.root.resolve("ROOT_DATA*"); await N.loaders.tasks.resolve("TASKS_DATA"); expect(t.router.state).toMatchObject({ - historyAction: "REPLACE", + historyAction: "PUSH", location: { pathname: "/tasks" }, navigation: IDLE_NAVIGATION, revalidation: "idle", @@ -6292,7 +6382,7 @@ describe("a router", () => { tasks: "TASKS_ACTION", }, }); - expect(t.history.replace).toHaveBeenCalledWith( + expect(t.history.push).toHaveBeenCalledWith( t.router.state.location, t.router.state.location.state ); @@ -6492,7 +6582,7 @@ describe("a router", () => { await R.loaders.root.resolve("ROOT_DATA*"); await R.loaders.tasks.resolve("TASKS_DATA*"); expect(t.router.state).toMatchObject({ - historyAction: "REPLACE", + historyAction: "PUSH", location: { pathname: "/tasks" }, navigation: IDLE_NAVIGATION, revalidation: "idle", @@ -6501,7 +6591,7 @@ describe("a router", () => { tasks: "TASKS_DATA*", }, }); - expect(t.history.replace).toHaveBeenCalledWith( + expect(t.history.push).toHaveBeenCalledWith( t.router.state.location, t.router.state.location.state ); @@ -6579,7 +6669,7 @@ describe("a router", () => { await R.loaders.root.resolve("ROOT_DATA*"); await R.loaders.tasks.resolve("TASKS_DATA*"); expect(t.router.state).toMatchObject({ - historyAction: "REPLACE", + historyAction: "PUSH", location: { pathname: "/tasks" }, navigation: IDLE_NAVIGATION, revalidation: "idle", @@ -6591,7 +6681,7 @@ describe("a router", () => { tasks: "TASKS_ACTION", }, }); - expect(t.history.replace).toHaveBeenCalledWith( + expect(t.history.push).toHaveBeenCalledWith( t.router.state.location, t.router.state.location.state ); @@ -6894,7 +6984,7 @@ describe("a router", () => { let key = "key"; router.fetch(key, "root", "/"); - expect(router.state.fetchers.get(key)).toEqual({ + expect(router.state.fetchers.get(key)).toMatchObject({ state: "loading", formMethod: undefined, formEncType: undefined, @@ -6903,7 +6993,7 @@ describe("a router", () => { }); await dfd.resolve("DATA"); - expect(router.state.fetchers.get(key)).toEqual({ + expect(router.state.fetchers.get(key)).toMatchObject({ state: "idle", formMethod: undefined, formEncType: undefined, @@ -7394,7 +7484,7 @@ describe("a router", () => { expect(t.router.state.navigation.location?.pathname).toBe("/bar"); await AR.loaders.root.resolve("ROOT*"); await AR.loaders.bar.resolve("stuff"); - expect(A.fetcher).toEqual({ + expect(A.fetcher).toMatchObject({ data: undefined, state: "idle", formMethod: undefined, diff --git a/packages/router/router.ts b/packages/router/router.ts index 677de52e3f..f47a3109d6 100644 --- a/packages/router/router.ts +++ b/packages/router/router.ts @@ -435,6 +435,7 @@ type FetcherStates = { formEncType: undefined; formData: undefined; data: TData | undefined; + " _hasFetcherDoneAnything "?: boolean; }; Loading: { state: "loading"; @@ -443,6 +444,7 @@ type FetcherStates = { formEncType: FormEncType | undefined; formData: FormData | undefined; data: TData | undefined; + " _hasFetcherDoneAnything "?: boolean; }; Submitting: { state: "submitting"; @@ -451,6 +453,7 @@ type FetcherStates = { formEncType: FormEncType; formData: FormData; data: TData | undefined; + " _hasFetcherDoneAnything "?: boolean; }; }; @@ -815,11 +818,26 @@ export function createRouter(init: RouterInit): Router { ...init.history.encodeLocation(location), }; - let historyAction = - (opts && opts.replace) === true || - (submission != null && isMutationMethod(submission.formMethod)) - ? HistoryAction.Replace - : HistoryAction.Push; + let historyAction = HistoryAction.Push; + + if (opts && opts?.replace === true) { + historyAction = HistoryAction.Replace; + } else if (opts && opts.replace === false) { + // no-op + } else if ( + submission != null && + isMutationMethod(submission.formMethod) && + parsePath(submission.formAction).pathname === state.location.pathname + ) { + // TODO check this with Ryan + + // By default on submissions to the current location we REPLACE so that + // users don't have to double-click the back button to get to the prior + // location. If the user redirects from the action/loader this will be + // ignored and the redirect will be a PUSH + historyAction = HistoryAction.Replace; + } + let preventScrollReset = opts && "preventScrollReset" in opts ? opts.preventScrollReset === true @@ -1158,6 +1176,7 @@ export function createRouter(init: RouterInit): Router { formAction: undefined, formEncType: undefined, formData: undefined, + " _hasFetcherDoneAnything ": true, }; state.fetchers.set(key, revalidatingFetcher); }); @@ -1310,6 +1329,7 @@ export function createRouter(init: RouterInit): Router { state: "submitting", ...submission, data: existingFetcher && existingFetcher.data, + " _hasFetcherDoneAnything ": true, }; state.fetchers.set(key, fetcher); updateState({ fetchers: new Map(state.fetchers) }); @@ -1347,6 +1367,7 @@ export function createRouter(init: RouterInit): Router { state: "loading", ...submission, data: undefined, + " _hasFetcherDoneAnything ": true, }; state.fetchers.set(key, loadingFetcher); updateState({ fetchers: new Map(state.fetchers) }); @@ -1385,6 +1406,7 @@ export function createRouter(init: RouterInit): Router { state: "loading", data: actionResult.data, ...submission, + " _hasFetcherDoneAnything ": true, }; state.fetchers.set(key, loadFetcher); @@ -1415,6 +1437,7 @@ export function createRouter(init: RouterInit): Router { formAction: undefined, formEncType: undefined, formData: undefined, + " _hasFetcherDoneAnything ": true, }; state.fetchers.set(staleKey, revalidatingFetcher); fetchControllers.set(staleKey, abortController); @@ -1465,6 +1488,7 @@ export function createRouter(init: RouterInit): Router { formAction: undefined, formEncType: undefined, formData: undefined, + " _hasFetcherDoneAnything ": true, }; state.fetchers.set(key, doneFetcher); @@ -1518,6 +1542,7 @@ export function createRouter(init: RouterInit): Router { formData: undefined, ...submission, data: existingFetcher && existingFetcher.data, + " _hasFetcherDoneAnything ": true, }; state.fetchers.set(key, loadingFetcher); updateState({ fetchers: new Map(state.fetchers) }); @@ -1586,6 +1611,7 @@ export function createRouter(init: RouterInit): Router { formAction: undefined, formEncType: undefined, formData: undefined, + " _hasFetcherDoneAnything ": true, }; state.fetchers.set(key, doneFetcher); updateState({ fetchers: new Map(state.fetchers) }); @@ -1791,6 +1817,7 @@ export function createRouter(init: RouterInit): Router { formAction: undefined, formEncType: undefined, formData: undefined, + " _hasFetcherDoneAnything ": true, }; state.fetchers.set(key, doneFetcher); } @@ -2977,6 +3004,7 @@ function processLoaderData( formAction: undefined, formEncType: undefined, formData: undefined, + " _hasFetcherDoneAnything ": true, }; state.fetchers.set(key, doneFetcher); } From e8d5340d6853d18ba82aedb9dcf99451cb50e7b4 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Fri, 9 Dec 2022 11:39:21 -0500 Subject: [PATCH 08/26] Add tests for SSR null loader values on non-executed loaders --- packages/router/__tests__/router-test.ts | 51 ++++++++++++++++++++++++ packages/router/router.ts | 10 ++--- 2 files changed, 55 insertions(+), 6 deletions(-) diff --git a/packages/router/__tests__/router-test.ts b/packages/router/__tests__/router-test.ts index d64fff465b..08711243c7 100644 --- a/packages/router/__tests__/router-test.ts +++ b/packages/router/__tests__/router-test.ts @@ -10560,6 +10560,57 @@ describe("a router", () => { }); }); + it("should fill in null loaderData values for routes without loaders", async () => { + let { query } = createStaticHandler([ + { + id: "root", + path: "/", + children: [ + { + id: "none", + path: "none", + }, + { + id: "a", + path: "a", + loader: () => "A", + children: [ + { + id: "b", + path: "b", + }, + ], + }, + ], + }, + ]); + + // No loaders at all + let context = await query(createRequest("/none")); + expect(context).toMatchObject({ + actionData: null, + loaderData: { + root: null, + none: null, + }, + errors: null, + location: { pathname: "/none" }, + }); + + // Mix of loaders and no loaders + context = await query(createRequest("/a/b")); + expect(context).toMatchObject({ + actionData: null, + loaderData: { + root: null, + a: "A", + b: null, + }, + errors: null, + location: { pathname: "/a/b" }, + }); + }); + it("should support document load navigations returning responses", async () => { let { query } = createStaticHandler(SSR_ROUTES); let context = await query(createRequest("/parent/json")); diff --git a/packages/router/router.ts b/packages/router/router.ts index f47a3109d6..181b6a8152 100644 --- a/packages/router/router.ts +++ b/packages/router/router.ts @@ -2345,6 +2345,7 @@ export function unstable_createStaticHandler( if (matchesToLoad.length === 0) { return { matches, + // Add a null for all matched routes for proper revalidation on the client loaderData: matches.reduce( (acc, m) => Object.assign(acc, { [m.route.id]: null }), {} @@ -2375,11 +2376,11 @@ export function unstable_createStaticHandler( throw new Error(`${method}() call aborted`); } - // Can't do anything with these without the Remix side of things, so just - // cancel them for now let executedLoaders = new Set(); results.forEach((result, i) => { executedLoaders.add(matchesToLoad[i].route.id); + // Can't do anything with these without the Remix side of things, so just + // cancel them for now if (isDeferredResult(result)) { result.deferredData.cancel(); } @@ -2393,10 +2394,7 @@ export function unstable_createStaticHandler( pendingActionError ); - // Add a null for any routes that didn't have loaders so we know they've been - // previously "executed" and qualify as a revalidation on subsequent - // invocations. This is mostly needed for SSr scenarios where there may be - // no server loader but there may be a client loaders to fetch code split bundles + // Add a null for any non-loader matches for proper revalidation on the client matches.forEach((match) => { if (!executedLoaders.has(match.route.id)) { context.loaderData[match.route.id] = null; From 99993d96f27fb86d56c6c651811dbef529dd6d3a Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Fri, 9 Dec 2022 12:56:28 -0500 Subject: [PATCH 09/26] more tests for submission replace/push logic --- .../__tests__/data-browser-router-test.tsx | 96 ++++++++++++++++++- 1 file changed, 95 insertions(+), 1 deletion(-) 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 6375b79954..74fb30c8bc 100644 --- a/packages/react-router-dom/__tests__/data-browser-router-test.tsx +++ b/packages/react-router-dom/__tests__/data-browser-router-test.tsx @@ -1266,7 +1266,7 @@ function testDomRouter( `); }); - it('defaults useSubmit({ method: "post" }) to be a REPLACE navigation', async () => { + it('defaults useSubmit({ method: "post" }) to a new location to be a PUSH navigation', async () => { let { container } = render( }> @@ -1342,6 +1342,100 @@ function testDomRouter( " `); + fireEvent.click(screen.getByText("Go back")); + await waitFor(() => screen.getByText("Page 1")); + expect(getHtml(container.querySelector(".output"))) + .toMatchInlineSnapshot(` + "
+

+ Page 1 +

+
" + `); + }); + + it('defaults useSubmit({ method: "post" }) to the same location to be a REPLACE navigation', async () => { + let { container } = render( + + }> + "index"} element={

index

} /> + "action"} + loader={() => "1"} + element={

Page 1

} + /> + + + ); + + function Layout() { + let navigate = useNavigate(); + let submit = useSubmit(); + let actionData = useActionData(); + let formData = new FormData(); + formData.append("test", "value"); + return ( + <> + Go to 1 + + +
+ {actionData ?

{actionData}

: null} + +
+ + ); + } + + expect(getHtml(container.querySelector(".output"))) + .toMatchInlineSnapshot(` + "
+

+ index +

+
" + `); + + fireEvent.click(screen.getByText("Go to 1")); + await waitFor(() => screen.getByText("Page 1")); + expect(getHtml(container.querySelector(".output"))) + .toMatchInlineSnapshot(` + "
+

+ Page 1 +

+
" + `); + + fireEvent.click(screen.getByText("Submit")); + await waitFor(() => screen.getByText("action")); + expect(getHtml(container.querySelector(".output"))) + .toMatchInlineSnapshot(` + "
+

+ action +

+

+ Page 1 +

+
" + `); + fireEvent.click(screen.getByText("Go back")); await waitFor(() => screen.getByText("index")); expect(getHtml(container.querySelector(".output"))) From e3b31d0e97de8ef88fdf206d7a7cd0bfe825122b Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Fri, 9 Dec 2022 12:56:45 -0500 Subject: [PATCH 10/26] fix test for null loader data during ssr --- .../react-router-dom/__tests__/data-static-router-test.tsx | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/packages/react-router-dom/__tests__/data-static-router-test.tsx b/packages/react-router-dom/__tests__/data-static-router-test.tsx index a1c16550eb..be1381abd4 100644 --- a/packages/react-router-dom/__tests__/data-static-router-test.tsx +++ b/packages/react-router-dom/__tests__/data-static-router-test.tsx @@ -355,7 +355,10 @@ describe("A ", () => { let expectedJsonString = JSON.stringify( JSON.stringify({ - loaderData: {}, + loaderData: { + 0: null, + "0-0": null, + }, actionData: null, errors: null, }) From 470082b6461d48b85da2b133133101f1f43a1580 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Fri, 9 Dec 2022 13:05:16 -0500 Subject: [PATCH 11/26] SSr error serialization tests --- .../__tests__/data-browser-router-test.tsx | 53 +++++++++++++++++-- .../__tests__/data-static-router-test.tsx | 44 +++++++++++++++ packages/react-router-dom/index.tsx | 4 +- packages/react-router-dom/server.tsx | 2 +- 4 files changed, 97 insertions(+), 6 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 74fb30c8bc..e0e4f94618 100644 --- a/packages/react-router-dom/__tests__/data-browser-router-test.tsx +++ b/packages/react-router-dom/__tests__/data-browser-router-test.tsx @@ -287,14 +287,59 @@ function testDomRouter( function Boundary() { let error = useRouteError(); - return isRouteErrorResponse(error) ?

Yes!

:

No :(

; + return isRouteErrorResponse(error) ? ( + 
{JSON.stringify(error)}
+ ) : ( +

No :(

+ ); } expect(getHtml(container)).toMatchInlineSnapshot(` "
-

- Yes! -

+ 
+            {\\"status\\":404,\\"statusText\\":\\"Not Found\\",\\"internal\\":false,\\"data\\":{\\"not\\":\\"found\\"}}
+
+
" + `); + }); + + it("deserializes Error instances from the window", async () => { + window.__staticRouterHydrationData = { + loaderData: {}, + actionData: null, + errors: { + "0": { + message: "error message", + __type: "Error", + }, + }, + }; + let { container } = render( + + Nope} errorElement={} /> + + ); + + function Boundary() { + let error = useRouteError(); + return error instanceof Error ? ( + <> + 
{error.toString()}
+ 
stack:{error.stack}
+ + ) : ( +

No :(

+ ); + } + + expect(getHtml(container)).toMatchInlineSnapshot(` + "
+ 
+            Error: error message
+
+ 
+            stack:
+
" `); }); diff --git a/packages/react-router-dom/__tests__/data-static-router-test.tsx b/packages/react-router-dom/__tests__/data-static-router-test.tsx index be1381abd4..40a674ac86 100644 --- a/packages/react-router-dom/__tests__/data-static-router-test.tsx +++ b/packages/react-router-dom/__tests__/data-static-router-test.tsx @@ -321,6 +321,50 @@ describe("A ", () => { ); }); + it("serializes Error instances", async () => { + let routes = [ + { + path: "/", + loader: () => { + throw new Error("oh no"); + }, + }, + ]; + let { query } = createStaticHandler(routes); + + let context = (await query( + new Request("http://localhost/", { + signal: new AbortController().signal, + }) + )) as StaticHandlerContext; + + let html = ReactDOMServer.renderToStaticMarkup( + + + + ); + + // stack is stripped by default from SSR errors + let expectedJsonString = JSON.stringify( + JSON.stringify({ + loaderData: {}, + actionData: null, + errors: { + "0": { + message: "oh no", + __type: "Error", + }, + }, + }) + ); + expect(html).toMatch( + `` + ); + }); + it("supports a nonce prop", async () => { let routes = [ { diff --git a/packages/react-router-dom/index.tsx b/packages/react-router-dom/index.tsx index 82d1cdb425..374b1247de 100644 --- a/packages/react-router-dom/index.tsx +++ b/packages/react-router-dom/index.tsx @@ -254,7 +254,9 @@ function deserializeErrors( ); } else if (val && val.__type === "Error") { let error = new Error(val.message); - error.stack = val.stack; + // Wipe away the client-side stack trace. Nothing to fill it in with + // because we don't serialize SSR stack traces for security reasons + error.stack = ""; serialized[key] = error; } else { serialized[key] = val; diff --git a/packages/react-router-dom/server.tsx b/packages/react-router-dom/server.tsx index 3aa3802591..28bde589b3 100644 --- a/packages/react-router-dom/server.tsx +++ b/packages/react-router-dom/server.tsx @@ -158,9 +158,9 @@ function serializeErrors( if (isRouteErrorResponse(val)) { serialized[key] = { ...val, __type: "RouteErrorResponse" }; } else if (val instanceof Error) { + // Do not serialize stack traces from SSR for security reasons serialized[key] = { message: val.message, - stack: val.stack, __type: "Error", }; } else { From 239bc7c80a1f19b5c96045a0f6551f858109ce60 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Fri, 9 Dec 2022 13:22:19 -0500 Subject: [PATCH 12/26] form tests --- .../__tests__/data-browser-router-test.tsx | 151 ++++++++++++++++++ packages/react-router-dom/index.tsx | 3 +- 2 files changed, 153 insertions(+), 1 deletion(-) 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 e0e4f94618..b1c8ebace3 100644 --- a/packages/react-router-dom/__tests__/data-browser-router-test.tsx +++ b/packages/react-router-dom/__tests__/data-browser-router-test.tsx @@ -1662,6 +1662,157 @@ function testDomRouter( `); }); + it("allows a button to override the ", async () => { + let loaderDefer = createDeferred(); + + let { container } = render( + + { + throw new Error("Should not hit this"); + }} + loader={() => loaderDefer.promise} + element={} + /> + + ); + + function Home() { + let data = useLoaderData(); + let navigation = useNavigation(); + return ( +
+ { + // jsdom doesn't handle submitter so we add it here + // See https://github.com/jsdom/jsdom/issues/3117 + // @ts-expect-error + e.nativeEvent.submitter = + e.currentTarget.querySelector("button"); + }} + > + + + +
+

{navigation.state}

+

{data}

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

+ idle +

+

+

" + `); + + fireEvent.click(screen.getByText("Submit Form")); + await waitFor(() => screen.getByText("loading")); + expect(getHtml(container.querySelector("#output"))) + .toMatchInlineSnapshot(` + "
+

+ loading +

+

+

" + `); + + loaderDefer.resolve("Loader Data"); + await waitFor(() => screen.getByText("idle")); + expect(getHtml(container.querySelector("#output"))) + .toMatchInlineSnapshot(` + "
+

+ idle +

+

+ Loader Data +

+
" + `); + }); + + it("supports uppercase form method attributes", async () => { + let loaderDefer = createDeferred(); + let actionDefer = createDeferred(); + + let { container } = render( + + { + let resolvedValue = await actionDefer.promise; + let formData = await request.formData(); + return `${resolvedValue}:${formData.get("test")}`; + }} + loader={() => loaderDefer.promise} + element={} + /> + + ); + + function Home() { + let data = useLoaderData(); + let actionData = useActionData(); + let navigation = useNavigation(); + return ( +
+
+ + +
+
+

{navigation.state}

+

{data}

+

{actionData}

+
+ +
+ ); + } + + fireEvent.click(screen.getByText("Submit Form")); + await waitFor(() => screen.getByText("submitting")); + actionDefer.resolve("Action Data"); + await waitFor(() => screen.getByText("loading")); + loaderDefer.resolve("Loader Data"); + await waitFor(() => screen.getByText("idle")); + expect(getHtml(container.querySelector("#output"))) + .toMatchInlineSnapshot(` + "
+

+ idle +

+

+ Loader Data +

+

+ Action Data:value +

+
" + `); + }); + describe("
", () => { function NoActionComponent() { return ( diff --git a/packages/react-router-dom/index.tsx b/packages/react-router-dom/index.tsx index 374b1247de..11381e770b 100644 --- a/packages/react-router-dom/index.tsx +++ b/packages/react-router-dom/index.tsx @@ -657,7 +657,8 @@ const FormImpl = React.forwardRef( .submitter as HTMLFormSubmitter | null; let submitMethod = - (submitter?.formMethod as FormMethod | undefined) || method; + (submitter?.getAttribute("formmethod") as FormMethod | undefined) || + method; submit(submitter || event.currentTarget, { method: submitMethod, From ecbca6445308f8949dae9aae43441c45089590ca Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Mon, 12 Dec 2022 13:36:31 -0500 Subject: [PATCH 13/26] bundle bump --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index c1cc5f84d3..5205d657ff 100644 --- a/package.json +++ b/package.json @@ -107,7 +107,7 @@ }, "filesize": { "packages/router/dist/router.umd.min.js": { - "none": "36.5 kB" + "none": "37 kB" }, "packages/react-router/dist/react-router.production.min.js": { "none": "12.5 kB" From 09209b67499b9dccbb92395638451579bfd05edc Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Mon, 12 Dec 2022 18:05:11 -0500 Subject: [PATCH 14/26] Updates to ScrollRestoration for Remix --- packages/react-router-dom/index.tsx | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/packages/react-router-dom/index.tsx b/packages/react-router-dom/index.tsx index 11381e770b..2aafcd7421 100644 --- a/packages/react-router-dom/index.tsx +++ b/packages/react-router-dom/index.tsx @@ -64,6 +64,7 @@ import { export type { FormEncType, FormMethod, + GetScrollRestorationKeyFunction, ParamKeyValuePair, SubmitOptions, URLSearchParamsInit, @@ -683,7 +684,7 @@ if (__DEV__) { FormImpl.displayName = "FormImpl"; } -interface ScrollRestorationProps { +export interface ScrollRestorationProps { getKey?: GetScrollRestorationKeyFunction; storageKey?: string; } @@ -1080,9 +1081,11 @@ let savedScrollPositions: Record = {}; function useScrollRestoration({ getKey, storageKey, + skip = false, }: { getKey?: GetScrollRestorationKeyFunction; storageKey?: string; + skip?: boolean; } = {}) { let { router } = useDataRouterContext(DataRouterHook.UseScrollRestoration); let { restoreScrollPosition, preventScrollReset } = useDataRouterState( @@ -1141,6 +1144,11 @@ function useScrollRestoration({ // Restore scrolling when state.restoreScrollPosition changes React.useLayoutEffect(() => { + // Skip if the consumer asked us to (used for SSR) + if (skip) { + return; + } + // Explicit false means don't do anything (used for submissions) if (restoreScrollPosition === false) { return; @@ -1168,7 +1176,7 @@ function useScrollRestoration({ // otherwise go to the top on new locations window.scrollTo(0, 0); - }, [location, restoreScrollPosition, preventScrollReset]); + }, [location, restoreScrollPosition, preventScrollReset, skip]); } function useBeforeUnload(callback: () => any): void { @@ -1203,3 +1211,5 @@ function warning(cond: boolean, message: string): void { } } //#endregion + +export { useScrollRestoration as UNSAFE_useScrollRestoration }; From a5e45d70b8cd27e29869b684f350b83501d7d156 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Mon, 12 Dec 2022 18:13:37 -0500 Subject: [PATCH 15/26] Export useBeforeUnload --- docs/hooks/use-before-unload.md | 32 +++++++++++++++++++++++++++++ packages/react-router-dom/index.tsx | 13 ++++++++++-- 2 files changed, 43 insertions(+), 2 deletions(-) create mode 100644 docs/hooks/use-before-unload.md diff --git a/docs/hooks/use-before-unload.md b/docs/hooks/use-before-unload.md new file mode 100644 index 0000000000..613284afbe --- /dev/null +++ b/docs/hooks/use-before-unload.md @@ -0,0 +1,32 @@ +--- +title: useBeforeUnload +new: true +--- + +# `useBeforeUnload` + +This hook is just a helper around `window.onbeforeunload`. It can be useful to save important application state on the page (to something like the browser's local storage), before the user navigates away from your page. That way if they come back you can restore any stateful information (restore form input values, etc.) + +```tsx lines=[1,7-11] +import { useBeforeUnload } from "react-router-dom"; + +function SomeForm() { + const [state, setState] = React.useState(null); + + // save it off before users navigate away + useBeforeUnload( + React.useCallback(() => { + localStorage.stuff = state; + }, [state]) + ); + + // read it in when they return + React.useEffect(() => { + if (state === null && localStorage.stuff != null) { + setState(localStorage.stuff); + } + }, [state]); + + return <>{/*... */}; +} +``` diff --git a/packages/react-router-dom/index.tsx b/packages/react-router-dom/index.tsx index 2aafcd7421..8cfd873b7f 100644 --- a/packages/react-router-dom/index.tsx +++ b/packages/react-router-dom/index.tsx @@ -1179,7 +1179,17 @@ function useScrollRestoration({ }, [location, restoreScrollPosition, preventScrollReset, skip]); } -function useBeforeUnload(callback: () => any): void { +/** + * Setup a callback to be fired on the window's `beforeunload` event. This is + * useful for saving some data to `window.localStorage` just before the page + * refreshes. + * + * Note: The `callback` argument should be a function created with + * `React.useCallback()`. + */ +export function useBeforeUnload( + callback: (event: BeforeUnloadEvent) => any +): void { React.useEffect(() => { window.addEventListener("beforeunload", callback); return () => { @@ -1187,7 +1197,6 @@ function useBeforeUnload(callback: () => any): void { }; }, [callback]); } - //#endregion //////////////////////////////////////////////////////////////////////////////// From 6c69af549ff85845442cdb82e90586096b71a663 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Mon, 12 Dec 2022 18:27:21 -0500 Subject: [PATCH 16/26] Avoid SSR layout effects --- packages/react-router-dom/index.tsx | 105 +++++++++++++++------------- 1 file changed, 55 insertions(+), 50 deletions(-) diff --git a/packages/react-router-dom/index.tsx b/packages/react-router-dom/index.tsx index 8cfd873b7f..f220621f16 100644 --- a/packages/react-router-dom/index.tsx +++ b/packages/react-router-dom/index.tsx @@ -1119,64 +1119,69 @@ function useScrollRestoration({ ); // Read in any saved scroll locations - React.useLayoutEffect(() => { - try { - let sessionPositions = sessionStorage.getItem( - storageKey || SCROLL_RESTORATION_STORAGE_KEY + if (typeof document !== "undefined") { + // eslint-disable-next-line react-hooks/rules-of-hooks + React.useLayoutEffect(() => { + try { + let sessionPositions = sessionStorage.getItem( + storageKey || SCROLL_RESTORATION_STORAGE_KEY + ); + if (sessionPositions) { + savedScrollPositions = JSON.parse(sessionPositions); + } + } catch (e) { + // no-op, use default empty object + } + }, [storageKey]); + + // Enable scroll restoration in the router + // eslint-disable-next-line react-hooks/rules-of-hooks + React.useLayoutEffect(() => { + let disableScrollRestoration = router?.enableScrollRestoration( + savedScrollPositions, + () => window.scrollY, + getKey ); - if (sessionPositions) { - savedScrollPositions = JSON.parse(sessionPositions); + return () => disableScrollRestoration && disableScrollRestoration(); + }, [router, getKey]); + + // Restore scrolling when state.restoreScrollPosition changes + // eslint-disable-next-line react-hooks/rules-of-hooks + React.useLayoutEffect(() => { + // Skip if the consumer asked us to (used for SSR) + if (skip) { + return; } - } catch (e) { - // no-op, use default empty object - } - }, [storageKey]); - - // Enable scroll restoration in the router - React.useLayoutEffect(() => { - let disableScrollRestoration = router?.enableScrollRestoration( - savedScrollPositions, - () => window.scrollY, - getKey - ); - return () => disableScrollRestoration && disableScrollRestoration(); - }, [router, getKey]); - - // Restore scrolling when state.restoreScrollPosition changes - React.useLayoutEffect(() => { - // Skip if the consumer asked us to (used for SSR) - if (skip) { - return; - } - // Explicit false means don't do anything (used for submissions) - if (restoreScrollPosition === false) { - return; - } - - // been here before, scroll to it - if (typeof restoreScrollPosition === "number") { - window.scrollTo(0, restoreScrollPosition); - return; - } + // Explicit false means don't do anything (used for submissions) + if (restoreScrollPosition === false) { + return; + } - // try to scroll to the hash - if (location.hash) { - let el = document.getElementById(location.hash.slice(1)); - if (el) { - el.scrollIntoView(); + // been here before, scroll to it + if (typeof restoreScrollPosition === "number") { + window.scrollTo(0, restoreScrollPosition); return; } - } - // Opt out of scroll reset if this link requested it - if (preventScrollReset === true) { - return; - } + // try to scroll to the hash + if (location.hash) { + let el = document.getElementById(location.hash.slice(1)); + if (el) { + el.scrollIntoView(); + return; + } + } - // otherwise go to the top on new locations - window.scrollTo(0, 0); - }, [location, restoreScrollPosition, preventScrollReset, skip]); + // Opt out of scroll reset if this link requested it + if (preventScrollReset === true) { + return; + } + + // otherwise go to the top on new locations + window.scrollTo(0, 0); + }, [location, restoreScrollPosition, preventScrollReset, skip]); + } } /** From 9c3548f4113df1e334a20784ba9d2a1dc701feeb Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Tue, 13 Dec 2022 10:11:14 -0500 Subject: [PATCH 17/26] Add changesets --- .changeset/afraid-kiwis-grow.md | 5 +++++ .changeset/bright-gorillas-pump.md | 5 +++++ .changeset/empty-teachers-tie.md | 5 +++++ .changeset/itchy-swans-travel.md | 5 +++++ .changeset/small-dots-try.md | 5 +++++ 5 files changed, 25 insertions(+) create mode 100644 .changeset/afraid-kiwis-grow.md create mode 100644 .changeset/bright-gorillas-pump.md create mode 100644 .changeset/empty-teachers-tie.md create mode 100644 .changeset/itchy-swans-travel.md create mode 100644 .changeset/small-dots-try.md diff --git a/.changeset/afraid-kiwis-grow.md b/.changeset/afraid-kiwis-grow.md new file mode 100644 index 0000000000..1fd527e245 --- /dev/null +++ b/.changeset/afraid-kiwis-grow.md @@ -0,0 +1,5 @@ +--- +"react-router-dom": minor +--- + +Add `useBeforeUnload()` hook diff --git a/.changeset/bright-gorillas-pump.md b/.changeset/bright-gorillas-pump.md new file mode 100644 index 0000000000..52bceecc5a --- /dev/null +++ b/.changeset/bright-gorillas-pump.md @@ -0,0 +1,5 @@ +--- +"react-router-dom": patch +--- + +Support uppercase `` and `useSubmit` method values diff --git a/.changeset/empty-teachers-tie.md b/.changeset/empty-teachers-tie.md new file mode 100644 index 0000000000..e54ba35689 --- /dev/null +++ b/.changeset/empty-teachers-tie.md @@ -0,0 +1,5 @@ +--- +"react-router-dom": major +--- + +Proper hydration of `Error` objects from `StaticRouterProvider` diff --git a/.changeset/itchy-swans-travel.md b/.changeset/itchy-swans-travel.md new file mode 100644 index 0000000000..39f30e7e44 --- /dev/null +++ b/.changeset/itchy-swans-travel.md @@ -0,0 +1,5 @@ +--- +"@remix-run/router": patch +--- + +Fix explicit `replace` on submissions and `PUSH` on submission to new paths diff --git a/.changeset/small-dots-try.md b/.changeset/small-dots-try.md new file mode 100644 index 0000000000..798dd37e49 --- /dev/null +++ b/.changeset/small-dots-try.md @@ -0,0 +1,5 @@ +--- +"react-router-dom": patch +--- + +Fix ` - -
- {actionData ?

{actionData}

: null} - -
- - ); - } - - expect(getHtml(container.querySelector(".output"))) - .toMatchInlineSnapshot(` - "
-

- index -

-
" - `); - - fireEvent.click(screen.getByText("Go to 1")); - await waitFor(() => screen.getByText("Page 1")); - expect(getHtml(container.querySelector(".output"))) - .toMatchInlineSnapshot(` - "
-

- Page 1 -

-
" - `); - - fireEvent.click(screen.getByText("Submit")); - await waitFor(() => screen.getByText("action")); - expect(getHtml(container.querySelector(".output"))) - .toMatchInlineSnapshot(` - "
-

- action -

-

- Page 1 -

-
" - `); - fireEvent.click(screen.getByText("Go back")); await waitFor(() => screen.getByText("index")); expect(getHtml(container.querySelector(".output"))) From 71d7b2f9478a3a0bb8e4216e4459e0650f3fd53d Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Thu, 15 Dec 2022 16:50:24 -0500 Subject: [PATCH 22/26] Revert "Updates for fetcher type and redirect replace logic" This reverts commit f188645373ce3400bf982f4be3593758162e95e8. --- packages/router/__tests__/router-test.ts | 108 ++--------------------- packages/router/router.ts | 38 ++------ 2 files changed, 14 insertions(+), 132 deletions(-) diff --git a/packages/router/__tests__/router-test.ts b/packages/router/__tests__/router-test.ts index 5126fedfc9..c625cf77e7 100644 --- a/packages/router/__tests__/router-test.ts +++ b/packages/router/__tests__/router-test.ts @@ -2519,35 +2519,6 @@ describe("a router", () => { expect(t.router.state.location.pathname).toEqual("/foo"); }); - it("navigates correctly using POP navigations across actions to new locations", async () => { - let t = initializeTmTest(); - - // Navigate to /foo - let A = await t.navigate("/foo"); - await A.loaders.foo.resolve("FOO"); - expect(t.router.state.location.pathname).toEqual("/foo"); - - // Navigate to /bar - let B = await t.navigate("/bar"); - await B.loaders.bar.resolve("BAR"); - expect(t.router.state.location.pathname).toEqual("/bar"); - - // Post to /baz (should not replace) - let C = await t.navigate("/baz", { - formMethod: "post", - formData: createFormData({ key: "value" }), - }); - await C.actions.baz.resolve("BAZ ACTION"); - await C.loaders.root.resolve("ROOT"); - await C.loaders.baz.resolve("BAZ"); - expect(t.router.state.location.pathname).toEqual("/baz"); - - // POP to /bar - let D = await t.navigate(-1); - await D.loaders.bar.resolve("BAR"); - expect(t.router.state.location.pathname).toEqual("/bar"); - }); - it("navigates correctly using POP navigations across action errors", async () => { let t = initializeTmTest(); @@ -2696,67 +2667,6 @@ describe("a router", () => { expect(t.router.state.historyAction).toEqual("POP"); expect(t.router.state.location.pathname).toEqual("/foo"); }); - - it("should respect explicit replace:false on non-redirected actions to new locations", async () => { - // start at / (history stack: [/]) - let t = initializeTmTest(); - - // Link to /foo (history stack: [/, /foo]) - let A = await t.navigate("/foo"); - await A.loaders.root.resolve("ROOT"); - await A.loaders.foo.resolve("FOO"); - expect(t.router.state.historyAction).toEqual("PUSH"); - expect(t.router.state.location.pathname).toEqual("/foo"); - - // POST /bar (history stack: [/, /foo, /bar]) - let B = await t.navigate("/bar", { - formMethod: "post", - formData: createFormData({ gosh: "dang" }), - replace: false, - }); - await B.actions.bar.resolve("BAR"); - await B.loaders.root.resolve("ROOT"); - await B.loaders.bar.resolve("BAR"); - expect(t.router.state.historyAction).toEqual("PUSH"); - expect(t.router.state.location.pathname).toEqual("/bar"); - - // POP /foo (history stack: [GET /, GET /foo]) - let C = await t.navigate(-1); - await C.loaders.foo.resolve("FOO"); - expect(t.router.state.historyAction).toEqual("POP"); - expect(t.router.state.location.pathname).toEqual("/foo"); - }); - - it("should respect explicit replace:false on non-redirected actions to the same location", async () => { - // start at / (history stack: [/]) - let t = initializeTmTest(); - - // Link to /foo (history stack: [/, /foo]) - let A = await t.navigate("/foo"); - await A.loaders.root.resolve("ROOT"); - await A.loaders.foo.resolve("FOO"); - expect(t.router.state.historyAction).toEqual("PUSH"); - expect(t.router.state.location.pathname).toEqual("/foo"); - - // POST /foo (history stack: [/, /foo, /foo]) - let B = await t.navigate("/foo", { - formMethod: "post", - formData: createFormData({ gosh: "dang" }), - replace: false, - }); - await B.actions.foo.resolve("FOO2 ACTION"); - await B.loaders.root.resolve("ROOT2"); - await B.loaders.foo.resolve("FOO2"); - expect(t.router.state.historyAction).toEqual("PUSH"); - expect(t.router.state.location.pathname).toEqual("/foo"); - - // POP /foo (history stack: [/, /foo]) - let C = await t.navigate(-1); - await C.loaders.root.resolve("ROOT3"); - await C.loaders.foo.resolve("FOO3"); - expect(t.router.state.historyAction).toEqual("POP"); - expect(t.router.state.location.pathname).toEqual("/foo"); - }); }); describe("submission navigations", () => { @@ -6474,7 +6384,7 @@ describe("a router", () => { await N.loaders.root.resolve("ROOT_DATA*"); await N.loaders.tasks.resolve("TASKS_DATA"); expect(t.router.state).toMatchObject({ - historyAction: "PUSH", + historyAction: "REPLACE", location: { pathname: "/tasks" }, navigation: IDLE_NAVIGATION, revalidation: "idle", @@ -6486,7 +6396,7 @@ describe("a router", () => { tasks: "TASKS_ACTION", }, }); - expect(t.history.push).toHaveBeenCalledWith( + expect(t.history.replace).toHaveBeenCalledWith( t.router.state.location, t.router.state.location.state ); @@ -6686,7 +6596,7 @@ describe("a router", () => { await R.loaders.root.resolve("ROOT_DATA*"); await R.loaders.tasks.resolve("TASKS_DATA*"); expect(t.router.state).toMatchObject({ - historyAction: "PUSH", + historyAction: "REPLACE", location: { pathname: "/tasks" }, navigation: IDLE_NAVIGATION, revalidation: "idle", @@ -6695,7 +6605,7 @@ describe("a router", () => { tasks: "TASKS_DATA*", }, }); - expect(t.history.push).toHaveBeenCalledWith( + expect(t.history.replace).toHaveBeenCalledWith( t.router.state.location, t.router.state.location.state ); @@ -6773,7 +6683,7 @@ describe("a router", () => { await R.loaders.root.resolve("ROOT_DATA*"); await R.loaders.tasks.resolve("TASKS_DATA*"); expect(t.router.state).toMatchObject({ - historyAction: "PUSH", + historyAction: "REPLACE", location: { pathname: "/tasks" }, navigation: IDLE_NAVIGATION, revalidation: "idle", @@ -6785,7 +6695,7 @@ describe("a router", () => { tasks: "TASKS_ACTION", }, }); - expect(t.history.push).toHaveBeenCalledWith( + expect(t.history.replace).toHaveBeenCalledWith( t.router.state.location, t.router.state.location.state ); @@ -7088,7 +6998,7 @@ describe("a router", () => { let key = "key"; router.fetch(key, "root", "/"); - expect(router.state.fetchers.get(key)).toMatchObject({ + expect(router.state.fetchers.get(key)).toEqual({ state: "loading", formMethod: undefined, formEncType: undefined, @@ -7097,7 +7007,7 @@ describe("a router", () => { }); await dfd.resolve("DATA"); - expect(router.state.fetchers.get(key)).toMatchObject({ + expect(router.state.fetchers.get(key)).toEqual({ state: "idle", formMethod: undefined, formEncType: undefined, @@ -7588,7 +7498,7 @@ describe("a router", () => { expect(t.router.state.navigation.location?.pathname).toBe("/bar"); await AR.loaders.root.resolve("ROOT*"); await AR.loaders.bar.resolve("stuff"); - expect(A.fetcher).toMatchObject({ + expect(A.fetcher).toEqual({ data: undefined, state: "idle", formMethod: undefined, diff --git a/packages/router/router.ts b/packages/router/router.ts index 20bf6871c0..82128c0a9c 100644 --- a/packages/router/router.ts +++ b/packages/router/router.ts @@ -435,7 +435,6 @@ type FetcherStates = { formEncType: undefined; formData: undefined; data: TData | undefined; - " _hasFetcherDoneAnything "?: boolean; }; Loading: { state: "loading"; @@ -444,7 +443,6 @@ type FetcherStates = { formEncType: FormEncType | undefined; formData: FormData | undefined; data: TData | undefined; - " _hasFetcherDoneAnything "?: boolean; }; Submitting: { state: "submitting"; @@ -453,7 +451,6 @@ type FetcherStates = { formEncType: FormEncType; formData: FormData; data: TData | undefined; - " _hasFetcherDoneAnything "?: boolean; }; }; @@ -821,26 +818,11 @@ export function createRouter(init: RouterInit): Router { ...init.history.encodeLocation(location), }; - let historyAction = HistoryAction.Push; - - if (opts && opts?.replace === true) { - historyAction = HistoryAction.Replace; - } else if (opts && opts.replace === false) { - // no-op - } else if ( - submission != null && - isMutationMethod(submission.formMethod) && - parsePath(submission.formAction).pathname === state.location.pathname - ) { - // TODO check this with Ryan - - // By default on submissions to the current location we REPLACE so that - // users don't have to double-click the back button to get to the prior - // location. If the user redirects from the action/loader this will be - // ignored and the redirect will be a PUSH - historyAction = HistoryAction.Replace; - } - + let historyAction = + (opts && opts.replace) === true || + (submission != null && isMutationMethod(submission.formMethod)) + ? HistoryAction.Replace + : HistoryAction.Push; let preventScrollReset = opts && "preventScrollReset" in opts ? opts.preventScrollReset === true @@ -1179,7 +1161,6 @@ export function createRouter(init: RouterInit): Router { formAction: undefined, formEncType: undefined, formData: undefined, - " _hasFetcherDoneAnything ": true, }; state.fetchers.set(key, revalidatingFetcher); }); @@ -1332,7 +1313,6 @@ export function createRouter(init: RouterInit): Router { state: "submitting", ...submission, data: existingFetcher && existingFetcher.data, - " _hasFetcherDoneAnything ": true, }; state.fetchers.set(key, fetcher); updateState({ fetchers: new Map(state.fetchers) }); @@ -1370,7 +1350,6 @@ export function createRouter(init: RouterInit): Router { state: "loading", ...submission, data: undefined, - " _hasFetcherDoneAnything ": true, }; state.fetchers.set(key, loadingFetcher); updateState({ fetchers: new Map(state.fetchers) }); @@ -1409,7 +1388,6 @@ export function createRouter(init: RouterInit): Router { state: "loading", data: actionResult.data, ...submission, - " _hasFetcherDoneAnything ": true, }; state.fetchers.set(key, loadFetcher); @@ -1440,7 +1418,6 @@ export function createRouter(init: RouterInit): Router { formAction: undefined, formEncType: undefined, formData: undefined, - " _hasFetcherDoneAnything ": true, }; state.fetchers.set(staleKey, revalidatingFetcher); fetchControllers.set(staleKey, abortController); @@ -1491,7 +1468,6 @@ export function createRouter(init: RouterInit): Router { formAction: undefined, formEncType: undefined, formData: undefined, - " _hasFetcherDoneAnything ": true, }; state.fetchers.set(key, doneFetcher); @@ -1545,7 +1521,6 @@ export function createRouter(init: RouterInit): Router { formData: undefined, ...submission, data: existingFetcher && existingFetcher.data, - " _hasFetcherDoneAnything ": true, }; state.fetchers.set(key, loadingFetcher); updateState({ fetchers: new Map(state.fetchers) }); @@ -1614,7 +1589,6 @@ export function createRouter(init: RouterInit): Router { formAction: undefined, formEncType: undefined, formData: undefined, - " _hasFetcherDoneAnything ": true, }; state.fetchers.set(key, doneFetcher); updateState({ fetchers: new Map(state.fetchers) }); @@ -1820,7 +1794,6 @@ export function createRouter(init: RouterInit): Router { formAction: undefined, formEncType: undefined, formData: undefined, - " _hasFetcherDoneAnything ": true, }; state.fetchers.set(key, doneFetcher); } @@ -3016,7 +2989,6 @@ function processLoaderData( formAction: undefined, formEncType: undefined, formData: undefined, - " _hasFetcherDoneAnything ": true, }; state.fetchers.set(key, doneFetcher); } From 2da3b2ce38c40706d18bd80e0b30402cb1924493 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Thu, 15 Dec 2022 16:51:24 -0500 Subject: [PATCH 23/26] Add _hasFetcherdoneAnything for Remix back-compat --- packages/router/router.ts | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/packages/router/router.ts b/packages/router/router.ts index 82128c0a9c..2ea4621da9 100644 --- a/packages/router/router.ts +++ b/packages/router/router.ts @@ -435,6 +435,7 @@ type FetcherStates = { formEncType: undefined; formData: undefined; data: TData | undefined; + " _hasFetcherDoneAnything "?: boolean; }; Loading: { state: "loading"; @@ -443,6 +444,7 @@ type FetcherStates = { formEncType: FormEncType | undefined; formData: FormData | undefined; data: TData | undefined; + " _hasFetcherDoneAnything "?: boolean; }; Submitting: { state: "submitting"; @@ -451,6 +453,7 @@ type FetcherStates = { formEncType: FormEncType; formData: FormData; data: TData | undefined; + " _hasFetcherDoneAnything "?: boolean; }; }; @@ -1161,6 +1164,7 @@ export function createRouter(init: RouterInit): Router { formAction: undefined, formEncType: undefined, formData: undefined, + " _hasFetcherDoneAnything ": true, }; state.fetchers.set(key, revalidatingFetcher); }); @@ -1313,6 +1317,7 @@ export function createRouter(init: RouterInit): Router { state: "submitting", ...submission, data: existingFetcher && existingFetcher.data, + " _hasFetcherDoneAnything ": true, }; state.fetchers.set(key, fetcher); updateState({ fetchers: new Map(state.fetchers) }); @@ -1350,6 +1355,7 @@ export function createRouter(init: RouterInit): Router { state: "loading", ...submission, data: undefined, + " _hasFetcherDoneAnything ": true, }; state.fetchers.set(key, loadingFetcher); updateState({ fetchers: new Map(state.fetchers) }); @@ -1388,6 +1394,7 @@ export function createRouter(init: RouterInit): Router { state: "loading", data: actionResult.data, ...submission, + " _hasFetcherDoneAnything ": true, }; state.fetchers.set(key, loadFetcher); @@ -1418,6 +1425,7 @@ export function createRouter(init: RouterInit): Router { formAction: undefined, formEncType: undefined, formData: undefined, + " _hasFetcherDoneAnything ": true, }; state.fetchers.set(staleKey, revalidatingFetcher); fetchControllers.set(staleKey, abortController); @@ -1468,6 +1476,7 @@ export function createRouter(init: RouterInit): Router { formAction: undefined, formEncType: undefined, formData: undefined, + " _hasFetcherDoneAnything ": true, }; state.fetchers.set(key, doneFetcher); @@ -1521,6 +1530,7 @@ export function createRouter(init: RouterInit): Router { formData: undefined, ...submission, data: existingFetcher && existingFetcher.data, + " _hasFetcherDoneAnything ": true, }; state.fetchers.set(key, loadingFetcher); updateState({ fetchers: new Map(state.fetchers) }); @@ -1589,6 +1599,7 @@ export function createRouter(init: RouterInit): Router { formAction: undefined, formEncType: undefined, formData: undefined, + " _hasFetcherDoneAnything ": true, }; state.fetchers.set(key, doneFetcher); updateState({ fetchers: new Map(state.fetchers) }); @@ -1794,6 +1805,7 @@ export function createRouter(init: RouterInit): Router { formAction: undefined, formEncType: undefined, formData: undefined, + " _hasFetcherDoneAnything ": true, }; state.fetchers.set(key, doneFetcher); } @@ -2989,6 +3001,7 @@ function processLoaderData( formAction: undefined, formEncType: undefined, formData: undefined, + " _hasFetcherDoneAnything ": true, }; state.fetchers.set(key, doneFetcher); } From c3642e075546ec3366883e2b0e7b2a269e6cb1d7 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Thu, 15 Dec 2022 17:25:41 -0500 Subject: [PATCH 24/26] Remove changeset for redirect logic in new PR --- .changeset/itchy-swans-travel.md | 5 ----- 1 file changed, 5 deletions(-) delete mode 100644 .changeset/itchy-swans-travel.md diff --git a/.changeset/itchy-swans-travel.md b/.changeset/itchy-swans-travel.md deleted file mode 100644 index 39f30e7e44..0000000000 --- a/.changeset/itchy-swans-travel.md +++ /dev/null @@ -1,5 +0,0 @@ ---- -"@remix-run/router": patch ---- - -Fix explicit `replace` on submissions and `PUSH` on submission to new paths From 86699a1da8da09bd4a039f3fc366c8987067cfa2 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Thu, 15 Dec 2022 17:30:24 -0500 Subject: [PATCH 25/26] Fix tests --- packages/router/__tests__/router-test.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/packages/router/__tests__/router-test.ts b/packages/router/__tests__/router-test.ts index c625cf77e7..313b503df6 100644 --- a/packages/router/__tests__/router-test.ts +++ b/packages/router/__tests__/router-test.ts @@ -7004,6 +7004,7 @@ describe("a router", () => { formEncType: undefined, formData: undefined, data: undefined, + " _hasFetcherDoneAnything ": true, }); await dfd.resolve("DATA"); @@ -7013,6 +7014,7 @@ describe("a router", () => { formEncType: undefined, formData: undefined, data: "DATA", + " _hasFetcherDoneAnything ": true, }); expect(router._internalFetchControllers.size).toBe(0); @@ -7505,6 +7507,7 @@ describe("a router", () => { formAction: undefined, formEncType: undefined, formData: undefined, + " _hasFetcherDoneAnything ": true, }); expect(t.router.state.historyAction).toBe("PUSH"); expect(t.router.state.location.pathname).toBe("/bar"); From 10b6c5b7bb6aac8e8d1c7608036224c7531592e5 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Thu, 15 Dec 2022 17:32:46 -0500 Subject: [PATCH 26/26] Add changeset --- .changeset/shiny-pants-decide.md | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 .changeset/shiny-pants-decide.md diff --git a/.changeset/shiny-pants-decide.md b/.changeset/shiny-pants-decide.md new file mode 100644 index 0000000000..b5d57d185e --- /dev/null +++ b/.changeset/shiny-pants-decide.md @@ -0,0 +1,6 @@ +--- +"react-router-dom": patch +"@remix-run/router": patch +--- + +Skip initial scroll restoration for SSR apps with hydrationData