From 222a9a27bf1b9f7dd2bc10f61d57dff2d7567fc1 Mon Sep 17 00:00:00 2001 From: Pedro Cattori Date: Wed, 4 Sep 2024 14:58:30 -0400 Subject: [PATCH 1/5] fix linting & deprecation warnings --- packages/react-router-dev/vite/plugin.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/react-router-dev/vite/plugin.ts b/packages/react-router-dev/vite/plugin.ts index 27a6a0afe8..75bea67f7e 100644 --- a/packages/react-router-dev/vite/plugin.ts +++ b/packages/react-router-dev/vite/plugin.ts @@ -1263,7 +1263,7 @@ export const reactRouterVitePlugin: ReactRouterVitePlugin = (_config) => { { name: "react-router-route-entry", enforce: "pre", - async transform(code, id, options) { + async transform(_code, id, options) { if (!isRouteEntry(id)) return; let routeModuleId = id.replace(ROUTE_ENTRY_QUERY_STRING, ""); @@ -1586,7 +1586,7 @@ export const reactRouterVitePlugin: ReactRouterVitePlugin = (_config) => { } } - server.ws.send({ + server.hot.send({ type: "custom", event: "react-router:hmr", data: hmrEventData, From b9e98f987786d739b49a8c45b4ee3d0dbed29962 Mon Sep 17 00:00:00 2001 From: Pedro Cattori Date: Wed, 4 Sep 2024 16:24:31 -0400 Subject: [PATCH 2/5] refactor removeExports to be composable with other AST transforms each AST transform should accept an AST and mutate it --- packages/react-router-dev/vite/babel.ts | 6 +- packages/react-router-dev/vite/plugin.ts | 5 +- .../vite/remove-exports-test.ts | 93 ++++++++++--------- .../react-router-dev/vite/remove-exports.ts | 18 ++-- 4 files changed, 63 insertions(+), 59 deletions(-) diff --git a/packages/react-router-dev/vite/babel.ts b/packages/react-router-dev/vite/babel.ts index a075e02836..b4449c65b3 100644 --- a/packages/react-router-dev/vite/babel.ts +++ b/packages/react-router-dev/vite/babel.ts @@ -1,7 +1,7 @@ /* eslint-disable @typescript-eslint/consistent-type-imports */ import type { NodePath } from "@babel/traverse"; -import type { types as BabelTypes } from "@babel/core"; -import { parse } from "@babel/parser"; +import type { types as Babel } from "@babel/core"; +import { parse, type ParseResult } from "@babel/parser"; import * as t from "@babel/types"; // These `require`s were needed to support building within vite-ecosystem-ci, @@ -12,4 +12,4 @@ const generate = require("@babel/generator") .default as typeof import("@babel/generator").default; export { traverse, generate, parse, t }; -export type { BabelTypes, NodePath }; +export type { Babel, NodePath, ParseResult }; diff --git a/packages/react-router-dev/vite/plugin.ts b/packages/react-router-dev/vite/plugin.ts index 75bea67f7e..e6f1b3d394 100644 --- a/packages/react-router-dev/vite/plugin.ts +++ b/packages/react-router-dev/vite/plugin.ts @@ -29,6 +29,7 @@ import colors from "picocolors"; import { type RouteManifestEntry, type RouteManifest } from "../config/routes"; import type { Manifest as ReactRouterManifest } from "../manifest"; import invariant from "../invariant"; +import { generate, parse } from "./babel"; import type { NodeRequestHandler } from "./node-adapter"; import { fromNodeRequest, toNodeRequest } from "./node-adapter"; import { getStylesForUrl, isCssModulesFile } from "./styles"; @@ -1450,7 +1451,9 @@ export const reactRouterVitePlugin: ReactRouterVitePlugin = (_config) => { let [filepath] = id.split("?"); - return removeExports(code, SERVER_ONLY_ROUTE_EXPORTS, { + let ast = parse(code, { sourceType: "module" }); + removeExports(ast, SERVER_ONLY_ROUTE_EXPORTS); + return generate(ast, { sourceMaps: true, filename: id, sourceFileName: filepath, diff --git a/packages/react-router-dev/vite/remove-exports-test.ts b/packages/react-router-dev/vite/remove-exports-test.ts index e5be493d5f..bf83438e53 100644 --- a/packages/react-router-dev/vite/remove-exports-test.ts +++ b/packages/react-router-dev/vite/remove-exports-test.ts @@ -1,8 +1,15 @@ +import { generate, parse } from "./babel"; import { removeExports } from "./remove-exports"; -describe("removeExports", () => { +function transform(code: string, exportsToRemove: string[]) { + let ast = parse(code, { sourceType: "module" }); + removeExports(ast, exportsToRemove); + return generate(ast); +} + +describe("transform", () => { test("arrow function", () => { - let result = removeExports( + let result = transform( ` export const serverExport_1 = () => {} export const serverExport_2 = () => {} @@ -20,7 +27,7 @@ describe("removeExports", () => { }); test("arrow function with dependencies", () => { - let result = removeExports( + let result = transform( ` import { serverLib } from 'server-lib' import { clientLib } from 'client-lib' @@ -52,7 +59,7 @@ describe("removeExports", () => { }); test("function statement", () => { - let result = removeExports( + let result = transform( ` export function serverExport_1(){} export function serverExport_2(){} @@ -70,7 +77,7 @@ describe("removeExports", () => { }); test("function statement with dependencies", () => { - let result = removeExports( + let result = transform( ` import { serverLib } from 'server-lib' import { clientLib } from 'client-lib' @@ -110,7 +117,7 @@ describe("removeExports", () => { }); test("object", () => { - let result = removeExports( + let result = transform( ` export const serverExport_1 = {} export const serverExport_2 = {} @@ -128,7 +135,7 @@ describe("removeExports", () => { }); test("object with dependencies", () => { - let result = removeExports( + let result = transform( ` import { serverLib } from 'server-lib' import { clientLib } from 'client-lib' @@ -164,7 +171,7 @@ describe("removeExports", () => { }); test("class", () => { - let result = removeExports( + let result = transform( ` export class serverExport_1 {} export class serverExport_2 {} @@ -182,7 +189,7 @@ describe("removeExports", () => { }); test("class with dependencies", () => { - let result = removeExports( + let result = transform( ` import { serverLib } from 'server-lib' import { clientLib } from 'client-lib' @@ -226,7 +233,7 @@ describe("removeExports", () => { }); test("function call", () => { - let result = removeExports( + let result = transform( ` export const serverExport_1 = globalFunction() export const serverExport_2 = globalFunction() @@ -244,7 +251,7 @@ describe("removeExports", () => { }); test("function call with dependencies", () => { - let result = removeExports( + let result = transform( ` import { serverLib } from 'server-lib' import { clientLib } from 'client-lib' @@ -276,7 +283,7 @@ describe("removeExports", () => { }); test("iife", () => { - let result = removeExports( + let result = transform( ` export const serverExport_1 = (() => {})() export const serverExport_2 = (() => {})() @@ -294,7 +301,7 @@ describe("removeExports", () => { }); test("iife with dependencies", () => { - let result = removeExports( + let result = transform( ` import { serverLib } from 'server-lib' import { clientLib } from 'client-lib' @@ -326,7 +333,7 @@ describe("removeExports", () => { }); test("aggregated export", () => { - let result = removeExports( + let result = transform( ` const serverExport_1 = 123 const serverExport_2 = 123 @@ -352,7 +359,7 @@ describe("removeExports", () => { }); test("aggregated export multiple", () => { - let result = removeExports( + let result = transform( ` const serverExport_1 = 123 const serverExport_2 = 123 @@ -374,7 +381,7 @@ describe("removeExports", () => { }); test("aggregated export multiple mixed", () => { - let result = removeExports( + let result = transform( ` const removeMe_1 = 123 const removeMe_2 = 123 @@ -397,7 +404,7 @@ describe("removeExports", () => { }); test("re-export", () => { - let result = removeExports( + let result = transform( ` export { serverExport_1 } from './server/1' export { serverExport_2 } from './server/2' @@ -415,7 +422,7 @@ describe("removeExports", () => { }); test("re-export multiple", () => { - let result = removeExports( + let result = transform( ` export { serverExport_1, serverExport_2 } from './server' export { clientExport_1, clientExport_2 } from './client' @@ -429,7 +436,7 @@ describe("removeExports", () => { }); test("re-export multiple mixed", () => { - let result = removeExports( + let result = transform( ` export { removeMe_1, keepMe_1 } from './1' export { removeMe_2, keepMe_2 } from './2' @@ -444,7 +451,7 @@ describe("removeExports", () => { }); test("re-export manual", () => { - let result = removeExports( + let result = transform( ` import { serverExport_1 } from './server/1' import { serverExport_2 } from './server/2' @@ -469,7 +476,7 @@ describe("removeExports", () => { }); test("re-export manual multiple", () => { - let result = removeExports( + let result = transform( ` import { serverExport_1, serverExport_2 } from './server' import { clientExport_1, clientExport_2 } from './client' @@ -487,7 +494,7 @@ describe("removeExports", () => { }); test("re-export manual multiple mixed", () => { - let result = removeExports( + let result = transform( ` import { removeMe_1, keepMe_1 } from './1' import { removeMe_2, keepMe_2 } from './2' @@ -507,7 +514,7 @@ describe("removeExports", () => { }); test("number", () => { - let result = removeExports( + let result = transform( ` export const serverExport_1 = 123 export const serverExport_2 = 123 @@ -525,7 +532,7 @@ describe("removeExports", () => { }); test("string", () => { - let result = removeExports( + let result = transform( ` export const serverExport_1 = 'string' export const serverExport_2 = 'string' @@ -543,7 +550,7 @@ describe("removeExports", () => { }); test("string reference", () => { - let result = removeExports( + let result = transform( ` const SERVER_STRING = 'SERVER_STRING'; const CLIENT_STRING = 'CLIENT_STRING'; @@ -565,7 +572,7 @@ describe("removeExports", () => { }); test("null", () => { - let result = removeExports( + let result = transform( ` export const serverExport_1 = null export const serverExport_2 = null @@ -583,7 +590,7 @@ describe("removeExports", () => { }); test("multiple variable declarators", () => { - let result = removeExports( + let result = transform( ` export const serverExport_1 = null, serverExport_2 = null @@ -601,11 +608,11 @@ describe("removeExports", () => { }); test("multiple variable declarators mixed", () => { - let result = removeExports( + let result = transform( ` export const serverExport_1 = null, clientExport_1 = null - + export const clientExport_2 = null, serverExport_2 = null `, @@ -620,7 +627,7 @@ describe("removeExports", () => { test("array destructuring throws on removed export", () => { expect(() => - removeExports( + transform( ` export const [serverExport_1, serverExport_2] = [null, null] @@ -635,7 +642,7 @@ describe("removeExports", () => { test("array rest destructuring throws on removed export", () => { expect(() => - removeExports( + transform( ` export const [...serverExport_1] = [null, null] @@ -650,7 +657,7 @@ describe("removeExports", () => { test("nested array destructuring throws on removed export", () => { expect(() => - removeExports( + transform( ` export const [keepMe_1, [{ nested: [ { nested: [serverExport_2] } ] }] ] = nested; `, @@ -662,7 +669,7 @@ describe("removeExports", () => { }); test("array destructuring works when nothing is removed", () => { - let result = removeExports( + let result = transform( ` export const [clientExport_1, clientExport_2] = [null, null] `, @@ -676,7 +683,7 @@ describe("removeExports", () => { test("object destructuring throws on removed export", () => { expect(() => - removeExports( + transform( ` export const { serverExport_1, serverExport_2 } = {} @@ -691,7 +698,7 @@ describe("removeExports", () => { test("object rest destructuring throws on removed export", () => { expect(() => - removeExports( + transform( ` export const { ...serverExport_1 } = {} @@ -706,7 +713,7 @@ describe("removeExports", () => { test("nested object destructuring throws on removed export", () => { expect(() => - removeExports( + transform( ` export const [keepMe_1, [{ nested: [ { nested: { serverExport_2 } } ] }]] = nested; `, @@ -718,7 +725,7 @@ describe("removeExports", () => { }); test("object destructuring works when nothing is removed", () => { - let result = removeExports( + let result = transform( ` export const { clientExport_1, clientExport_2 } = {} `, @@ -734,7 +741,7 @@ describe("removeExports", () => { }); test("default export", () => { - let result = removeExports( + let result = transform( ` export const keepMe = null; @@ -749,7 +756,7 @@ describe("removeExports", () => { }); test("default export aggregated", () => { - let result = removeExports( + let result = transform( ` export const keepMe = null; @@ -764,7 +771,7 @@ describe("removeExports", () => { }); test("default export aggregated mixed", () => { - let result = removeExports( + let result = transform( ` const keepMe = null; @@ -782,7 +789,7 @@ describe("removeExports", () => { }); test("default re-export", () => { - let result = removeExports( + let result = transform( ` export const keepMe = null; @@ -795,7 +802,7 @@ describe("removeExports", () => { }); test("default re-export mixed", () => { - let result = removeExports( + let result = transform( ` export { default, keepMe } from "./module"; `, @@ -808,7 +815,7 @@ describe("removeExports", () => { }); test("nothing removed", () => { - let result = removeExports( + let result = transform( ` export const clientExport_1 = () => {} export const clientExport_2 = () => {} diff --git a/packages/react-router-dev/vite/remove-exports.ts b/packages/react-router-dev/vite/remove-exports.ts index 7859134d76..342fb1178c 100644 --- a/packages/react-router-dev/vite/remove-exports.ts +++ b/packages/react-router-dev/vite/remove-exports.ts @@ -1,22 +1,18 @@ -import type { GeneratorOptions } from "@babel/generator"; import { findReferencedIdentifiers, deadCodeElimination, } from "babel-dead-code-elimination"; -import type { BabelTypes, NodePath } from "./babel"; -import { parse, traverse, generate } from "./babel"; +import type { Babel, NodePath, ParseResult } from "./babel"; +import { traverse } from "./babel"; export const removeExports = ( - source: string, - exportsToRemove: string[], - generateOptions: GeneratorOptions = {} + ast: ParseResult, + exportsToRemove: string[] ) => { - let ast = parse(source, { sourceType: "module" }); - let previouslyReferencedIdentifiers = findReferencedIdentifiers(ast); let exportsFiltered = false; - let markedForRemoval = new Set>(); + let markedForRemoval = new Set>(); traverse(ast, { ExportDeclaration(path) { @@ -116,12 +112,10 @@ export const removeExports = ( // Run dead code elimination on any newly unreferenced identifiers deadCodeElimination(ast, previouslyReferencedIdentifiers); } - - return generate(ast, generateOptions); }; function validateDestructuredExports( - id: BabelTypes.ArrayPattern | BabelTypes.ObjectPattern, + id: Babel.ArrayPattern | Babel.ObjectPattern, exportsToRemove: string[] ) { if (id.type === "ArrayPattern") { From 444de15bc91ef3bc6832f2372e5ed3468f55ab64 Mon Sep 17 00:00:00 2001 From: Pedro Cattori Date: Wed, 4 Sep 2024 16:25:15 -0400 Subject: [PATCH 3/5] hoc injection for route component exports --- packages/react-router-dev/vite/plugin.ts | 2 + packages/react-router-dev/vite/with-props.ts | 89 ++++++++++++++++++++ packages/react-router/index.ts | 5 ++ packages/react-router/lib/hocs.tsx | 47 +++++++++++ 4 files changed, 143 insertions(+) create mode 100644 packages/react-router-dev/vite/with-props.ts create mode 100644 packages/react-router/lib/hocs.tsx diff --git a/packages/react-router-dev/vite/plugin.ts b/packages/react-router-dev/vite/plugin.ts index e6f1b3d394..3b6f7a4211 100644 --- a/packages/react-router-dev/vite/plugin.ts +++ b/packages/react-router-dev/vite/plugin.ts @@ -45,6 +45,7 @@ import { resolveEntryFiles, resolvePublicPath, } from "./config"; +import { withProps } from "./with-props"; export async function resolveViteConfig({ configFile, @@ -1453,6 +1454,7 @@ export const reactRouterVitePlugin: ReactRouterVitePlugin = (_config) => { let ast = parse(code, { sourceType: "module" }); removeExports(ast, SERVER_ONLY_ROUTE_EXPORTS); + withProps(ast); return generate(ast, { sourceMaps: true, filename: id, diff --git a/packages/react-router-dev/vite/with-props.ts b/packages/react-router-dev/vite/with-props.ts new file mode 100644 index 0000000000..5825dcac87 --- /dev/null +++ b/packages/react-router-dev/vite/with-props.ts @@ -0,0 +1,89 @@ +import type { Babel, NodePath, ParseResult } from "./babel"; +import { traverse, t } from "./babel"; + +const NAMED_COMPONENT_EXPORTS = ["HydrateFallback", "ErrorBoundary"]; + +export const withProps = (ast: ParseResult) => { + const hocs: Array<[string, Babel.Identifier]> = []; + function getHocUid(path: NodePath, hocName: string) { + const uid = path.scope.generateUidIdentifier(hocName); + hocs.push([hocName, uid]); + return uid; + } + + traverse(ast, { + ExportDeclaration(path) { + if (path.isExportDefaultDeclaration()) { + const declaration = path.get("declaration"); + // prettier-ignore + const expr = + declaration.isExpression() ? declaration.node : + declaration.isFunctionDeclaration() ? toFunctionExpression(declaration.node) : + undefined + if (expr) { + const uid = getHocUid(path, "withComponentProps"); + declaration.replaceWith(t.callExpression(uid, [expr])); + } + return; + } + + if (path.isExportNamedDeclaration()) { + const decl = path.get("declaration"); + + if (decl.isVariableDeclaration()) { + decl.get("declarations").forEach((varDeclarator) => { + const id = varDeclarator.get("id"); + const init = varDeclarator.get("init"); + const expr = init.node; + if (!expr) return; + if (!id.isIdentifier()) return; + const { name } = id.node; + if (!NAMED_COMPONENT_EXPORTS.includes(name)) return; + + const uid = getHocUid(path, `with${name}Props`); + init.replaceWith(t.callExpression(uid, [expr])); + }); + return; + } + + if (decl.isFunctionDeclaration()) { + const { id } = decl.node; + if (!id) return; + const { name } = id; + if (!NAMED_COMPONENT_EXPORTS.includes(name)) return; + + const uid = getHocUid(path, `with${name}Props`); + decl.replaceWith( + t.variableDeclaration("const", [ + t.variableDeclarator( + t.identifier(name), + t.callExpression(uid, [toFunctionExpression(decl.node)]) + ), + ]) + ); + } + } + }, + }); + + if (hocs.length > 0) { + ast.program.body.unshift( + t.importDeclaration( + hocs.map(([name, identifier]) => + t.importSpecifier(identifier, t.identifier(name)) + ), + t.stringLiteral("react-router") + ) + ); + } +}; + +function toFunctionExpression(decl: Babel.FunctionDeclaration) { + return t.functionExpression( + decl.id, + decl.params, + decl.body, + decl.generator, + decl.async + ); +} diff --git a/packages/react-router/index.ts b/packages/react-router/index.ts index 22a44815f8..ef44be1a93 100644 --- a/packages/react-router/index.ts +++ b/packages/react-router/index.ts @@ -133,6 +133,11 @@ export { useRouteLoaderData, useRoutes, } from "./lib/hooks"; +export { + withComponentProps, + withHydrateFallbackProps, + withErrorBoundaryProps, +} from "./lib/hocs"; // Expose old RR DOM API export type { diff --git a/packages/react-router/lib/hocs.tsx b/packages/react-router/lib/hocs.tsx new file mode 100644 index 0000000000..11d27a85a3 --- /dev/null +++ b/packages/react-router/lib/hocs.tsx @@ -0,0 +1,47 @@ +import * as React from "react"; +import { useActionData, useLoaderData, useParams } from "./hooks"; + +export function withComponentProps( + Component: React.ComponentType<{ + params: unknown; + loaderData: unknown; + actionData: unknown; + }> +) { + return function () { + const props = { + params: useParams(), + loaderData: useLoaderData(), + actionData: useActionData(), + }; + return ; + }; +} + +export function withHydrateFallbackProps( + HydrateFallback: React.ComponentType<{ params: unknown }> +) { + return function () { + const props = { + params: useParams(), + }; + return ; + }; +} + +export function withErrorBoundaryProps( + ErrorBoundary: React.ComponentType<{ + params: unknown; + loaderData: unknown; + actionData: unknown; + }> +) { + return function () { + const props = { + params: useParams(), + loaderData: useLoaderData(), + actionData: useActionData(), + }; + return ; + }; +} From af9ffe7e9dd8a48a1bce38470d7c378b3182d2bb Mon Sep 17 00:00:00 2001 From: Pedro Cattori Date: Wed, 4 Sep 2024 20:16:07 -0400 Subject: [PATCH 4/5] add names for hoc wrappers to enable HMR --- packages/react-router/lib/hocs.tsx | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/react-router/lib/hocs.tsx b/packages/react-router/lib/hocs.tsx index 11d27a85a3..582752cd7a 100644 --- a/packages/react-router/lib/hocs.tsx +++ b/packages/react-router/lib/hocs.tsx @@ -8,7 +8,7 @@ export function withComponentProps( actionData: unknown; }> ) { - return function () { + return function Wrapped() { const props = { params: useParams(), loaderData: useLoaderData(), @@ -21,7 +21,7 @@ export function withComponentProps( export function withHydrateFallbackProps( HydrateFallback: React.ComponentType<{ params: unknown }> ) { - return function () { + return function Wrapped() { const props = { params: useParams(), }; @@ -36,7 +36,7 @@ export function withErrorBoundaryProps( actionData: unknown; }> ) { - return function () { + return function Wrapped() { const props = { params: useParams(), loaderData: useLoaderData(), From b81f12102a9b2f9c948cb59eda7ca7c260b53c7f Mon Sep 17 00:00:00 2001 From: Pedro Cattori Date: Thu, 5 Sep 2024 11:15:34 -0400 Subject: [PATCH 5/5] do not error when useLoaderData is used within error boundaries --- integration/error-boundary-test.ts | 250 ------------------ .../__tests__/data-memory-router-test.tsx | 175 ------------ packages/react-router/lib/hooks.tsx | 7 - 3 files changed, 432 deletions(-) diff --git a/integration/error-boundary-test.ts b/integration/error-boundary-test.ts index a71a3a38b3..fbe134442b 100644 --- a/integration/error-boundary-test.ts +++ b/integration/error-boundary-test.ts @@ -655,256 +655,6 @@ test.describe("ErrorBoundary", () => { }); }); -test.describe("loaderData in ErrorBoundary", () => { - let fixture: Fixture; - let appFixture: AppFixture; - let consoleErrors: string[]; - let oldConsoleError: () => void; - - test.beforeAll(async () => { - fixture = await createFixture({ - files: { - "app/root.tsx": js` - import { Links, Meta, Outlet, Scripts } from "react-router"; - - export default function Root() { - return ( - - - - - - -
- -
- - - - ); - } - `, - - "app/routes/parent.tsx": js` - import { Outlet, useLoaderData, useMatches, useRouteError } from "react-router"; - - export function loader() { - return "PARENT"; - } - - export default function () { - return ( -
-

{useLoaderData()}

- -
- ) - } - - export function ErrorBoundary() { - let error = useRouteError(); - return ( - <> -

{useLoaderData()}

-

- {useMatches().find(m => m.id === 'routes/parent').data} -

-

{error.message}

- - ); - } - `, - - "app/routes/parent.child-with-boundary.tsx": js` - import { Form, useLoaderData, useRouteError } from "react-router"; - - export function loader() { - return "CHILD"; - } - - export function action() { - throw new Error("Broken!"); - } - - export default function () { - return ( - <> -

{useLoaderData()}

-
- -
- - ) - } - - export function ErrorBoundary() { - let error = useRouteError(); - return ( - <> -

{useLoaderData()}

-

{error.message}

- - ); - } - `, - - "app/routes/parent.child-without-boundary.tsx": js` - import { Form, useLoaderData } from "react-router"; - - export function loader() { - return "CHILD"; - } - - export function action() { - throw new Error("Broken!"); - } - - export default function () { - return ( - <> -

{useLoaderData()}

-
- -
- - ) - } - `, - }, - }); - - appFixture = await createAppFixture(fixture, ServerMode.Development); - }); - - test.afterAll(() => { - appFixture.close(); - }); - - test.beforeEach(({ page }) => { - oldConsoleError = console.error; - console.error = () => {}; - consoleErrors = []; - // Listen for all console events and handle errors - page.on("console", (msg) => { - if (msg.type() === "error") { - consoleErrors.push(msg.text()); - } - }); - }); - - test.afterEach(() => { - console.error = oldConsoleError; - }); - - test.describe("without JavaScript", () => { - test.use({ javaScriptEnabled: false }); - runBoundaryTests(); - }); - - test.describe("with JavaScript", () => { - test.use({ javaScriptEnabled: true }); - runBoundaryTests(); - }); - - function runBoundaryTests() { - test("Prevents useLoaderData in self ErrorBoundary", async ({ - page, - javaScriptEnabled, - }) => { - let app = new PlaywrightFixture(appFixture, page); - await app.goto("/parent/child-with-boundary"); - - expect(await app.getHtml("#parent-data")).toEqual( - '

PARENT

' - ); - expect(await app.getHtml("#child-data")).toEqual( - '

CHILD

' - ); - expect(consoleErrors).toEqual([]); - - await app.clickSubmitButton("/parent/child-with-boundary"); - await page.waitForSelector("#child-error"); - - expect(await app.getHtml("#child-error")).toEqual( - '

Broken!

' - ); - expect(await app.getHtml("#parent-data")).toEqual( - '

PARENT

' - ); - expect(await app.getHtml("#child-data")).toEqual( - '

' - ); - - // Only look for this message. Chromium browsers will also log the - // network error but firefox does not - // "Failed to load resource: the server responded with a status of 500 (Internal Server Error)", - let msg = - "You cannot `useLoaderData` in an errorElement (routeId: routes/parent.child-with-boundary)"; - if (javaScriptEnabled) { - expect(consoleErrors.filter((m) => m === msg)).toEqual([msg]); - } else { - // We don't get the useLoaderData message in the client when JS is disabled - expect(consoleErrors.filter((m) => m === msg)).toEqual([]); - } - }); - - test("Prevents useLoaderData in bubbled ErrorBoundary", async ({ - page, - javaScriptEnabled, - }) => { - let app = new PlaywrightFixture(appFixture, page); - await app.goto("/parent/child-without-boundary"); - - expect(await app.getHtml("#parent-data")).toEqual( - '

PARENT

' - ); - expect(await app.getHtml("#child-data")).toEqual( - '

CHILD

' - ); - expect(consoleErrors).toEqual([]); - - await app.clickSubmitButton("/parent/child-without-boundary"); - await page.waitForSelector("#parent-error"); - - expect(await app.getHtml("#parent-error")).toEqual( - '

Broken!

' - ); - if (javaScriptEnabled) { - // This data remains in single fetch with JS because we don't revalidate - // due to the 500 action response - expect(await app.getHtml("#parent-matches-data")).toEqual( - '

PARENT

' - ); - } else { - // But without JS document requests call all loaders up to the - // boundary route so parent's data clears out - expect(await app.getHtml("#parent-matches-data")).toEqual( - '

' - ); - } - expect(await app.getHtml("#parent-data")).toEqual( - '

' - ); - - // Only look for this message. Chromium browsers will also log the - // network error but firefox does not - // "Failed to load resource: the server responded with a status of 500 (Internal Server Error)", - let msg = - "You cannot `useLoaderData` in an errorElement (routeId: routes/parent)"; - if (javaScriptEnabled) { - expect(consoleErrors.filter((m) => m === msg)).toEqual([msg]); - } else { - // We don't get the useLoaderData message in the client when JS is disabled - expect(consoleErrors.filter((m) => m === msg)).toEqual([]); - } - }); - } -}); - test.describe("Default ErrorBoundary", () => { let fixture: Fixture; let appFixture: AppFixture; diff --git a/packages/react-router/__tests__/data-memory-router-test.tsx b/packages/react-router/__tests__/data-memory-router-test.tsx index c1d807d97e..3375d47fde 100644 --- a/packages/react-router/__tests__/data-memory-router-test.tsx +++ b/packages/react-router/__tests__/data-memory-router-test.tsx @@ -2476,181 +2476,6 @@ describe("createMemoryRouter", () => { " `); }); - - it("does not allow loaderData usage in self-caught error boundaries", async () => { - let errorSpy = jest.spyOn(console, "error"); - - let router = createMemoryRouter( - createRoutesFromElements( - }> - Promise.reject(new Error("Kaboom!"))} - element={

Foo

} - errorElement={} - /> - - ) - ); - let { container } = render(); - - function Layout() { - let navigation = useNavigation(); - return ( -
- Link to Foo -

{navigation.state}

- -
- ); - } - - function FooError() { - let error = useRouteError() as Error; - let data = useLoaderData() as { message: string }; - return ( - <> -

- Foo Data:{data === undefined ? "undefined" : JSON.stringify(data)} -

-

Foo Error:{error.message}

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

- idle -

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

- idle -

-

- Foo Data: - undefined -

-

- Foo Error: - Kaboom! -

-
-
" - `); - - expect(errorSpy).toHaveBeenCalledWith( - "You cannot `useLoaderData` in an errorElement (routeId: 0-0)" - ); - errorSpy.mockRestore(); - }); - - it("does not allow useLoaderData usage in bubbled error boundaries", async () => { - let errorSpy = jest.spyOn(console, "error"); - - let router = createMemoryRouter( - createRoutesFromElements( - } - loader={() => "ROOT"} - errorElement={} - > - Promise.reject(new Error("Kaboom!"))} - element={

Foo

} - /> - - ), - { - hydrationData: { - loaderData: { - "0": "ROOT", - }, - }, - } - ); - let { container } = render(); - - function Layout() { - let navigation = useNavigation(); - return ( -
- Link to Foo -

{navigation.state}

- -
- ); - } - function LayoutError() { - let data = useLoaderData() as { message: string }; - let error = useRouteError() as Error; - return ( - <> -

- Layout Data: - {data === undefined ? "undefined" : JSON.stringify(data)} -

-

Layout Error:{error.message}

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

- idle -

-
-
" - `); - - fireEvent.click(screen.getByText("Link to Foo")); - await waitFor(() => screen.getByText("Layout Error:Kaboom!")); - expect(getHtml(container)).toMatchInlineSnapshot(` - "
-

- Layout Data: - undefined -

-

- Layout Error: - Kaboom! -

-
" - `); - - expect(errorSpy).toHaveBeenCalledWith( - "You cannot `useLoaderData` in an errorElement (routeId: 0)" - ); - errorSpy.mockRestore(); - }); }); describe("defer", () => { diff --git a/packages/react-router/lib/hooks.tsx b/packages/react-router/lib/hooks.tsx index f6adc4ab69..dab9f51569 100644 --- a/packages/react-router/lib/hooks.tsx +++ b/packages/react-router/lib/hooks.tsx @@ -1085,13 +1085,6 @@ export function useMatches(): UIMatch[] { export function useLoaderData(): unknown { let state = useDataRouterState(DataRouterStateHook.UseLoaderData); let routeId = useCurrentRouteId(DataRouterStateHook.UseLoaderData); - - if (state.errors && state.errors[routeId] != null) { - console.error( - `You cannot \`useLoaderData\` in an errorElement (routeId: ${routeId})` - ); - return undefined; - } return state.loaderData[routeId]; }