-
Notifications
You must be signed in to change notification settings - Fork 63
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
chore(js): allow uncaughtException plugin to be loaded more than once #1170
Conversation
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 looks good! With this PR, we add support for loading the plugin multiple plugins, but we have yet to reload the plugin when necessary (i.e. when configure
is called more than once), right?
expect(removeLambdaSpy).toHaveBeenCalledTimes(1) | ||
expect(newMonitor.__isReporting).toBe(false) | ||
expect(newMonitor.__handlerAlreadyCalled).toBe(false) | ||
expect(newMonitor.__listener).toStrictEqual(expect.any(Function)) | ||
expect(newMonitor.__listener.name).toBe('honeybadgerUncaughtExceptionListener') |
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.
+1 for named functions!
Correct! There will be a second PR to do this same thing with unhandled rejection (load more than once). Then the 3rd PR will actually reload them when necessary. That way each PR is small and there's only 1 that actually changes the behavior from a user perspective. |
Status
READY
Description
Part of #1163
This PR makes it safe to load the uncaughtException plugin more than once.
However, the client itself still only loads the plugin once, so this PR should not cause any user-facing change in behavior.
Todos
Steps to Test or Reproduce
Unit tests should mostly cover it. I also tested it by throwing an uncaught error in one of the example applications. I chose
sails
, but it could have beenexpress
or another node project. The trick is that you have to throw an error in a node application that is not covered by the honeybadger middleware, but also doesn't cause an import to fail entirely. So for example:Also tested with
enableUncaught: false
, which did not send a report (as expected).I also found that you can't test this in AWS Lambda because the wrapper just wraps the whole thing and calls notify(). So if you throw an uncaught error within your lambda, it’s just going to get reported up to the wrapper.