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

Remove misleading type definition #1713

Open
wants to merge 1 commit into
base: next
Choose a base branch
from

Conversation

adkenyon
Copy link

@adkenyon adkenyon commented Apr 1, 2022

Goal

The type definition for client.notify method implies that several types are acceptable. Which isn't exactly true. When using an Error type client.notify works as expected. When using a string type, the error reported is notify() received a non-error. See "notify()" tab for more detail. while the error details are on the notify tab. This is confusing and does not work as expected.

Design

I removed the NotifiableError type in favor of Error. This is purely a type definition change with no change in functionality.

Changeset

The NotifiableError type was removed in favor of Error.

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.

@adkenyon adkenyon changed the title Remove incorrect type definition Remove misleading type definition Apr 2, 2022
@xljones
Copy link
Contributor

xljones commented Apr 4, 2022

Hey @adkenyon, I do agree, an Error object should always be the preference for making a call to Bugsnag.notify(), especially in Promises so that we have a known object getting sent in (https://eslint.org/docs/rules/prefer-promise-reject-errors). However, I don't think this is a PR we'd be looking to accept as in some cases where an Error is not thrown, we still want a notification to be sent, e.g. legacy browsers, packages & code, or even just mistakes where a non-Error is tolerable, even if it's not perfect.

When notify is called, an Event is created:

const event = Event.create(maybeError, true, undefined, 'notify()', this._depth + 1, this._logger)

When creating the event we make a call to normalize the "error":

const [error, internalFrames] = normaliseError(maybeError, tolerateNonErrors, component, logger)

const normaliseError = (maybeError, tolerateNonErrors, component, logger) => {
let error
let internalFrames = 0
const createAndLogInputError = (reason) => {
if (logger) logger.warn(`${component} received a non-error: "${reason}"`)
const err = new Error(`${component} received a non-error. See "${component}" tab for more detail.`)
err.name = 'InvalidError'
return err
}
// In some cases:
//
// - the promise rejection handler (both in the browser and node)
// - the node uncaughtException handler
//
// We are really limited in what we can do to get a stacktrace. So we use the
// tolerateNonErrors option to ensure that the resulting error communicates as
// such.
if (!tolerateNonErrors) {
if (isError(maybeError)) {
error = maybeError
} else {
error = createAndLogInputError(typeof maybeError)
internalFrames += 2
}
} else {
switch (typeof maybeError) {
case 'string':
case 'number':
case 'boolean':
error = new Error(String(maybeError))
internalFrames += 1
break
case 'function':
error = createAndLogInputError('function')
internalFrames += 2
break
case 'object':
if (maybeError !== null && isError(maybeError)) {
error = maybeError
} else if (maybeError !== null && hasNecessaryFields(maybeError)) {
error = new Error(maybeError.message || maybeError.errorMessage)
error.name = maybeError.name || maybeError.errorClass
internalFrames += 1
} else {
error = createAndLogInputError(maybeError === null ? 'null' : 'unsupported object')
internalFrames += 2
}
break
default:
error = createAndLogInputError('nothing')
internalFrames += 2
}
}
if (!hasStack(error)) {
// in IE10/11 a new Error() doesn't have a stacktrace until you throw it, so try that here
try {
throw error
} catch (e) {
if (hasStack(e)) {
error = e
// if the error only got a stacktrace after we threw it here, we know it
// will only have one extra internal frame from this function, regardless
// of whether it went through createAndLogInputError() or not
internalFrames = 1
}
}
}
return [error, internalFrames]
}

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 notify() received a non-error. See "notify()" tab for more detail., this sounds like a situation where you needed to specify an Error but did not, i.e. this specific message you saw was context specific.

@adkenyon
Copy link
Author

adkenyon commented Apr 4, 2022

@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 Error type compared to a non-Error type results in two very different results on BugSnag. This last week we shipped an app release that passed a String type to .notify() and the data in BugSnag isn't useful or actionable. We've having to update the code and create another app release. We've effectively wasted our time and lost a week of data. By passing an Error type, the data is much more useful in Bugsnag and actionable. This is something also comes up every few months, a dev thinks that a String type will work as expected when in actuality it doesn't.

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

@xljones
Copy link
Contributor

xljones commented Apr 8, 2022

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 xljones added the needs discussion Requires internal analysis/discussion label Apr 8, 2022
@adkenyon
Copy link
Author

@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.

@gareththackeray
Copy link
Contributor

gareththackeray commented Apr 27, 2022

Hi @adkenyon. Could you confirm that you simply passed called Bugsnag.notify('some string') and got notify() received a non-error. See "notify()" tab for more detail. as the message?
I've tried to reproduce this and can't. You should only get that message under certain circumstances like unhandled rejections. If you pass an arbitrary object you will get the behaviour you describe. Is it possible that that is what happened?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs discussion Requires internal analysis/discussion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants