Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

feat($http): flag timed out requests #11272

Closed
wants to merge 1 commit into from

Conversation

gabrielmaldi
Copy link
Contributor

This allows differentiating a network error from a timeout.

If you guys thinks this is OK I'll write some tests and update the docs.

the response object passed into the then method will have an additional
timedOut=true property when the request was aborted due to a timeout.
// normalize internal statuses to 0
status = Math.max(status, 0);

(isSuccess(status) ? deferred.resolve : deferred.reject)({
var result = {
data: response,
status: status,
headers: headersGetter(headers),
config: config,
statusText: statusText
Copy link
Contributor

Choose a reason for hiding this comment

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

why not just add timeOut: timeOut here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't want to alter the current success result, since it logically can never be a timeout (hence timeOut: false would be redundant; I think of timeOut as a possible outcome of an error result). But maybe it'd better to add timeOut: [true | false] every time.

Another question: would you go with timedOut, timeOut, or timeout?

@lgalfaso
Copy link
Contributor

this looks interesting, but it needs some tests and documentation.

@pkozlowski-opensource
Copy link
Member

We've got an issue for this already (#4491) with a associated PR (#8756). Would be good to discuss pros and cons of the respective approaches. But yeh, in any case we need tests, doc updates etc.

@gabrielmaldi
Copy link
Contributor Author

I gave it a bit of thought and think that the status approach is better. So I'll wait for #8756 👍

@gabrielmaldi gabrielmaldi deleted the http-timeout-flag branch April 12, 2015 23:56
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants