From 37e6ad69d2054b6b51d1e26e7a29a95b7719fbc9 Mon Sep 17 00:00:00 2001 From: Jacob Ebey Date: Fri, 7 Jul 2023 12:06:11 -0700 Subject: [PATCH 1/6] fix: invalid deferred data hanging response --- .changeset/deferred-data-hang.md | 5 +++++ packages/remix-react/components.tsx | 18 ++++++++++++------ 2 files changed, 17 insertions(+), 6 deletions(-) create mode 100644 .changeset/deferred-data-hang.md diff --git a/.changeset/deferred-data-hang.md b/.changeset/deferred-data-hang.md new file mode 100644 index 00000000000..06027576f2e --- /dev/null +++ b/.changeset/deferred-data-hang.md @@ -0,0 +1,5 @@ +--- +"@remix-run/react": patch +--- + +warn and resolve null for invalid deferred data result to avoid unexpected hanging diff --git a/packages/remix-react/components.tsx b/packages/remix-react/components.tsx index 039d8211f46..25a1d3fd1d4 100644 --- a/packages/remix-react/components.tsx +++ b/packages/remix-react/components.tsx @@ -997,16 +997,22 @@ export function Scripts(props: ScriptProps) { JSON.stringify(toSerialize) )})`; } else { - if (typeof trackedPromise._data === "undefined") { - throw new Error( - `The deferred data for ${key} was not resolved, did you forget to return data from a deferred promise?` + let data = trackedPromise._data; + if (typeof data === "undefined") { + console.error( + `Deferred data for ${routeId} ${key} resolved to undefined, defaulting to null.` ); + data = null; + } + let serializedData = "null"; + try { + serializedData = JSON.stringify(data); + } catch (error) { + console.error(error); } return `${JSON.stringify( key - )}:__remixContext.p(${escapeHtml( - JSON.stringify(trackedPromise._data) - )})`; + )}:__remixContext.p(${escapeHtml(serializedData)})`; } } }) From bd1819e8bed3537c4dfd73eaf2c5d1f4800aba25 Mon Sep 17 00:00:00 2001 From: Jacob Ebey Date: Fri, 7 Jul 2023 12:21:02 -0700 Subject: [PATCH 2/6] better error handling and sanitization of errors in production --- packages/remix-react/components.tsx | 33 ++++++++++++++++--------- packages/remix-react/entry.ts | 6 +++++ packages/remix-react/server.tsx | 1 + packages/remix-server-runtime/entry.ts | 2 ++ packages/remix-server-runtime/server.ts | 1 + 5 files changed, 31 insertions(+), 12 deletions(-) diff --git a/packages/remix-react/components.tsx b/packages/remix-react/components.tsx index 25a1d3fd1d4..2aa4591a977 100644 --- a/packages/remix-react/components.tsx +++ b/packages/remix-react/components.tsx @@ -894,7 +894,8 @@ export type ScriptProps = Omit< * @see https://remix.run/components/scripts */ export function Scripts(props: ScriptProps) { - let { manifest, serverHandoffString, abortDelay } = useRemixContext(); + let { manifest, serverHandoffString, abortDelay, serializeError } = + useRemixContext(); let { router, static: isStatic, staticContext } = useDataRouterContext(); let { matches } = useDataRouterStateContext(); let navigation = useNavigation(); @@ -903,6 +904,16 @@ export function Scripts(props: ScriptProps) { isHydrated = true; }, []); + let serializeErrorImp = (error: unknown) => { + let toSerialize: unknown; + if (serializeError && error instanceof Error) { + toSerialize = serializeError(error); + } else { + toSerialize = error; + } + return toSerialize; + }; + let deferredScripts: any[] = []; let initialScripts = React.useMemo(() => { let contextScript = staticContext @@ -981,16 +992,7 @@ export function Scripts(props: ScriptProps) { } else { let trackedPromise = deferredData.data[key] as TrackedPromise; if (typeof trackedPromise._error !== "undefined") { - let toSerialize: { message: string; stack?: string } = - process.env.NODE_ENV === "development" - ? { - message: trackedPromise._error.message, - stack: trackedPromise._error.stack, - } - : { - message: "Unexpected Server Error", - stack: undefined, - }; + let toSerialize = serializeErrorImp(trackedPromise._error); return `${JSON.stringify( key )}:__remixContext.p(!1, ${escapeHtml( @@ -1008,7 +1010,14 @@ export function Scripts(props: ScriptProps) { try { serializedData = JSON.stringify(data); } catch (error) { - console.error(error); + let toSerialize = serializeErrorImp( + trackedPromise._error + ); + return `${JSON.stringify( + key + )}:__remixContext.p(!1, ${escapeHtml( + JSON.stringify(toSerialize) + )})`; } return `${JSON.stringify( key diff --git a/packages/remix-react/entry.ts b/packages/remix-react/entry.ts index c948858b852..2462c016fe7 100644 --- a/packages/remix-react/entry.ts +++ b/packages/remix-react/entry.ts @@ -4,6 +4,11 @@ import type { RouteManifest, EntryRoute } from "./routes"; import type { RouteModules } from "./routeModules"; // Object passed to RemixContext.Provider + +type SerializedError = { + message: string; + stack?: string; +}; export interface RemixContextObject { manifest: AssetsManifest; routeModules: RouteModules; @@ -11,6 +16,7 @@ export interface RemixContextObject { future: FutureConfig; abortDelay?: number; dev?: { port: number }; + serializeError?(error: Error): SerializedError; } // Additional React-Router information needed at runtime, but not hydrated diff --git a/packages/remix-react/server.tsx b/packages/remix-react/server.tsx index 94eefbdbfaf..35d3cbd21f8 100644 --- a/packages/remix-react/server.tsx +++ b/packages/remix-react/server.tsx @@ -48,6 +48,7 @@ export function RemixServer({ routeModules, serverHandoffString, future: context.future, + serializeError: context.serializeError, abortDelay, }} > diff --git a/packages/remix-server-runtime/entry.ts b/packages/remix-server-runtime/entry.ts index 03774f9adb4..b20be17e389 100644 --- a/packages/remix-server-runtime/entry.ts +++ b/packages/remix-server-runtime/entry.ts @@ -1,5 +1,6 @@ import type { StaticHandlerContext } from "@remix-run/router"; +import type { SerializedError } from "./errors"; import type { RouteManifest, ServerRouteManifest, EntryRoute } from "./routes"; import type { RouteModules, EntryRouteModule } from "./routeModules"; @@ -9,6 +10,7 @@ export interface EntryContext { serverHandoffString?: string; staticHandlerContext: StaticHandlerContext; future: FutureConfig; + serializeError(error: Error): SerializedError; } type Dev = { diff --git a/packages/remix-server-runtime/server.ts b/packages/remix-server-runtime/server.ts index eb30cb36f18..37fc6d8c1ef 100644 --- a/packages/remix-server-runtime/server.ts +++ b/packages/remix-server-runtime/server.ts @@ -298,6 +298,7 @@ async function handleDocumentRequestRR( future: build.future, }), future: build.future, + serializeError: (err) => serializeError(err, serverMode), }; let handleDocumentRequestFunction = build.entry.module.default; From 95bcaaac3350d4fb4dcaf0eafa19e255eb816f39 Mon Sep 17 00:00:00 2001 From: Jacob Ebey Date: Mon, 10 Jul 2023 10:12:38 -0700 Subject: [PATCH 3/6] Update packages/remix-react/components.tsx Co-authored-by: Matt Brophy --- packages/remix-react/components.tsx | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/packages/remix-react/components.tsx b/packages/remix-react/components.tsx index 2aa4591a977..3a49fbba93d 100644 --- a/packages/remix-react/components.tsx +++ b/packages/remix-react/components.tsx @@ -1010,9 +1010,7 @@ export function Scripts(props: ScriptProps) { try { serializedData = JSON.stringify(data); } catch (error) { - let toSerialize = serializeErrorImp( - trackedPromise._error - ); + let toSerialize = serializeErrorImp(error); return `${JSON.stringify( key )}:__remixContext.p(!1, ${escapeHtml( From 3cf80dd967660109645edb6ac6259e3df4e3b66c Mon Sep 17 00:00:00 2001 From: Jacob Ebey Date: Mon, 10 Jul 2023 15:38:26 -0700 Subject: [PATCH 4/6] consolidate serialization logic --- integration/defer-test.ts | 6 ++ packages/remix-react/components.tsx | 150 +++++++++++++++++----------- 2 files changed, 99 insertions(+), 57 deletions(-) diff --git a/integration/defer-test.ts b/integration/defer-test.ts index 267ffba5b49..4551ae31f3c 100644 --- a/integration/defer-test.ts +++ b/integration/defer-test.ts @@ -232,6 +232,7 @@ test.describe("non-aborted", () => { return defer({ deferredId: "${DEFERRED_ID}", resolvedId: Promise.resolve("${RESOLVED_DEFERRED_ID}"), + deferredUndefined: Promise.resolve(undefined), }); } @@ -271,6 +272,11 @@ test.describe("non-aborted", () => { resolve("${RESOLVED_DEFERRED_ID}"); }, 10) ), + deferredUndefined: new Promise( + (resolve) => setTimeout(() => { + resolve(undefined); + }, 10) + ), }); } diff --git a/packages/remix-react/components.tsx b/packages/remix-react/components.tsx index 3a49fbba93d..5c615c0f28a 100644 --- a/packages/remix-react/components.tsx +++ b/packages/remix-react/components.tsx @@ -904,14 +904,68 @@ export function Scripts(props: ScriptProps) { isHydrated = true; }, []); - let serializeErrorImp = (error: unknown) => { + let serializePreResolvedErrorImp = (key: string, error: unknown) => { let toSerialize: unknown; if (serializeError && error instanceof Error) { toSerialize = serializeError(error); } else { toSerialize = error; } - return toSerialize; + return `${JSON.stringify(key)}:__remixContext.p(!1, ${escapeHtml( + JSON.stringify(toSerialize) + )})`; + }; + + let serializePreresolvedDataImp = ( + routeId: string, + key: string, + data: unknown + ) => { + if (typeof data === "undefined") { + console.error( + `Deferred data for ${routeId} ${key} resolved to undefined, defaulting to null.` + ); + data = null; + } + let serializedData = "null"; + try { + serializedData = JSON.stringify(data); + } catch (error) { + return serializePreResolvedErrorImp(key, error); + } + return `${JSON.stringify(key)}:__remixContext.p(${escapeHtml( + serializedData + )})`; + }; + + let serializeErrorImp = (routeId: string, key: string, error: unknown) => { + let toSerialize: unknown; + if (serializeError && error instanceof Error) { + toSerialize = serializeError(error); + } else { + toSerialize = error; + } + return `__remixContext.r(${JSON.stringify(routeId)}, ${JSON.stringify( + key + )}, !1, ${escapeHtml(JSON.stringify(toSerialize))})`; + }; + + let serializeDataImp = (routeId: string, key: string, data: unknown) => { + if (typeof data === "undefined") { + console.error( + `Deferred data for ${routeId} ${key} resolved to undefined, defaulting to null.` + ); + data = null; + } + let serializedData = "null"; + try { + serializedData = JSON.stringify(data); + } catch (error) { + return serializeErrorImp(routeId, key, error); + } + return `__remixContext.r(${JSON.stringify(routeId)}, ${JSON.stringify( + key + )}, ${escapeHtml(serializedData)})`; }; let deferredScripts: any[] = []; @@ -981,6 +1035,8 @@ export function Scripts(props: ScriptProps) { routeId={routeId} dataKey={key} scriptProps={props} + serializeData={serializeDataImp} + serializeError={serializeErrorImp} /> ); @@ -992,34 +1048,16 @@ export function Scripts(props: ScriptProps) { } else { let trackedPromise = deferredData.data[key] as TrackedPromise; if (typeof trackedPromise._error !== "undefined") { - let toSerialize = serializeErrorImp(trackedPromise._error); - return `${JSON.stringify( - key - )}:__remixContext.p(!1, ${escapeHtml( - JSON.stringify(toSerialize) - )})`; + return serializePreResolvedErrorImp( + key, + trackedPromise._error + ); } else { - let data = trackedPromise._data; - if (typeof data === "undefined") { - console.error( - `Deferred data for ${routeId} ${key} resolved to undefined, defaulting to null.` - ); - data = null; - } - let serializedData = "null"; - try { - serializedData = JSON.stringify(data); - } catch (error) { - let toSerialize = serializeErrorImp(error); - return `${JSON.stringify( - key - )}:__remixContext.p(!1, ${escapeHtml( - JSON.stringify(toSerialize) - )})`; - } - return `${JSON.stringify( - key - )}:__remixContext.p(${escapeHtml(serializedData)})`; + return serializePreresolvedDataImp( + routeId, + key, + trackedPromise._data + ); } } }) @@ -1082,7 +1120,12 @@ import(${JSON.stringify(manifest.entry.module)});`; if (!isStatic && typeof __remixContext === "object" && __remixContext.a) { for (let i = 0; i < __remixContext.a; i++) { deferredScripts.push( - + ); } } @@ -1138,11 +1181,15 @@ function DeferredHydrationScript({ deferredData, routeId, scriptProps, + serializeData, + serializeError, }: { dataKey?: string; deferredData?: DeferredData; routeId?: string; scriptProps?: ScriptProps; + serializeData: (routeId: string, key: string, data: unknown) => string; + serializeError: (routeId: string, key: string, error: unknown) => string; }) { if (typeof document === "undefined" && deferredData && dataKey && routeId) { invariant( @@ -1178,22 +1225,21 @@ function DeferredHydrationScript({ dataKey={dataKey} routeId={routeId} scriptProps={scriptProps} + serializeError={serializeError} /> } - children={(data) => ( -