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

Add cause to TimeoutError #593

Closed
wants to merge 2 commits into from
Closed

Add cause to TimeoutError #593

wants to merge 2 commits into from

Conversation

arty-name
Copy link
Contributor

Fixes #592

This is the simplest approach we could take

@arty-name
Copy link
Contributor Author

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
Copy link
Owner

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
Copy link
Contributor Author

arty-name commented Jun 19, 2024

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
Copy link
Owner

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
Copy link
Contributor Author

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
Copy link
Collaborator

sholladay commented Jun 20, 2024

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
Copy link
Owner

sindresorhus commented Jun 20, 2024

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
Copy link
Collaborator

Closing in favor of PR #595

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

@sholladay sholladay closed this Jun 23, 2024
@arty-name
Copy link
Contributor Author

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

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.

Set TimeoutError#cause to be Request or URL
3 participants