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

fix: augment FetchError type to include IFetchError #279

Merged
merged 2 commits into from
Aug 25, 2023
Merged

fix: augment FetchError type to include IFetchError #279

merged 2 commits into from
Aug 25, 2023

Conversation

johannschopplich
Copy link
Contributor

πŸ”— Linked issue

#278

❓ Type of change

  • πŸ“– Documentation (updates to the documentation, readme, or JSdoc annotations)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

With the latest ofetch version, the FetchError does implement the IFetchError interface, but TS won't provide the types. Thus, only message, stack etc. properties are available.

Since the FetchError will be initiated by createFetchError, we can add the properties from the interface directly in the class definition, without the need of setting them in the constructor. With these changes, TypeScript understands.

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

@pi0
Copy link
Member

pi0 commented Aug 24, 2023

Thanks for this PR dear @johannschopplich but splitting the types out of the class was intentional. (sadly) esbuild adds them as public accessors which makes them (wrongly) visible on stack traces while they should be getters only.. I am thinking if possible to sole type issue somehow differently while keeping runtime minimal.

@johannschopplich
Copy link
Contributor Author

johannschopplich commented Aug 25, 2023

Oh, I see! I wasn't aware of that issue. Maybe there's another way of merging the types then. πŸ€”

@johannschopplich johannschopplich changed the title fix: add missing error type properties fix: augment FetchError type to include IFetchError Aug 25, 2023
@johannschopplich
Copy link
Contributor Author

Edit: I have updated this PR to use a type-only approach: We can augment FetchError type to include all IFetchError properties.

// Augment `FetchError` type to include `IFetchError` properties
export interface FetchError<T = any> extends IFetchError<T> {}

Copy link
Member

@pi0 pi0 left a comment

Choose a reason for hiding this comment

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

πŸ’―

@pi0 pi0 merged commit 26fd44b into unjs:main Aug 25, 2023
@johannschopplich johannschopplich deleted the fix/add-missing-error-types branch August 25, 2023 10:13
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.

2 participants