sindresorhus / ky

🌳 Tiny & elegant JavaScript HTTP client based on the Fetch API
MIT License
13.61k stars 364 forks source link

Add `cause` to `TimeoutError` #593

Closed arty-name closed 3 months ago

arty-name commented 3 months ago

Fixes https://github.com/sindresorhus/ky/issues/592

This is the simplest approach we could take

arty-name commented 3 months ago

The CI fails on TypeScript not knowing of this property. Locally I see this defined in lib.es2022.error.d.ts, so apparently this was added in ES 2022, which is indirectly confirmed by MDN. Apparently Ky uses a lower version of JS, which is reasonable.

I believe that this second parameter to the Error constructor will be simply ignored by older runtimes, so there should be no danger in adding it.

What would be your preferred way of handling this error in Ky project?

sindresorhus commented 3 months ago

I think it would be more correct to attach the URL as a property on the error. The URL is not really the "cause" of the error.

arty-name commented 3 months ago

Thank you for the quick response!

I see your point, that’s fair enough. I understand that you wouldn’t want to have this in the core.

Personally, I’ll still use cause for practical reason: only cause is supported by tooling. Do you think that this use case justifies adding the beforeTimeout hook?

sindresorhus commented 3 months ago

Personally, I’ll still use cause for practical reason: only cause is supported by tooling.

Not sure what you mean by this, but adding properties to errors has always been the way to add metadata.

arty-name commented 3 months ago

It was, but only the cause gets special treatment by e.g. browser developer tools and Vite/Astro developer overlay. And I am not sure how normative this is, but MDN suggests to use it for metadata. image image image

sholladay commented 3 months ago

The URL alone doesn't seem like a good use of cause.

I would be okay with either or both of the following:

  1. Move error.request to error.cause to conform to the expectations of the broader ecosystem and make the metadata more discoverable
  2. Add the request method and URL directly to the timeout error message for simple debugging cases
sindresorhus commented 3 months ago

Move error.request to error.cause to conform to the expectations of the broader ecosystem and make the metadata more discoverable

While it is acceptable to use cause for non-errors, I don't think the community conventions for cause are established enough yet for me to want to do this.

Add the request method and URL directly to the timeout error message for simple debugging cases

Let's do this.


I tried doing a Twitter poll for this, but it ended up being inconclusive: https://x.com/sindresorhus/status/1803812187758080325

sholladay commented 3 months ago

Closing in favor of PR #595

@arty-name what do you think of that solution?

arty-name commented 3 months ago

@sholladay This solution will also resolve my issue, thank you!