diff --git a/.changeset/proud-timers-tickle.md b/.changeset/proud-timers-tickle.md new file mode 100644 index 0000000000..f0f18b7987 --- /dev/null +++ b/.changeset/proud-timers-tickle.md @@ -0,0 +1,6 @@ +--- +"@remix-run/router": patch +--- + +- Throw an error if an `action`/`loader` function returns `undefined` as revalidations need to know whether the loader has previously been executed. `undefined` also causes issues during SSR stringification for hydration. You should always ensure you loader/acton return a value, and you may return `null` if you don't wish to return anything. +- Enhanced `ErrorResponse` bodies to contain more descriptive text in internal 403/404/405 scenarios diff --git a/package.json b/package.json index 1ce68898d3..46a937dd23 100644 --- a/package.json +++ b/package.json @@ -107,7 +107,7 @@ }, "filesize": { "packages/router/dist/router.umd.min.js": { - "none": "34 kB" + "none": "34.5 kB" }, "packages/react-router/dist/react-router.production.min.js": { "none": "12.5 kB" diff --git a/packages/react-router-dom/__tests__/nav-link-active-test.tsx b/packages/react-router-dom/__tests__/nav-link-active-test.tsx index 4e00e30fee..f44dce59a9 100644 --- a/packages/react-router-dom/__tests__/nav-link-active-test.tsx +++ b/packages/react-router-dom/__tests__/nav-link-active-test.tsx @@ -509,7 +509,7 @@ describe("NavLink using a data router", () => { fireEvent.click(screen.getByText("Link to Bar")); expect(screen.getByText("Link to Bar").className).toBe("pending"); - dfd.resolve(); + dfd.resolve(null); await waitFor(() => screen.getByText("Bar page")); expect(screen.getByText("Link to Bar").className).toBe("active"); }); @@ -562,7 +562,7 @@ describe("NavLink using a data router", () => { "some-pending-classname" ); - dfd.resolve(); + dfd.resolve(null); await waitFor(() => screen.getByText("Bar page")); expect(screen.getByText("Link to Bar").className).toBe( "some-active-classname" @@ -617,7 +617,7 @@ describe("NavLink using a data router", () => { "lowercase" ); - dfd.resolve(); + dfd.resolve(null); await waitFor(() => screen.getByText("Bar page")); expect(screen.getByText("Link to Bar").style.textTransform).toBe( "uppercase" @@ -667,7 +667,7 @@ describe("NavLink using a data router", () => { fireEvent.click(screen.getByText("Link to Bar (idle)")); expect(screen.getByText("Link to Bar (loading...)")).toBeDefined(); - dfd.resolve(); + dfd.resolve(null); await waitFor(() => screen.getByText("Bar page")); expect(screen.getByText("Link to Bar (current)")).toBeDefined(); }); @@ -708,7 +708,7 @@ describe("NavLink using a data router", () => { fireEvent.click(screen.getByText("Link to Baz")); expect(screen.getByText("Link to Bar").className).toBe(""); - dfd.resolve(); + dfd.resolve(null); await waitFor(() => screen.getByText("Baz page")); expect(screen.getByText("Link to Bar").className).toBe(""); }); diff --git a/packages/react-router/__tests__/data-memory-router-test.tsx b/packages/react-router/__tests__/data-memory-router-test.tsx index e6e57ae87b..5c850f3096 100644 --- a/packages/react-router/__tests__/data-memory-router-test.tsx +++ b/packages/react-router/__tests__/data-memory-router-test.tsx @@ -2713,7 +2713,7 @@ describe("", () => { expect(getAwaitRenderCount()).toBe(3); // complete /baz navigation - bazDefer.resolve(); + bazDefer.resolve(null); await waitFor(() => screen.getByText("Baz")); expect(getHtml(container)).toMatchInlineSnapshot(` "
{ root: "ROOT", }); expect(t.router.state.errors).toEqual({ - root: { - status: 404, - statusText: "Not Found", - data: null, - }, + root: new ErrorResponse( + 404, + "Not Found", + new Error('No route matches URL "/not-found"'), + true + ), }); expect(t.router.state.matches).toMatchObject([ { @@ -2134,7 +2139,6 @@ describe("a router", () => { route: { hasErrorBoundary: true, children: expect.any(Array), - id: "root", loader: expect.any(Function), module: "", @@ -2151,11 +2155,12 @@ describe("a router", () => { t.navigate("/not-found"); expect(t.router.state.errors).toEqual({ - root: { - status: 404, - statusText: "Not Found", - data: null, - }, + root: new ErrorResponse( + 404, + "Not Found", + new Error('No route matches URL "/not-found"'), + true + ), }); expect(t.router.state.matches).toMatchObject([ { @@ -2197,11 +2202,12 @@ describe("a router", () => { root: "ROOT*", }); expect(t.router.state.errors).toEqual({ - root: { - status: 404, - statusText: "Not Found", - data: null, - }, + root: new ErrorResponse( + 404, + "Not Found", + new Error('No route matches URL "/not-found"'), + true + ), }); expect(t.router.state.matches).toMatchObject([ { @@ -2210,7 +2216,6 @@ describe("a router", () => { route: { hasErrorBoundary: true, children: expect.any(Array), - id: "root", loader: expect.any(Function), module: "", @@ -3153,9 +3158,16 @@ describe("a router", () => { formData: createFormData({ gosh: "dang" }), }); expect(t.router.state.errors).toEqual({ - child: new ErrorResponse(405, "Method Not Allowed", ""), + child: new ErrorResponse( + 405, + "Method Not Allowed", + new Error( + 'You made a POST request to "/child" but did not provide an ' + + '`action` for route "child", so there is no way to handle the request.' + ), + true + ), }); - expect(console.warn).toHaveBeenCalled(); spy.mockReset(); }); @@ -3206,7 +3218,16 @@ describe("a router", () => { }); expect(t.router.state.actionData).toBe(null); expect(t.router.state.errors).toEqual({ - grandchild: new ErrorResponse(405, "Method Not Allowed", ""), + grandchild: new ErrorResponse( + 405, + "Method Not Allowed", + new Error( + 'You made a POST request to "/child/grandchild" but did not ' + + 'provide an `action` for route "grandchild", so there is no way ' + + "to handle the request." + ), + true + ), }); }); }); @@ -3823,11 +3844,12 @@ describe("a router", () => { navigation: IDLE_NAVIGATION, loaderData: {}, errors: { - root: { - status: 404, - statusText: "Not Found", - data: null, - }, + root: new ErrorResponse( + 404, + "Not Found", + new Error('No route matches URL "/junk"'), + true + ), }, }); }); @@ -3938,7 +3960,8 @@ describe("a router", () => { tasks: new ErrorResponse( 400, "Bad Request", - "Cannot submit binary form data using GET" + new Error("Cannot submit binary form data using GET"), + true ), }); }); @@ -3992,7 +4015,8 @@ describe("a router", () => { child: new ErrorResponse( 400, "Bad Request", - "Cannot submit binary form data using GET" + new Error("Cannot submit binary form data using GET"), + true ), }); }); @@ -5069,6 +5093,61 @@ describe("a router", () => { router.dispose(); }); + + it("throws an error if actions/loaders return undefined", async () => { + let t = setup({ + routes: [ + { + index: true, + }, + { + id: "path", + path: "/path", + loader: true, + action: true, + }, + ], + }); + + let nav1 = await t.navigate("/path"); + await nav1.loaders.path.resolve(undefined); + expect(t.router.state).toMatchObject({ + location: { + pathname: "/path", + }, + errors: { + path: new Error( + 'You defined a loader for route "path" but didn\'t return anything ' + + "from your `loader` function. Please return a value or `null`." + ), + }, + }); + + await t.navigate("/"); + expect(t.router.state).toMatchObject({ + location: { + pathname: "/", + }, + errors: {}, + }); + + let nav3 = await t.navigate("/path", { + formMethod: "post", + formData: createFormData({}), + }); + await nav3.actions.path.resolve(undefined); + expect(t.router.state).toMatchObject({ + location: { + pathname: "/path", + }, + errors: { + path: new Error( + 'You defined an action for route "path" but didn\'t return anything ' + + "from your `action` function. Please return a value or `null`." + ), + }, + }); + }); }); describe("redirects", () => { @@ -6739,7 +6818,15 @@ describe("a router", () => { }); expect(A.fetcher).toBe(IDLE_FETCHER); expect(t.router.state.errors).toEqual({ - root: new ErrorResponse(405, "Method Not Allowed", ""), + root: new ErrorResponse( + 405, + "Method Not Allowed", + new Error( + 'You made a post request to "/" but did not provide a `loader` ' + + 'for route "root", so there is no way to handle the request.' + ), + true + ), }); }); @@ -6783,7 +6870,12 @@ describe("a router", () => { await t.fetch("/not-found", "key2", "wit"); expect(t.router.getFetcher("key2")).toBe(IDLE_FETCHER); expect(t.router.state.errors).toEqual({ - root: new ErrorResponse(404, "Not Found", null), + root: new ErrorResponse( + 404, + "Not Found", + new Error('No route matches URL "/not-found"'), + true + ), }); // Navigate to /wit and trigger errors, handled at the wit boundary @@ -6803,13 +6895,23 @@ describe("a router", () => { }); expect(t.router.getFetcher("key4")).toBe(IDLE_FETCHER); expect(t.router.state.errors).toEqual({ - wit: new ErrorResponse(404, "Not Found", null), + wit: new ErrorResponse( + 404, + "Not Found", + new Error('No route matches URL "/not-found"'), + true + ), }); await t.fetch("/not-found", "key5", "wit"); expect(t.router.getFetcher("key5")).toBe(IDLE_FETCHER); expect(t.router.state.errors).toEqual({ - wit: new ErrorResponse(404, "Not Found", null), + wit: new ErrorResponse( + 404, + "Not Found", + new Error('No route matches URL "/not-found"'), + true + ), }); // Navigate to /witout and fetch a 404, handled at the root boundary @@ -6826,7 +6928,12 @@ describe("a router", () => { await t.fetch("/not-found", "key7", "witout"); expect(t.router.getFetcher("key7")).toBe(IDLE_FETCHER); expect(t.router.state.errors).toEqual({ - root: new ErrorResponse(404, "Not Found", null), + root: new ErrorResponse( + 404, + "Not Found", + new Error('No route matches URL "/not-found"'), + true + ), }); }); }); @@ -8893,7 +9000,8 @@ describe("a router", () => { parent: new ErrorResponse( 400, "Bad Request", - "Cannot submit binary form data using GET" + new Error("Cannot submit binary form data using GET"), + true ), }); @@ -9004,7 +9112,8 @@ describe("a router", () => { bChild: new ErrorResponse( 400, "Bad Request", - "Cannot submit binary form data using GET" + new Error("Cannot submit binary form data using GET"), + true ), }); expect(B.loaders.bChild.stub).not.toHaveBeenCalled(); @@ -10196,11 +10305,12 @@ describe("a router", () => { loaderData: {}, actionData: null, errors: { - index: { - data: null, - status: 404, - statusText: "Not Found", - }, + index: new ErrorResponse( + 404, + "Not Found", + new Error('No route matches URL "/not/found"'), + true + ), }, matches: [{ route: { id: "index" } }], }); @@ -10347,11 +10457,15 @@ describe("a router", () => { actionData: null, loaderData: {}, errors: { - root: { - status: 405, - statusText: "Method Not Allowed", - data: "", - }, + root: new ErrorResponse( + 405, + "Method Not Allowed", + new Error( + 'You made a POST request to "/" but did not provide an `action` ' + + 'for route "root", so there is no way to handle the request.' + ), + true + ), }, matches: [{ route: { id: "root" } }], }); @@ -10370,11 +10484,12 @@ describe("a router", () => { actionData: null, loaderData: {}, errors: { - root: { - status: 405, - statusText: "Method Not Allowed", - data: null, - }, + root: new ErrorResponse( + 405, + "Method Not Allowed", + new Error('Invalid request method "OPTIONS"'), + true + ), }, matches: [{ route: { id: "root" } }], }); @@ -11066,6 +11181,35 @@ describe("a router", () => { expect(data).toBe(""); }); + it("should error if an action/loader returns undefined", async () => { + let T = setupFlexRouteTest(); + let data; + + try { + data = await T.resolveLoader(undefined); + } catch (e) { + data = e; + } + expect(data).toEqual( + new Error( + 'You defined a loader for route "flex" but didn\'t return anything ' + + "from your `loader` function. Please return a value or `null`." + ) + ); + + try { + data = await T.resolveAction(undefined); + } catch (e) { + data = e; + } + expect(data).toEqual( + new Error( + 'You defined an action for route "flex" but didn\'t return anything ' + + "from your `action` function. Please return a value or `null`." + ) + ); + }); + it("should handle relative redirect responses (loader)", async () => { let { queryRoute } = createStaticHandler([ { @@ -11214,7 +11358,7 @@ describe("a router", () => { ); }); - it("should handle not found routes with a 404 Response", async () => { + describe("Errors with Status Codes", () => { /* eslint-disable jest/no-conditional-expect */ let { queryRoute } = createStaticHandler([ { @@ -11223,93 +11367,104 @@ describe("a router", () => { }, ]); - try { - await queryRoute(createRequest("/junk")); - expect(false).toBe(true); - } catch (data) { - expect(data instanceof Response).toBe(true); - expect(data.status).toBe(404); - expect(data.statusText).toBe("Not Found"); - expect(data.headers.get("X-Remix-Router-Error")).toBe("yes"); - } - - try { - await queryRoute(createRequest("/"), "junk"); - expect(false).toBe(true); - } catch (data) { - expect(data instanceof Response).toBe(true); - expect(data.status).toBe(404); - expect(data.statusText).toBe("Not Found"); - expect(data.headers.get("X-Remix-Router-Error")).toBe("yes"); - } + it("should handle not found paths with a 404 Response", async () => { + try { + await queryRoute(createRequest("/junk")); + expect(false).toBe(true); + } catch (data) { + expect(isRouteErrorResponse(data)).toBe(true); + expect(data.status).toBe(404); + expect(data.error).toEqual( + new Error('No route matches URL "/junk"') + ); + expect(data.internal).toBe(true); + } - try { - await queryRoute(createSubmitRequest("/junk")); - expect(false).toBe(true); - } catch (data) { - expect(data instanceof Response).toBe(true); - expect(data.status).toBe(404); - expect(data.statusText).toBe("Not Found"); - expect(data.headers.get("X-Remix-Router-Error")).toBe("yes"); - } + try { + await queryRoute(createSubmitRequest("/junk")); + expect(false).toBe(true); + } catch (data) { + expect(isRouteErrorResponse(data)).toBe(true); + expect(data.status).toBe(404); + expect(data.error).toEqual( + new Error('No route matches URL "/junk"') + ); + expect(data.internal).toBe(true); + } + }); - try { - await queryRoute(createSubmitRequest("/"), "junk"); - expect(false).toBe(true); - } catch (data) { - expect(data instanceof Response).toBe(true); - expect(data.status).toBe(404); - expect(data.statusText).toBe("Not Found"); - expect(data.headers.get("X-Remix-Router-Error")).toBe("yes"); - } + it("should handle not found routeIds with a 403 Response", async () => { + try { + await queryRoute(createRequest("/"), "junk"); + expect(false).toBe(true); + } catch (data) { + expect(isRouteErrorResponse(data)).toBe(true); + expect(data.status).toBe(403); + expect(data.error).toEqual( + new Error('Route "junk" does not match URL "/"') + ); + expect(data.internal).toBe(true); + } - /* eslint-enable jest/no-conditional-expect */ - }); + try { + await queryRoute(createSubmitRequest("/"), "junk"); + expect(false).toBe(true); + } catch (data) { + expect(isRouteErrorResponse(data)).toBe(true); + expect(data.status).toBe(403); + expect(data.error).toEqual( + new Error('Route "junk" does not match URL "/"') + ); + expect(data.internal).toBe(true); + } + }); - it("should handle not found action submissions with a 405 Response", async () => { - /* eslint-disable jest/no-conditional-expect */ - let { queryRoute } = createStaticHandler([ - { - id: "root", - path: "/", - }, - ]); + it("should handle not found action/loader submissions with a 405 Response", async () => { + try { + await queryRoute(createRequest("/"), "root"); + expect(false).toBe(true); + } catch (data) { + expect(isRouteErrorResponse(data)).toBe(true); + expect(data.status).toBe(405); + expect(data.error).toEqual( + new Error( + 'You made a GET request to "/" but did not provide a `loader` ' + + 'for route "root", so there is no way to handle the request.' + ) + ); + expect(data.internal).toBe(true); + } - try { - await queryRoute(createSubmitRequest("/"), "root"); - expect(false).toBe(true); - } catch (data) { - expect(data instanceof Response).toBe(true); - expect(data.status).toBe(405); - expect(data.statusText).toBe("Method Not Allowed"); - expect(data.headers.get("X-Remix-Router-Error")).toBe("yes"); - expect(await data.text()).toBe(""); - } - /* eslint-enable jest/no-conditional-expect */ - }); + try { + await queryRoute(createSubmitRequest("/"), "root"); + expect(false).toBe(true); + } catch (data) { + expect(isRouteErrorResponse(data)).toBe(true); + expect(data.status).toBe(405); + expect(data.error).toEqual( + new Error( + 'You made a POST request to "/" but did not provide an `action` ' + + 'for route "root", so there is no way to handle the request.' + ) + ); + expect(data.internal).toBe(true); + } + }); - it("should handle unsupported methods with a 405 Response", async () => { - /* eslint-disable jest/no-conditional-expect */ - let { queryRoute } = createStaticHandler([ - { - id: "root", - path: "/", - }, - ]); + it("should handle unsupported methods with a 405 Response", async () => { + try { + await queryRoute(createRequest("/", { method: "OPTIONS" }), "root"); + expect(false).toBe(true); + } catch (data) { + expect(isRouteErrorResponse(data)).toBe(true); + expect(data.status).toBe(405); + expect(data.error).toEqual( + new Error('Invalid request method "OPTIONS"') + ); + expect(data.internal).toBe(true); + } + }); - try { - await queryRoute( - createSubmitRequest("/", { method: "OPTIONS" }), - "root" - ); - expect(false).toBe(true); - } catch (data) { - expect(data instanceof Response).toBe(true); - expect(data.status).toBe(405); - expect(data.statusText).toBe("Method Not Allowed"); - expect(data.headers.get("X-Remix-Router-Error")).toBe("yes"); - expect(await data.text()).toBe(""); - } /* eslint-enable jest/no-conditional-expect */ }); }); diff --git a/packages/router/router.ts b/packages/router/router.ts index 700b5c463a..51e02187de 100644 --- a/packages/router/router.ts +++ b/packages/router/router.ts @@ -568,7 +568,10 @@ export function createRouter(init: RouterInit): Router { if (initialMatches == null) { // If we do not match a user-provided-route, fall back to the root // to allow the error boundary to take over - let { matches, route, error } = getNotFoundMatches(dataRoutes); + let error = getInternalRouterError(404, { + pathname: init.history.location.pathname, + }); + let { matches, route } = getShortCircuitMatches(dataRoutes); initialMatches = matches; initialErrors = { [route.id]: error }; } @@ -858,11 +861,9 @@ export function createRouter(init: RouterInit): Router { // Short circuit with a 404 on the root error boundary if we match nothing if (!matches) { - let { - matches: notFoundMatches, - route, - error, - } = getNotFoundMatches(dataRoutes); + let error = getInternalRouterError(404, { pathname: location.pathname }); + let { matches: notFoundMatches, route } = + getShortCircuitMatches(dataRoutes); // Cancel all pending deferred on 404s since we don't keep any routes cancelActiveDeferreds(); completeNavigation(location, { @@ -976,7 +977,14 @@ export function createRouter(init: RouterInit): Router { let actionMatch = getTargetMatch(matches, location); if (!actionMatch.route.action) { - result = getMethodNotAllowedResult(location); + result = { + type: ResultType.error, + error: getInternalRouterError(405, { + method: request.method, + pathname: location.pathname, + routeId: actionMatch.route.id, + }), + }; } else { result = await callLoaderOrAction( "action", @@ -1208,7 +1216,11 @@ export function createRouter(init: RouterInit): Router { let matches = matchRoutes(dataRoutes, href, init.basename); if (!matches) { - setFetcherError(key, routeId, new ErrorResponse(404, "Not Found", null)); + setFetcherError( + key, + routeId, + getInternalRouterError(404, { pathname: href }) + ); return; } @@ -1240,7 +1252,11 @@ export function createRouter(init: RouterInit): Router { fetchLoadMatches.delete(key); if (!match.route.action) { - let { error } = getMethodNotAllowedResult(path); + let error = getInternalRouterError(405, { + method: submission.formMethod, + pathname: path, + routeId: routeId, + }); setFetcherError(key, routeId, error); return; } @@ -1864,11 +1880,9 @@ export function unstable_createStaticHandler( let matches = matchRoutes(dataRoutes, location); if (!validRequestMethods.has(request.method)) { - let { - matches: methodNotAllowedMatches, - route, - error, - } = getMethodNotAllowedMatches(dataRoutes); + let error = getInternalRouterError(405, { method: request.method }); + let { matches: methodNotAllowedMatches, route } = + getShortCircuitMatches(dataRoutes); return { location, matches: methodNotAllowedMatches, @@ -1882,11 +1896,9 @@ export function unstable_createStaticHandler( actionHeaders: {}, }; } else if (!matches) { - let { - matches: notFoundMatches, - route, - error, - } = getNotFoundMatches(dataRoutes); + let error = getInternalRouterError(404, { pathname: location.pathname }); + let { matches: notFoundMatches, route } = + getShortCircuitMatches(dataRoutes); return { location, matches: notFoundMatches, @@ -1925,9 +1937,12 @@ export function unstable_createStaticHandler( * can do proper boundary identification in Remix where a thrown Response * must go to the Catch Boundary but a returned Response is happy-path. * - * One thing to note is that any Router-initiated thrown Response (such as a - * 404 or 405) will have a custom X-Remix-Router-Error: "yes" header on it - * in order to differentiate from responses thrown from user actions/loaders. + * One thing to note is that any Router-initiated Errors that make sense + * to associate with a status code will be thrown as an ErrorResponse + * instance which include the raw Error, such that the calling context can + * serialize the error as they see fit while including the proper response + * code. Examples here are 404 and 405 errors that occur prior to reaching + * any user-defined loaders. */ async function queryRoute(request: Request, routeId?: string): Promise { let url = new URL(request.url); @@ -1935,26 +1950,23 @@ export function unstable_createStaticHandler( let matches = matchRoutes(dataRoutes, location); if (!validRequestMethods.has(request.method)) { - throw createRouterErrorResponse(null, { - status: 405, - statusText: "Method Not Allowed", - }); + throw getInternalRouterError(405, { method: request.method }); } else if (!matches) { - throw createRouterErrorResponse(null, { - status: 404, - statusText: "Not Found", - }); + throw getInternalRouterError(404, { pathname: location.pathname }); } let match = routeId ? matches.find((m) => m.route.id === routeId) : getTargetMatch(matches, location); - if (!match) { - throw createRouterErrorResponse(null, { - status: 404, - statusText: "Not Found", + if (routeId && !match) { + throw getInternalRouterError(403, { + pathname: location.pathname, + routeId, }); + } else if (!match) { + // This should never hit I don't think? + throw getInternalRouterError(404, { pathname: location.pathname }); } let result = await queryImpl(request, location, matches, match); @@ -2032,14 +2044,20 @@ export function unstable_createStaticHandler( isRouteRequest: boolean ): Promise | Response> { let result: DataResult; + if (!actionMatch.route.action) { + let error = getInternalRouterError(405, { + method: request.method, + pathname: createURL(request.url).pathname, + routeId: actionMatch.route.id, + }); if (isRouteRequest) { - throw createRouterErrorResponse(null, { - status: 405, - statusText: "Method Not Allowed", - }); + throw error; } - result = getMethodNotAllowedResult(request.url); + result = { + type: ResultType.error, + error, + }; } else { result = await callLoaderOrAction( "action", @@ -2078,20 +2096,7 @@ export function unstable_createStaticHandler( // Note: This should only be non-Response values if we get here, since // isRouteRequest should throw any Response received in callLoaderOrAction if (isErrorResult(result)) { - let boundaryMatch = findNearestBoundary(matches, actionMatch.route.id); - return { - matches: [actionMatch], - loaderData: {}, - actionData: null, - errors: { - [boundaryMatch.route.id]: result.error, - }, - // Note: statusCode + headers are unused here since queryRoute will - // return the raw Response or value - statusCode: 500, - loaderHeaders: {}, - actionHeaders: {}, - }; + throw result.error; } return { @@ -2153,6 +2158,16 @@ export function unstable_createStaticHandler( | Response > { let isRouteRequest = routeMatch != null; + + // Short circuit if we have no loaders to run (queryRoute()) + if (isRouteRequest && !routeMatch?.route.loader) { + throw getInternalRouterError(405, { + method: request.method, + pathname: createURL(request.url).pathname, + routeId: routeMatch?.route.id, + }); + } + let requestMatches = routeMatch ? [routeMatch] : getLoaderMatchesUntilBoundary( @@ -2161,7 +2176,7 @@ export function unstable_createStaticHandler( ); let matchesToLoad = requestMatches.filter((m) => m.route.loader); - // Short circuit if we have no loaders to run + // Short circuit if we have no loaders to run (query()) if (matchesToLoad.length === 0) { return { matches, @@ -2213,19 +2228,6 @@ export function unstable_createStaticHandler( }; } - function createRouterErrorResponse( - body: BodyInit | null | undefined, - init: ResponseInit - ) { - return new Response(body, { - ...init, - headers: { - ...init.headers, - "X-Remix-Router-Error": "yes", - }, - }); - } - return { dataRoutes, query, @@ -2313,11 +2315,7 @@ function normalizeNavigateOptions( } catch (e) { return { path, - error: new ErrorResponse( - 400, - "Bad Request", - "Cannot submit binary form data using GET" - ), + error: getInternalRouterError(400), }; } @@ -2531,6 +2529,13 @@ async function callLoaderOrAction( handler({ request, params: match.params }), abortPromise, ]); + + invariant( + result !== undefined, + `You defined ${type === "action" ? "an action" : "a loader"} for route ` + + `"${match.route.id}" but didn't return anything from your \`${type}\` ` + + `function. Please return a value or \`null\`.` + ); } catch (e) { resultType = ResultType.error; result = e; @@ -2851,18 +2856,13 @@ function findNearestBoundary( ); } -function getShortCircuitMatches( - routes: AgnosticDataRouteObject[], - status: number, - statusText: string -): { +function getShortCircuitMatches(routes: AgnosticDataRouteObject[]): { matches: AgnosticDataRouteMatch[]; route: AgnosticDataRouteObject; - error: ErrorResponse; } { // Prefer a root layout route if present, otherwise shim in a route object let route = routes.find((r) => r.index || !r.path || r.path === "/") || { - id: `__shim-${status}-route__`, + id: `__shim-error-route__`, }; return { @@ -2875,29 +2875,63 @@ function getShortCircuitMatches( }, ], route, - error: new ErrorResponse(status, statusText, null), }; } -function getNotFoundMatches(routes: AgnosticDataRouteObject[]) { - return getShortCircuitMatches(routes, 404, "Not Found"); -} - -function getMethodNotAllowedMatches(routes: AgnosticDataRouteObject[]) { - return getShortCircuitMatches(routes, 405, "Method Not Allowed"); -} +function getInternalRouterError( + status: number, + { + pathname, + routeId, + method, + message, + }: { + pathname?: string; + routeId?: string; + method?: string; + message?: string; + } = {} +) { + let statusText: string; + let errorMessage = message; + + if (status === 400) { + statusText = "Bad Request"; + errorMessage = "Cannot submit binary form data using GET"; + } else if (status === 403) { + statusText = "Forbidden"; + errorMessage = `Route "${routeId}" does not match URL "${pathname}"`; + } else if (status === 404) { + statusText = "Not Found"; + errorMessage = `No route matches URL "${pathname}"`; + } else if (status === 405) { + statusText = "Method Not Allowed"; + if (method && pathname && routeId) { + if (validActionMethods.has(method)) { + errorMessage = + `You made a ${method} request to "${pathname}" but ` + + `did not provide an \`action\` for route "${routeId}", ` + + `so there is no way to handle the request.`; + } else { + errorMessage = + `You made a ${method} request to "${pathname}" but ` + + `did not provide a \`loader\` for route "${routeId}", ` + + `so there is no way to handle the request.`; + } + } else { + errorMessage = `Invalid request method "${method}"`; + } + } else { + statusText = "Unknown Server Error"; + errorMessage = "Unknown @remix-run/router error"; + } -function getMethodNotAllowedResult(path: Location | string): ErrorResult { - let href = typeof path === "string" ? path : createPath(path); - console.warn( - "You're trying to submit to a route that does not have an action. To " + - "fix this, please add an `action` function to the route for " + - `[${href}]` + return new ErrorResponse( + status || 500, + statusText, + new Error(errorMessage), + true ); - return { - type: ResultType.error, - error: new ErrorResponse(405, "Method Not Allowed", ""), - }; } // Find any returned redirect errors, starting from the lowest match diff --git a/packages/router/utils.ts b/packages/router/utils.ts index 016e19bf27..5628fd7f45 100644 --- a/packages/router/utils.ts +++ b/packages/router/utils.ts @@ -1243,11 +1243,24 @@ export class ErrorResponse { status: number; statusText: string; data: any; - - constructor(status: number, statusText: string | undefined, data: any) { + error?: Error; + internal: boolean; + + constructor( + status: number, + statusText: string | undefined, + data: any, + internal = false + ) { this.status = status; this.statusText = statusText || ""; - this.data = data; + this.internal = internal; + if (data instanceof Error) { + this.data = data.toString(); + this.error = data; + } else { + this.data = data; + } } }