Fix the Koa plugin suppressing other error handlers #1482
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Koa internally wraps thrown non-Errors in an Error instance, which means we no longer detect a non-Error in Bugsnag This doesn't seem like a problem as it's still clearly not an Error instance originally: > non-error thrown: "<thing that was thrown>"
There's no need to attach metadata if ctx.bugsnag exists as we know we have added an onError callback to record the metadata already This was previously required as the errorHandler and requestHandler were totally independant
We no longer set the status code internally, but Koa does this in its error handler so we don't need to anymore
Koa doesn't add its own error handler if another handler has been added already. We don't want to suppress the default handler just by adding Bugsnag, so we need to manually call the built-in handler However, if another handler has already been added then we don't want to call the default as that is also a change in behaviour
imjoehaines
commented
Jul 20, 2021
Comment on lines
+77
to
+78
| And the exception "errorClass" equals "Error" | ||
| And the exception "message" equals 'non-error thrown: \\"error\\"' |
There was a problem hiding this comment.
As we rely on Koa catching errors now, it will internally wrap them in an Error instance before they reach Bugsnag (see Koa's context.js). This changes the error class and message, but is otherwise equivalent
tomlongridge
requested review from
djskinner
and removed request for
tomlongridge
|
| Minified | Minfied + Gzipped | |
|---|---|---|
| Before | 41.24 kB |
12.71 kB |
| After | 41.24 kB |
12.71 kB |
| ± | No change | No change |
code coverage diff
| Ok | File | Lines | Branches | Functions | Statements |
|---|---|---|---|---|---|
| ✅ | /home/runner/work/bugsnag-js/bugsnag-js/packages/plugin-koa/src/koa.js | 82.05% (+28.05%) |
81.25% (+56.25%) |
71.43% (+14.29%) |
82.05% (+31.14%) |
| ✅ | /home/runner/work/bugsnag-js/bugsnag-js/packages/plugin-koa/src/request-info.js | 100% (+0%) |
78.95% (+5.27%) |
100% (+0%) |
88.89% (+0%) |
Total:
| Lines | Branches | Functions | Statements |
|---|---|---|---|
| 82.58%(+0.33%) | 72.02%(+0.76%) | 83.57%(+0.09%) | 81.56%(+0.37%) |
imjoehaines
requested review from
tomlongridge and
djskinner
and removed request for
djskinner and
tomlongridge
imjoehaines
force-pushed
the
fix-koa-not-propagating-errors
branch
from
1fc4c0d
to
82be592
Compare
imjoehaines
force-pushed
the
fix-koa-not-propagating-errors
branch
from
82be592
to
febca91
Compare
djskinner
approved these changes
Jul 20, 2021
imjoehaines
force-pushed
the
fix-koa-not-propagating-errors
branch
from
a70c229
to
8cdad3d
Compare
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Goal
The Koa plugin currently works by catching exceptions in the request handler and notifying Bugsnag. This works but prevents Koa from detecting that there was an error and so it doesn't trigger error handlers to run. This also means that Bugsnag's error handler will never be called
This PR overhauls the plugin so that it works primarily through the error handler, not the request handler. The request handler still exists to attach request data as metadata and create
ctx.bugsnag, but does nothing else. The error handler will then be triggered by Koa when an error is thrown and Koa will then ensure all other error handlers also runThe error handler is now more complicated as logic from the request handler has been moved into it. Specifically:
Finally we no longer set the status code ourselves as Koa's default error handling does that already. Assertions against the status code have been added to the MazeRunner tests
Testing
Unit tests have been added for most functionality (where it makes sense) and the Maze Runner tests have been updated to ensure other error handlers run