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

feat($http): add notify callback for loading requests #9258

Conversation

chrisirhc
Copy link
Contributor

Allows progress notifications for $http requests.

This makes use of the readyState 3 of XMLHttpRequest and uses the promises' notifications API which contains partial data except for IE 9 and below.

It's intentional that this doesn't call $apply since this may occur many times in a short span of time.
I'll add tests if this proves to be useful to add.

@caitp
Copy link
Contributor

caitp commented Sep 24, 2014

it's bitrotten :x

@caitp
Copy link
Contributor

caitp commented Sep 24, 2014

otherwise the fix doesn't look too bad, the only issue is that this doesn't deal with XHRUpload progress events --- in either case it's really hard to test this without E2E tests :(

Allows progress notifications for $http requests.

This makes use of the readyState 3 of XMLHttpRequest.
@chrisirhc chrisirhc force-pushed the feature/add-http-loading-notifications branch from f9fc7bb to 8ed3035 Compare September 24, 2014 20:55
@chrisirhc
Copy link
Contributor Author

Sorry about the bad merge. Fixed.

Indeed, it'll be tricky to test in an actual environment even with E2E (uploads and downloads). The only way I thought of so far was to test it using readyState on the MockXhr object in the httpBackendSpec.

I didn't realize there were progress events on the XMLHttpRequest, but it looks like they don't actually contain partial data. But yes, it makes sense to make this work with upload progress events as well. I guess it can fire on both download and upload progress events as well as readyState = 3. Just need to figure out what exactly to call the notify() with for each of those cases. Right now, it's just calling it with the xhr object, while progress events actually have some useful information (loaded/total).

@pkozlowski-opensource
Copy link
Member

@chrisirhc / All interested in this PR: based on the discussion in #1934 (staring from here) it seems like the general trend is to remove notifications (at least in the current form) from promises. As such we don't want to solve the problem of progress notifications using promise APIs.

I'm going to close this PR (and similar ones: #3606, #5874, #9258) and focus on the callback-based solution instead (similar to the one proposed in #7995).

We still do appreciate your effort on this issue which seems to be important to many people - so if you care about progress notifications being available in AngularJS please let's have discussion in #7995.

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.

5 participants