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.

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

🗺 Fix server error logging and add onUnhandledError support #6495

Merged
merged 12 commits into from
Jun 1, 2023

Conversation

brophdawg11
Copy link
Contributor

@brophdawg11 brophdawg11 commented May 26, 2023

This PR does 2 things (broken up by commit for easier reviewing):

  • Ensure we're logging unsanitized errors to the server console in produciton - eventhough we only send the sanitized errors to the browser
    • This fixes a regression from the last set of fixes to ensure all code paths would sanitize before hydrating
  • While in this code, I also added support for a handleError export from entry.server (see Server-Side Uncaught Error Handler #2190).
    • The default implementation of handleError if not provided is our previous logServerErrorIfNotAborted which will log in non-test environments and only if the request wasn't aborted
    • This is the only logging we do from the server, so if you provide your own handleError then Remix will do no logging (except from the express middleware built into remix-serve) and it's entirely up to you to log from your handleError method.

TODO:

  • Add Docs

@changeset-bot
Copy link

changeset-bot bot commented May 26, 2023

🦋 Changeset detected

Latest commit: acd8f0e

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 18 packages
Name Type
@remix-run/server-runtime Minor
@remix-run/cloudflare Minor
@remix-run/deno Minor
@remix-run/dev Minor
@remix-run/node Minor
@remix-run/react Minor
@remix-run/cloudflare-pages Minor
@remix-run/cloudflare-workers Minor
create-remix Minor
@remix-run/css-bundle Minor
@remix-run/architect Minor
@remix-run/express Minor
@remix-run/netlify Minor
@remix-run/serve Minor
@remix-run/testing Minor
@remix-run/vercel Minor
remix Minor
@remix-run/eslint-config Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@brophdawg11 brophdawg11 force-pushed the brophdawg11/server-errors branch from 671ffcf to 469253c Compare May 26, 2023 19:44
@brophdawg11 brophdawg11 force-pushed the brophdawg11/server-errors branch from 469253c to b5dc636 Compare May 26, 2023 19:46
@@ -12,193 +13,196 @@ test.describe("headers export", () => {
let appFixture: Fixture;

test.beforeAll(async () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ignore changes in here, I added ServerMode.Test to suppress some thrown response logging, and the rest is whitespace changes for the updated indentation

>
{error.stack}
</pre>
) : null}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No need to include an empty <pre> with padding if we have no stack right? It's just an empty red box on the screen

@brophdawg11 brophdawg11 force-pushed the brophdawg11/server-errors branch from b5dc636 to 138db33 Compare May 26, 2023 19:49
Comment on lines -1747 to +1720
expect(calls.length).toBe(2 * DATA_CALL_MULTIPIER);
expect(spy.console.mock.calls.length).toBe(1 * DATA_CALL_MULTIPIER);
expect(calls.length).toBe(2);
expect(spy.console.mock.calls[0][0].data).toEqual(
'Error: No route matches URL "/"'
);
expect(spy.console.mock.calls[1][0].message).toEqual(
"thrown from handleDocumentRequest and expected to be logged in console only once"
);
expect(spy.console.mock.calls[2][0].message).toEqual(
"second error thrown from handleDocumentRequest"
);
expect(spy.console.mock.calls.length).toBe(3);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This tests gets updated now that we're properly rendering these from the server request handle and not from the render cycle

@brophdawg11 brophdawg11 changed the title Fix server error logging and add handleError support Fix server error logging and add onUnhandledError support May 31, 2023
Comment on lines +17 to 19
// Log streaming rendering errors from inside the shell
console.error(error);
responseStatusCode = 500;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is only async rendering errors, shell rendering errors will cause the promise to reject.

@brophdawg11 brophdawg11 force-pushed the brophdawg11/server-errors branch from 3773f4e to c9f9e2e Compare June 1, 2023 14:32
@brophdawg11 brophdawg11 force-pushed the brophdawg11/server-errors branch from c9f9e2e to b87dadf Compare June 1, 2023 14:33
Comment on lines +67 to +72
// Log streaming rendering errors from inside the shell. Don't log
// errors encountered during initial shell rendering since they'll
// reject and get logged in handleDocumentRequest.
if (shellRendered) {
console.error(error);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only log async rendering errors. Shell-rendering errors will also call this function and then will reject the promise via onShellError and we'll handle logging those in the server (or sending them to onUnhandledError if available)

Comment on lines -163 to +183
logServerErrorIfNotAborted(error.error || error, request, serverMode);
if (error.error) {
handleError(error.error);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

At this point these should always have an error prop since they should all be internal-router-generated errors. User-thrown Responses would have been returned as a plain Response above. But just to be safe, we don't want user-thrown Responses logging

Comment on lines +272 to +276
Object.values(context.errors).forEach((err) => {
if (!isRouteErrorResponse(err) || err.error) {
handleError(err);
}
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Log non-ErrorResponse's as well as internal router-generated ErrorResponse's

Comment on lines 466 to 478
export function onUnhandledError(
error: Error | unknown,
{ request }: { request: Request },
) {
console.error("App Specific Error Logging:");
console.error(" Request: " + request.method + " " + request.url);
if (error instanceof Error) {
console.error(" Error: " + error.message);
console.error(" Stack: " + error.stack);
} else {
console.error("Dunno what this is");
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Custom user-provided logging for tests

Comment on lines +52 to +58
let errorHandler =
build.entry.module.onUnhandledError ||
((error, { request }) => {
if (serverMode !== ServerMode.Test && !request.signal.aborted) {
console.error(error);
}
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

User-logging and built-in-logging are mutually exclusive

@brophdawg11 brophdawg11 merged commit c6d8c37 into dev Jun 1, 2023
@brophdawg11 brophdawg11 deleted the brophdawg11/server-errors branch June 1, 2023 22:12
@quinn
Copy link

quinn commented Jun 1, 2023

Now that this is merged, what remix version will this be included in?

@github-actions
Copy link
Contributor

github-actions bot commented Jun 2, 2023

🤖 Hello there,

We just published version v0.0.0-nightly-70cc627-20230602 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

@github-actions
Copy link
Contributor

github-actions bot commented Jun 2, 2023

🤖 Hello there,

We just published version 1.17.0-pre.0 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

@github-actions
Copy link
Contributor

github-actions bot commented Jun 6, 2023

🤖 Hello there,

We just published version 1.17.0 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

@MichaelDeBoey MichaelDeBoey changed the title Fix server error logging and add onUnhandledError support 🗺 Fix server error logging and add onUnhandledError support Jan 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants