Always throw error objects instead of strings #412
Conversation
markstos
added
2.0
breaking-change
|
I'm in agreement with this, and agree it would be a "breaking change" for some. @Gekkio Could your review what our docs say about the exceptions we throw and make sure they reflect the change as well? |
|
I agree with this change. I was going to save it a for a bundled "2.0" release, but clearly that approach didn't work. If you'll address the merge conflicts, I'll try to get it out sooner, which may possibly mean moving away from a bundled 2.0 release to a series of breaking change releases. |
While JS *technically* allows code to throw anything (including undefined/null/NaN), it's often considered bad practice.
Gekkio
force-pushed
the
throw-error-objects
branch
from
930c666
to
239a6bf
Compare
|
Sorry, I completely forgot about this pull request 😅 I've rebased the changes on top of current master, so the pull request is now up-to-date. I also looked at the docs and couldn't find a detailed description of thrown errors that would need to be changed, so the docs seem ok to me. |
|
@cjbarth Even though we didn't document the error format returned, I still think we should do a major version bump for this change. Would you agree? |
|
I completely agree. This already has the |
|
We are planning a 2.0 release today, so I'm merging this. |
As stated in https://github.com/node-saml/passport-saml/blob/master/CHANGELOG.md#v200-2020-11-03 and the corresponding PR node-saml/passport-saml#412 passport-saml now always throws error objects instead of strings. This fixes our error logging to accommodate this change. Signed-off-by: David Mehren <[email protected]>
While JS technically allows code to throw anything (including undefined/null/NaN), it's often considered bad practice. It's very annoying to handle/log errors from a library that sometimes throws strings and sometimes objects, and there's also no stack trace in string errors to help with debugging.
I realize this might be a controversial change, and it can break existing error handling code that assumes strings are thrown in some of the cases. But maybe it could be at least considered for 2.0?
More information:
https://eslint.org/docs/rules/no-throw-literal
http://bluebirdjs.com/docs/warning-explanations.html#warning-a-promise-was-rejected-with-a-non-error