Skip to content

fix: add name property to exported interface errors #2446

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

Merged
merged 3 commits into from
Mar 28, 2024

Conversation

2color
Copy link
Contributor

@2color 2color commented Mar 25, 2024

Description

As discussed in ipfs/helia-verified-fetch#25 (comment), this PR adds the name property to the AbortError type.

This has a couple of benefits:

  • Sticks the common convention for Error types (all built in Error types have the name property set to the class name)
  • Allows checking the error type at runtime in a TS codebase without importing the type

Notes & open questions

  • Should we also set the name property on UnexpectedPeerError, InvalidCryptoExchangeError and InvalidCryptoTransmissionError?

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation if necessary (this includes comments as well)
  • I have added tests that prove my fix is effective or that my feature works

@2color 2color requested a review from a team as a code owner March 25, 2024 14:59
@SgtPooki
Copy link
Member

SgtPooki commented Mar 25, 2024

nodejs/node#40692 (comment) has some relevant discussions about custom AbortError's too. Also some deets in ipfs/helia-verified-fetch#25 (comment)

@achingbrain achingbrain changed the title chore: add the name property to the AbortError type fix: add the name property to the AbortError type Mar 27, 2024
Copy link
Member

@achingbrain achingbrain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fetch spec says things like:

Let fallbackError be an "AbortError" DOMException.

...so I think longer term, in order to be closer to the spec, this class should be changed to extend DOMException instead of Error, then we get the .name property for free.

DOMException was added to Node.js in v17 so we are good to use it.

Unfortunately .code on DOMException is a number and not a string so it may be a breaking change, I'm not sure if we can override that for compatibilities sake, it may need testing out locally first.

@achingbrain
Copy link
Member

Should we also set the name property on UnexpectedPeerError, InvalidCryptoExchangeError and InvalidCryptoTransmissionError?

Sorry, missed this - yes, I think we should.

@2color
Copy link
Contributor Author

2color commented Mar 28, 2024

Should we also set the name property on UnexpectedPeerError, InvalidCryptoExchangeError and InvalidCryptoTransmissionError?

Sorry, missed this - yes, I think we should.

Ok I'll update the pr

@achingbrain
Copy link
Member

Just noticed that DOMException officially doesn't have a .stack property, and technically neither does Error, though every JS runtime adds it - there's a tc39 proposal to add it that's been around for, oh, 8 years 🤦

@achingbrain achingbrain changed the title fix: add the name property to the AbortError type fix: add name property to exported interface errors Mar 28, 2024
@achingbrain achingbrain merged commit 330a5ed into libp2p:main Mar 28, 2024
23 checks passed
@achingbrain achingbrain mentioned this pull request Mar 28, 2024
@2color 2color deleted the chore/abort-error-name branch March 28, 2024 12:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants