Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: invalid deferred data hanging response #6793

Merged
merged 7 commits into from
Jul 11, 2023
Merged

fix: invalid deferred data hanging response #6793

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
37e6ad6
fix: invalid deferred data hanging response
jacob-ebey Jul 7, 2023
bd1819e
better error handling and sanitization of errors in production
jacob-ebey Jul 7, 2023
95bcaaa
Update packages/remix-react/components.tsx
jacob-ebey Jul 10, 2023
3cf80dd
consolidate serialization logic
jacob-ebey Jul 10, 2023
13c8fb7
remove null default and surface error
jacob-ebey Jul 11, 2023
6f7ecb2
update tests
jacob-ebey Jul 11, 2023
4c2cfaa
Merge branch 'dev' into deferred-hang
jacob-ebey Jul 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/deferred-data-hang.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@remix-run/react": patch
---

warn and resolve null for invalid deferred data result to avoid unexpected hanging
49 changes: 32 additions & 17 deletions packages/remix-react/components.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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
Expand Down Expand Up @@ -981,32 +992,36 @@ 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(
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;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we could make this a TS error?

export function loader() {
  return defer({ 
    something: undefined,             // ❌
    lazy: Promise.resolve(undefined), // ❌

    else: null,                   // ✅
    lazy2: Promise.resolve(null), // ✅
  });
}

I didn't dig too deep but it looks like we have:

export type DeferFunction = <Data extends Record<string, unknown>>(
  data: Data, 
  init?: number | ResponseInit
) => TypedDeferredData<Data>;

I wonder if we could enhance that Record to allow something like NonNullable<unknown> | null | Promise<NonNullable<unknown> | null> as a value?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm honestly not sure how unknown would behave in this situation. @pcattori, what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC I got the NonNullable<unknown> | null trick from Pedro for use in RR a while back: https://github.com/remix-run/react-router/blob/main/packages/router/utils.ts#L160

I believe NonNullable<unknown> allows anything except undefined/null and then you can add null back in

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's do that in a followup PR if we want to.

let serializedData = "null";
try {
serializedData = JSON.stringify(data);
} catch (error) {
let toSerialize = serializeErrorImp(
trackedPromise._error
);
jacob-ebey marked this conversation as resolved.
Show resolved Hide resolved
return `${JSON.stringify(
key
)}:__remixContext.p(!1, ${escapeHtml(
JSON.stringify(toSerialize)
)})`;
}
return `${JSON.stringify(
key
)}:__remixContext.p(${escapeHtml(
JSON.stringify(trackedPromise._data)
)})`;
)}:__remixContext.p(${escapeHtml(serializedData)})`;
}
}
})
Expand Down
6 changes: 6 additions & 0 deletions packages/remix-react/entry.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,19 @@ 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;
serverHandoffString?: string;
future: FutureConfig;
abortDelay?: number;
dev?: { port: number };
serializeError?(error: Error): SerializedError;
}

// Additional React-Router information needed at runtime, but not hydrated
Expand Down
1 change: 1 addition & 0 deletions packages/remix-react/server.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ export function RemixServer({
routeModules,
serverHandoffString,
future: context.future,
serializeError: context.serializeError,
abortDelay,
}}
>
Expand Down
2 changes: 2 additions & 0 deletions packages/remix-server-runtime/entry.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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";

Expand All @@ -9,6 +10,7 @@ export interface EntryContext {
serverHandoffString?: string;
staticHandlerContext: StaticHandlerContext;
future: FutureConfig;
serializeError(error: Error): SerializedError;
}

type Dev = {
Expand Down
1 change: 1 addition & 0 deletions packages/remix-server-runtime/server.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -298,6 +298,7 @@ async function handleDocumentRequestRR(
future: build.future,
}),
future: build.future,
serializeError: (err) => serializeError(err, serverMode),
};

let handleDocumentRequestFunction = build.entry.module.default;
Expand Down