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

Handle non-Error objects thrown as error gracefully, still attempt to report them in a useful way #232

Merged
merged 5 commits into from Mar 4, 2019

Conversation

2 participants
@CherryDT
Copy link
Contributor

CherryDT commented Feb 28, 2019

Some code (unfortunately) throws things that are not Error objects. The things thrown may be raw data returned from an API, plain objects, strings, in the worst case even null or undefined.

While notifyError did already reject things that were not Error objects, the other handlers (uncaught exception, unhandled rejection, Express/Koa middlewares) did not.

I think that
a) The behavior should be the same regardless which handler handles the error
b) Things that are not Error objects should still be reported in the most meaningful way (i.e. giving the developer enough information to understand what the error actually was).

Therefore, I modified the behavior so that things other than Errors are simply converted to an Error with the JSON representation of the old data as message.

Even though the stack of those errors is then meaningless, the JSON data should give the developer enough information, definitely more than not seeing the error at all (old behavior of notifyError), getting no description of the error (old behavior of the other handlers, since err.message may simply be undefined) or crashing in a weird way (old behavior of the other handlers in the case that the error was null or undefined).

@CherryDT CherryDT changed the title Handle non-Error objects thrown as error gracefully, still attempt to report the in a useful way Handle non-Error objects thrown as error gracefully, still attempt to report them in a useful way Feb 28, 2019

@vmarchaud
Copy link
Member

vmarchaud left a comment

Thanks for taking the time to fixing this yourself, overall i agree that it could be useful to be able to notify different kind of objects.
I choose to force it to be an error because we got few case before when error where not saved because of the wrong data structure which is hard to understand for customers generally.

Show resolved Hide resolved src/features/notify.ts Outdated
Show resolved Hide resolved src/features/notify.ts Outdated
Show resolved Hide resolved src/features/notify.ts Outdated
Show resolved Hide resolved src/features/notify.ts Outdated
Show resolved Hide resolved src/features/notify.ts Outdated
Factor out conversion of non-error values to Error object, add length…
… limit for those errors' messages, increase robustness against weird values
Show resolved Hide resolved src/features/notify.ts Outdated
Show resolved Hide resolved src/pmx.ts Outdated

vmarchaud and others added some commits Mar 4, 2019

@vmarchaud vmarchaud merged commit ba45656 into keymetrics:master Mar 4, 2019

1 check passed

codeclimate All good!
Details
@vmarchaud

This comment has been minimized.

Copy link
Member

vmarchaud commented Mar 4, 2019

I will release this tomorrow under the 4.1.0 tag, thanks again for taking the time to change this !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.