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.

Sign up for GitHub

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

Jump to bottom

Remove is-error dependency #2911

Closed
wants to merge 3 commits into from
Closed

Remove is-error dependency #2911

wants to merge 3 commits into from

Conversation

drazisil
Copy link

👋

This isn't needed at all, but seemed like an easy edit to reduce dependencies.

@drazisil drazisil marked this pull request as ready for review December 15, 2021 03:06
@novemberborn
Copy link
Member

@sindresorhus is there any nuance with is-error that isn't captured here?

@sindresorhus
Copy link
Member

instanceof will fail if the error is coming from another realm (iframe, Node.js's VM module, etc).

You could alternatively inline Object.prototype.toString.call(error) === '[object Error]'.

@novemberborn
Copy link
Member

Is that a scenario we should still care about?

@novemberborn
Copy link
Member

Can we use https://nodejs.org/api/util.html#utiltypesisnativeerrorvalue?

@novemberborn novemberborn changed the title refactor(deps): remove is-error dependency Remove is-error dependency Dec 31, 2021
@novemberborn
Copy link
Member

novemberborn commented Dec 31, 2021 •
edited
Loading

isNativeError() didn't work, even in a situation where instanceof Error returned true. The documentation is quite sparse so I'm not sure what it's meant to do!

However I don't want to be in a situation where t.throws() fails because it's not detecting an Error instance even though when we format that object it very much looks like an Error. We should handle all (actual) errors that Node.js can throw at us even if from a different realm. That's what is-error provides.

@drazisil drazisil closed this Jan 2, 2022
@drazisil drazisil deleted the remove-is branch January 2, 2022 17:57
@novemberborn novemberborn mentioned this pull request Jul 30, 2023
Merged
novemberborn added a commit that referenced this pull request Jul 30, 2023
@novemberborn
Only treat native errors as errors
0ea5763 
* Remove is-error dependency
* Document edge case where `error instanceof Error` can be true, yet AVA does not recognize `error` as an error

See also #2911 for an earlier attempt.
novemberborn added a commit that referenced this pull request Jul 31, 2023
@novemberborn
Only treat native errors as errors
e58f466 
* Track worker errors. They're not native due to nodejs/node#48716, but we want to treat them as such anyway.
* Only treat native errors as errors
* Remove is-error dependency
* Document edge case where `error instanceof Error` can be true, yet AVA does not recognize `error` as an error

See also #2911 for an earlier attempt.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@drazisil @novemberborn @sindresorhus