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: include error stack in log when handling uncaught exceptions #1417

Merged
merged 2 commits into from Oct 1, 2022
Merged

fix: include error stack in log when handling uncaught exceptions #1417

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
33484d3
feat: include error stack in log when handling uncaught exceptions
jkieboom Sep 26, 2022
a4564c2
Merge branch 'main' into HEAD
kettanaito Oct 1, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/setupWorker/start/createRequestListener.ts
View file
Open in desktop
Expand Up @@ -105,7 +105,7 @@ export const createRequestListener = (
This exception has been gracefully handled as a 500 response, however, it's strongly recommended to resolve this error, as it indicates a mistake in your code. If you wish to mock an error response, please see this guide: https://mswjs.io/docs/recipes/mocking-error-responses`,
request.method,
request.url,
error,
error.stack ?? error,
Copy link
Member

Choose a reason for hiding this comment

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

Would this not print the error if its stack exists?

I thought we may add a new console.error(error) call below this devUtils.error() to forward the error to the console. What do you think about this?

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 prints the error (which will just be name + message) if the stack is undefined. The stack actually also includes the name and message already. Not sure about console.error vs devUtils.error. You have to tell me what is more appropriate. For me it fits will with displaying the error directly within this message and not as a separate disconnected thing on another log.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, so the issue was that at some point there are errors that don't have a stack? And those were somehow ignored?

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 the issue is that Error.toString() only includes the name and message, never the stack. Per spec, stack on Error is optional, so it's best to guard against it being undefined (although I don't know when this could happen in practice)

Copy link
Member

Choose a reason for hiding this comment

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

Got it. Thanks for the explanation!

)

// Treat all other exceptions in a request handler as unintended,
Expand Down
9 changes: 9 additions & 0 deletions test/msw-api/exception-handling.test.ts
View file
Open in desktop
Expand Up @@ -28,4 +28,13 @@ test('transforms uncaught exceptions into a 500 response', async () => {
'ReferenceError: nonExisting is not defined',
),
})

const errors = runtime.consoleSpy.get('error')

expect(errors).toEqual(
expect.arrayContaining([
expect.stringContaining('ReferenceError: nonExisting is not defined'),
expect.stringContaining(' at '),
]),
)
})