-
Notifications
You must be signed in to change notification settings - Fork 252
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
Remove misleading type definition #1713
base: next
Are you sure you want to change the base?
Conversation
Hey @adkenyon, I do agree, an When bugsnag-js/packages/core/client.js Line 286 in 58c0ade
When creating the event we make a call to normalize the "error": bugsnag-js/packages/core/event.js Line 163 in 8bae662
bugsnag-js/packages/core/event.js Lines 193 to 265 in 8bae662
If we made the change you're suggesting wouldn't break, but it would limit functionality for anyone using TS. In your case, where you're seeing |
@xander-jones thanks for the quick response! One point I'd like to convey is the frustration this type definition continues to cause us. Passing an I see your point on that this may limit functionality, but I'm not sure the functionality is desired? We've never found it useful or actionable |
Hey – thanks for the added context! This has struck up a bit of a discussion internally. Some more thought is needed on how we'd like to proceed on this, so at the moment we won't be accepting the PR directly, but I'll leave this open for future updates on this area. |
@xljones no worries! Thanks for considering it and happy to spur some discussion. This is all purely my team's experience and perspective. Understand ya'll have many more customers, perspectives, and tasks. |
Hi @adkenyon. Could you confirm that you simply passed called |
Goal
The type definition for
client.notify
method implies that several types are acceptable. Which isn't exactly true. When using anError
typeclient.notify
works as expected. When using astring
type, the error reported isnotify() received a non-error. See "notify()" tab for more detail.
while the error details are on thenotify
tab. This is confusing and does not work as expected.Design
I removed the
NotifiableError
type in favor ofError
. This is purely a type definition change with no change in functionality.Changeset
The
NotifiableError
type was removed in favor ofError
.Testing
I'm having issues building/testing the repo as is. I was able to run the typescript check command with 400+ errors. After my changes, the typescript check did not register any new errors.