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

Explicitly log errors for Flight onError #1320

Merged
merged 5 commits into from
May 27, 2022
Merged

Explicitly log errors for Flight onError #1320

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a1e7181
wip: Explicitly log errors for Flight onError
jplhomer May 19, 2022
5d6a75e
Merge remote-tracking branch 'origin/v1.x-2022-07' into jl-server-errors
jplhomer May 25, 2022
5268fd6
Rename variables for clarity
jplhomer May 25, 2022
7a97dd5
Add changeset
jplhomer May 25, 2022
15871f4
Revert streaming thing
jplhomer May 25, 2022
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/twenty-meals-smile.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@shopify/hydrogen': patch
---

Properly log errors during flight responses
1 change: 1 addition & 0 deletions .eslintrc.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ module.exports = {
ignorePatterns: [
'**/storefront-api-types.ts',
'**/storefront-api-types.d.ts',
'examples/**',
],
root: true,
plugins: ['eslint-plugin-tsdoc'],
Expand Down
64 changes: 48 additions & 16 deletions packages/hydrogen/src/entry-server.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -299,7 +299,7 @@ async function stream(
const {noScriptTemplate, bootstrapScripts, bootstrapModules} =
stripScriptsFromTemplate(template);

const {AppSSR, rscReadable} = buildAppSSR(
const {AppSSR, rscReadable, rscDidError} = buildAppSSR(
{
App,
log,
Expand All @@ -324,7 +324,7 @@ async function stream(
},
});

let didError: Error | undefined;
let ssrDidError: Error | undefined;

if (__WORKER__) {
const onCompleteAll = defer<true>();
Expand All @@ -341,7 +341,7 @@ async function stream(
bootstrapScripts,
bootstrapModules,
onError(error) {
didError = error;
ssrDidError = error;

if (dev && !writable.closed && !!responseOptions.status) {
writable.write(getErrorMarkup(error));
Expand Down Expand Up @@ -373,7 +373,7 @@ async function stream(
function prepareForStreaming(flush: boolean) {
Object.assign(
responseOptions,
getResponseOptions(componentResponse, didError)
getResponseOptions(componentResponse, rscDidError ?? ssrDidError)
);

/**
Expand All @@ -394,9 +394,13 @@ async function stream(
responseOptions.headers.set(CONTENT_TYPE, HTML_CONTENT_TYPE);
writable.write(encoder.encode(DOCTYPE));

if (didError) {
if (rscDidError ?? ssrDidError) {
// This error was delayed until the headers were properly sent.
writable.write(encoder.encode(getErrorMarkup(didError)));
writable.write(
encoder.encode(
getErrorMarkup((rscDidError ?? ssrDidError) as Error)
)
);
}

return true;
Expand Down Expand Up @@ -483,7 +487,12 @@ async function stream(
componentResponse.cacheControlHeader
);

writeHeadToServerResponse(response, componentResponse, log, didError);
writeHeadToServerResponse(
response,
componentResponse,
log,
rscDidError ?? ssrDidError
);

if (isRedirect(response)) {
// Return redirects early without further rendering/streaming
Expand All @@ -492,7 +501,10 @@ async function stream(

if (!componentResponse.canStream()) return;

startWritingHtmlToServerResponse(response, dev ? didError : undefined);
startWritingHtmlToServerResponse(
response,
dev ? rscDidError ?? ssrDidError : undefined
);

setTimeout(() => {
log.trace('node pipe response');
Expand All @@ -517,7 +529,12 @@ async function stream(
return;
}

writeHeadToServerResponse(response, componentResponse, log, didError);
writeHeadToServerResponse(
response,
componentResponse,
log,
rscDidError ?? ssrDidError
);

postRequestTasks(
'str',
Expand All @@ -531,7 +548,10 @@ async function stream(
return response.end();
}

startWritingHtmlToServerResponse(response, dev ? didError : undefined);
startWritingHtmlToServerResponse(
response,
dev ? rscDidError ?? ssrDidError : undefined
);

bufferReadableStream(rscToScriptTagReadable.getReader()).then(
(scriptTags) => {
Expand All @@ -554,7 +574,7 @@ async function stream(
}
},
onError(error: any) {
didError = error;
ssrDidError = error;

if (dev && response.headersSent) {
// Calling write would flush headers automatically.
Expand Down Expand Up @@ -592,9 +612,12 @@ async function hydrate(
response: componentResponse,
});

const rscReadable = rscRenderToReadableStream(AppRSC);
const rscReadable = rscRenderToReadableStream(AppRSC, {
onError(e) {
log.error(e);
Comment on lines +616 to +617
Copy link
Contributor

Choose a reason for hiding this comment

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

should rscDidError = e be here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Other places it would be, but we don't use that to change behavior of our outgoing flight response in this case (always 200, error gets converted to @E, etc).

},
});

// Note: CFW does not support reader.piteTo nor iterable syntax
const bufferedBody = await bufferReadableStream(rscReadable.getReader());

postRequestTasks('rsc', 200, request, componentResponse);
Expand Down Expand Up @@ -656,8 +679,17 @@ function buildAppSSR(
response,
});

const [rscReadableForFizz, rscReadableForFlight] =
rscRenderToReadableStream(AppRSC).tee();
let rscDidError;

const [rscReadableForFizz, rscReadableForFlight] = rscRenderToReadableStream(
AppRSC,
{
onError(e) {
rscDidError = e;
log.error(e);
},
}
).tee();

const rscResponse = createFromReadableStream(rscReadableForFizz);
const RscConsumer = () => rscResponse.readRoot();
Expand All @@ -682,7 +714,7 @@ function buildAppSSR(
</Html>
);

return {AppSSR, rscReadable: rscReadableForFlight};
return {AppSSR, rscReadable: rscReadableForFlight, rscDidError};
}

function PreloadQueries({
Expand Down
5 changes: 4 additions & 1 deletion packages/hydrogen/src/streaming.server.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,10 @@ import {createFromReadableStream as _createFromReadableStream} from '@shopify/hy
import type {Writable} from 'stream';

export const rscRenderToReadableStream = _rscRenderToReadableStream as (
App: JSX.Element
App: JSX.Element,
options?: {
onError?: (error: Error) => void;
}
) => ReadableStream<Uint8Array>;

export const createFromReadableStream = _createFromReadableStream as (
Expand Down