angular / angular.js

AngularJS - HTML enhanced for web apps!
https://angularjs.org
MIT License
58.8k stars 27.48k forks source link

Ability to react to progress events of $http XHR #1934

Closed ryankshaw closed 8 years ago

ryankshaw commented 11 years ago

I would like to be able to bind to the progress event of the xhr upload.

for example (in plain javascript):

var xhr = new XMLHttpRequest(); xhr.upload.addEventListener("progress", uploadProgress, false)

(so I can show a progress meter for uploaded files)

I want to be able to do the same thing by doing $http.post...

for this and any other case edge case where you need to interact with the raw XHR object itself it might be nice to provide something that lets you just get at the original xhr object.

michaeltaranto commented 11 years ago

+100! Really need to be able to access the internal xhr object of $http service.

bjconlan commented 11 years ago

Providing an 'xhr' config option (which I assume would be callback that gets the xhr instance passed in) is probably the simplest option to implement (and also means that we don't need to update angular every xhr spec change), but I think the correct way to do this is to extend the $q object to support a progress handler callback as per http://wiki.commonjs.org/wiki/Promises/A. This keeps with the nice abstraction angular provides to the $http object and since it is already based upon $q promises it could then provide something akin to what you are attempting to do (bind to the progress events)

Perhaps #2223 can help with this.

aminariana commented 11 years ago

+10

My issue is that I'd like to cancel the request if it wasn't completed upon new request.

I'm issuing many search queries async (depending on the user behavior) and there's severe race condition. The work-around I see right now is to fall back on jQuery.ajax method, simply because that's the only thing I'm familiar with that provides an XHR that I can cancel upon new request.

trea commented 11 years ago

+1 I'd also like to see something implemented for this purpose.

Thanks!

magro commented 11 years ago

+1

miki725 commented 11 years ago

+1

gustavohenke commented 11 years ago

+1

danialfarid commented 11 years ago

+1

anormal81 commented 11 years ago

+1

lu4 commented 11 years ago

+1

cgwyllie commented 11 years ago

+1

lovenigma commented 10 years ago

+1

bhalperin28 commented 10 years ago

+1

Guuz commented 10 years ago

:+1: I was surprised when I noticed this is not possible in AngularJS...

miki725 commented 10 years ago

Any plans to solve this. Maybe in 1.3?

IlanFrumer commented 10 years ago

+1

miki725 commented 10 years ago

+1

P.S.: I already +oned however posting again since not sure if my initial "+1" will count towards the 1.3 feature poll.

magro commented 10 years ago

+1

Guuz commented 10 years ago

+1

marcalj commented 10 years ago

+1

thvd commented 10 years ago

+1

michalkvasnicak commented 10 years ago

+1

gustavohenke commented 10 years ago

@miki725 seems like it's planned for the 1.3 milestone :)

standuprey commented 10 years ago

+1

bhalperin28 commented 10 years ago

+1

CWSpear commented 10 years ago

+1, especially with a $q.progress integration like @bjconlan suggests.

miki725 commented 10 years ago

@CWSpear can you post a link for the $q.progress proposal.

CWSpear commented 10 years ago

@miki725 Basically add support for $q.progress to match https://github.com/kriskowal/q#progress-notification and then have $http use it onprogress... Does that make sense?

CWSpear commented 10 years ago

@miki725 see also http://wiki.commonjs.org/wiki/Promises/A as @bjconlan mentioned.

caitp commented 10 years ago

$q already supports progress notifications, it's just that $http doesn't make use of them (however we have probably fallen behind q's implementation, since we don't currently pass the aplus 1.1 spec... but that doesn't cover progress notifications at all, so we're probably okay there)

CWSpear commented 10 years ago

Oh yeah, then all the more that $http should just hook into that, and it should be "relatively" simple.

caitp commented 10 years ago

submit a patch! you'll be a hero adored by the masses!

CWSpear commented 10 years ago

Well, I should expound on "relatively" simple. IE9 doesn't support the ProgressionEvent interface. Maybe, maybe the Angular team would allow it without IE9 support, but they traditionally like everything they have to support all their supported browsers, and things get complicated trying to support older versions of IE9.

But doesn't mean it's not worth trying to patch for at least a proof of concept for supported browsers, or for someone to maybe make a plugin so people who don't need to support it for IE9 are free to still use it, etc.

caitp commented 10 years ago

I think that's probably okay, because people with operating systems which support IE9 aren't locked into using IE9 (they can be using IE10 or 11, for instance) --- and this really just means that at worst, you might not see some fancy progress bar on IE9... In most cases, this is probably just a graceful degradation of a prettier website (assuming that people are mostly using these for things like progress-bars and the like, which seems probable)

So, since it's not going to royally break apps running on IE9, my bet is that it's merge-able. And it doesn't hurt to push it forward anyways

CWSpear commented 10 years ago

I don't disagree, it's just not my call. I have been digging in to see what it would take to add into the core, though.

caitp commented 10 years ago

Don't worry, I'm pretty sure this feature could be implemented without adding huge piles of code, and it's something people would appreciate... I'm quite sure we could get a good, lean implementation merged. It might potentially be blocking the $q rewrite, though, so perhaps the sooner, the better.

I know it's the holidays, but it would be good to get one (or more) proposal implementations for this some time in January

CWSpear commented 10 years ago

Speaking of holidays, I am all but on my way out the door, but I did want to throw up a super crude proof of concept to maybe helps someone get started: https://github.com/CWSpear/angular.js/commit/7b4daca345c5bfda5769b93818815567768b5e4e

It was adding basically 4 lines, and changing 2 lines (and I changed another variable name to clarify things), but it does work. No tests, I am sure breaks some tests and maybe other parts of the API, and no documentation or comments. But you can apply those changes yourself and see it in action with a snippet like this:

$http.post('/', { dummy: 'data' }).then(angular.noop, angular.noop, function (event) {
    console.log('Progress:', event);
});
caitp commented 10 years ago

Just to track this, there is one pull request implementing this (#3606) that I can see, in addition to @CWSpear's change up there.

CWSpear commented 10 years ago

Oh yeah, and his is much more complete. Basically did what I did, but much more thoroughly.

mjc-gh commented 10 years ago

The PR should be ready to be merged. The travis build did fail for it so that's probably the last thing holding it up. When I get back from ng-conf, I'll make whatever changes are needed to make travis happy.

caitp commented 10 years ago

@mikeycgto it (or some variation of it) will make it into one of the 1.3 releases. In the mean time, you may want to rebase your PR and make sure it's passing tests and jshint and stuff

mjc-gh commented 10 years ago

Will do. Sounds good! On Jan 15, 2014 5:00 PM, "Caitlin Potter" notifications@github.com wrote:

@mikeycgto https://github.com/mikeycgto it (or some variation of it) will make it into one of the 1.3 releases. In the mean time, you may want to rebase your PR and make sure it's passing tests and jshint and stuff

— Reply to this email directly or view it on GitHubhttps://github.com/angular/angular.js/issues/1934#issuecomment-32429235 .

ceefour commented 10 years ago

+1 for this too ^_^

jonnysamps commented 10 years ago

+1

jimmywarting commented 10 years ago

+1 for the upload progress event +1 for the download progress event +1 for access to xhr if spec is changing +1 for readyStage change notification (good for progressive enhancement if progress event dosen't exist) stage 2 = you are uploading, 3 = you are downloading, 4 = you are done i did a converter that is uploading a file and downloading the converted in the same request and showed a combined progress bar. basicly set the progress to 50% then (half the benefit if progress event wasn't supported) +1 for abortion/canceling +1 for just access the responsHeader in readystate == 2 so you can get filename from content-disposition header from the very beginning then changing it after stage 4 +1 for just being able to get header information about a request and cancel the request to save bandwith

// upload progress
xhr.upload.addEventListener("progress", uploadProgress, false)
// download progress
xhr.addEventListener("progress", downloadProgress, false)

// both events looks the same...
jimmywarting commented 10 years ago

maybe need something better then then(success, error, progress) if we are going to throw in upload/download/stagechange into the same progress callback

Hope 1.3 comes up with something good soon

jbruni commented 10 years ago

+1

psi-4ward commented 10 years ago

+1

wildjordanpaul commented 10 years ago

+1

gabi91cata commented 10 years ago

+1