-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Conversation
🦋 Changeset detectedLatest commit: acd8f0e The changes in this PR will be included in the next version bump. This PR includes changesets to release 18 packages
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 |
671ffcf
to
469253c
Compare
469253c
to
b5dc636
Compare
@@ -12,193 +13,196 @@ test.describe("headers export", () => { | |||
let appFixture: Fixture; | |||
|
|||
test.beforeAll(async () => { |
There was a problem hiding this comment.
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} |
There was a problem hiding this comment.
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
b5dc636
to
138db33
Compare
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); |
There was a problem hiding this comment.
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
// Log streaming rendering errors from inside the shell | ||
console.error(error); | ||
responseStatusCode = 500; |
There was a problem hiding this comment.
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.
3773f4e
to
c9f9e2e
Compare
c9f9e2e
to
b87dadf
Compare
// 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); | ||
} |
There was a problem hiding this comment.
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)
logServerErrorIfNotAborted(error.error || error, request, serverMode); | ||
if (error.error) { | ||
handleError(error.error); | ||
} |
There was a problem hiding this comment.
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
Object.values(context.errors).forEach((err) => { | ||
if (!isRouteErrorResponse(err) || err.error) { | ||
handleError(err); | ||
} | ||
}); |
There was a problem hiding this comment.
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
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"); | ||
} | ||
} |
There was a problem hiding this comment.
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
let errorHandler = | ||
build.entry.module.onUnhandledError || | ||
((error, { request }) => { | ||
if (serverMode !== ServerMode.Test && !request.signal.aborted) { | ||
console.error(error); | ||
} | ||
}); |
There was a problem hiding this comment.
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
Now that this is merged, what remix version will this be included in? |
🤖 Hello there, We just published version Thanks! |
🤖 Hello there, We just published version Thanks! |
🤖 Hello there, We just published version Thanks! |
This PR does 2 things (broken up by commit for easier reviewing):
handleError
export fromentry.server
(see Server-Side Uncaught Error Handler #2190).handleError
if not provided is our previouslogServerErrorIfNotAborted
which will log in non-test environments and only if the request wasn't abortedhandleError
then Remix will do no logging (except from the express middleware built intoremix-serve
) and it's entirely up to you to log from yourhandleError
method.TODO: