-
Notifications
You must be signed in to change notification settings - Fork 25.8k
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
feat(http): Add timeout option to HTTP requests #57194
base: main
Are you sure you want to change the base?
Conversation
I'm a bit conflicted about this. |
While I agree with adding a timeout to eliminate the need for an RxJs timeout, especially since fetch handles this natively, I would expect an aborted fetch request to result in a standard |
Just to avoid any confusion, since RXJs also has a TimeoutError interface. You mean throwing |
Yes, when not using |
I don't know how Angular works internally when it comes to polyfills, but after looking into it, I'd need to use AbortSignal.any() in order to support the Observable unsubscription and the timeout at the same time. But looking at caniuse, that function only has a 83% support rate. Is that acceptable? |
Angular doesn't provide any polyfills also we need to support at least the last 2 major versions of the browsers. Safari implemented it in the current major which makes it a no-go. |
So until then, we'd need to use the setTimeout method for fetch instead of combining the two signals. According to MDN TimeoutError is a legacy error code: https://developer.mozilla.org/en-US/docs/Web/API/DOMException#timeouterror |
That looks like the correct alternative. Beside the pending review, you'll need to update the "golden files". You can do this by running |
I updated the code with the suggestions, however that command fails for me. I'm currently on Windows and am getting this error (Bazel always gives me some trouble): I'll be away for a bit more than two weeks and won't be able to address this until then, when I can be on my normal machine. |
Got back from holidays and got the command to work on my non-Windows machine (I'm not sure if that's supposed to only work on Unix-like systems). |
I had to remove the test for what's in the DOMException. I have no idea why, but during unit tests, the information Exception doesn't get treated like in actual browsers and NodeJS.
|
Our tests run both in NodeJS and in actual browsers. You could guard your test like we already do in some other tests. angular/packages/common/test/directives/ng_optimized_image_spec.ts Lines 39 to 43 in 368f36d
|
Btw, the CLA job is red because you authored commits with 2 different addresses : |
Yes, I know that, I'm still not on the same two computers. I'll squash the commits when everything is okay with my private account. However I'm not sure why DOMException gets mocked in NodeJS. Is there a difference between DOMException in Node and in browsers? It seems to me that naive tests give always the same results when using it. |
This seems like a known issue: |
I stumbled upon that issue as well, but from my understanding it seems to not be related. When I run in Node.Js const e = new DOMException('', 'AbortError');
console.log(e.name); I still get "AbortError". But undefined in tests. So it somehow gets eaten at some point. That said, I don't know how important it is to you guys at the end of the day. |
Add timeout option to both XHR and fetch backends. POTENTIAL BREAKING CHANGE: Aborted requests with fetch backend no longer throw DOMException
Sorry I had missed @ajitzero's comment |
Add timeout option to both XHR and fetch backends.
POTENTIAL BREAKING CHANGE: Aborted requests with fetch backend no longer throw DOMException
PR Type
What kind of change does this PR introduce?
Does this PR introduce a breaking change?
I am not certain it is very relevant, but the old behaviour would throw a DOMException due to not setting the
reason
option to the AbortController.If you think it's too complicated, we can simplify it, by just calling abort() in both manual abort and timeouts without the reason.
Other information
Not quite satisfied with how the test for the fetch backend was done, but I'm not sure how to mock the timeout properly. The previous test for request aborting didn't seem to be very flexible.