Added support for XHR progress event

[nantunes]

"progress" callback can be supplied via options. Also created global event ajaxProgress.

[nantunes]

Ok, I'm sorry... Only latter I realised I should have searched http://bugs.jquery.com/, not this repository.
This pull request can be related to part of http://bugs.jquery.com/ticket/9883.

Edited: Well, I've seen other direct pull requests that got merged, so I'll keep this to see if someone reviews my code. But I'll understand if it gets closed.

[dmethvin]

It's definitely worth a look from @jaubourg. Before this lands it would need documentation, and that concerns me because it's yet another case where platforms and transports would not work consistently in $.ajax calls. Also, we haven't worked out our standard-Promise story and this complicates it since Promise doesn't support progress yet.

[nantunes]

Yes, I thought about that. I even dedicated some time looking for some kind of existing or proposed "onprogress" event for the script tag, similar to img tag, but no luck (every tag that loads resources should have such an event, imho).

I proceeded because:

1. There are already several cases of functionality available only to xhr requests.
2. I think one should not limit the functionality to the least common denominator (or we couldn't do POST requests, as script doesn't allow it).
3. I tried to keep consistency: progress option can be passed on script or jsonp requests, it simply isn't called as the progress is indeterminate.

Regarding this last point, the same is true for xhr requests if Content-Length header is missing. Maybe we should not call progress callback if the event.lengthComputable isn't true, making it similar to the other transports?
(The current code calls the callback with progress_ratio undefined, but also doesn't update jqXHR.bytesLoaded - I will correct that, but keep the call to the progress callback for now).

As for the Promises, the ongoing long discussion about the progress callback isn't clear about how it should work. Maybe for now that's not something for what Promises are for. Maybe that's something still only for the event listeners/callbacks world.

[dcherman]

This looks pretty tied to working with XHR progress events, but what about a general support for progress callbacks/promises for ajaxTransports in general which the XHR transport could then build off of?

[nantunes]

xhr.js is, of course, tied to the XHR event, but in ajax.js it is somewhat abstracted... The terminology used is the one from the Progress Events specification.

Currently other transportations have no mechanism for progress reporting, but that doesn't break the spec, as it states that progress callback is called "zero or more" times.

Maybe the callback signature should be progress(progress_ratio) but progress(event), simply passing the ProgressEvent?

[dcherman]

I guess it would depend on how you define what "progress" is. In the case of XHR it's relatively straightforward since we have a spec to state what it means, but in the case of other ajaxTransports that may not be the case.

As an example, I have an ajaxTransport where a server may immediately return an HTTP 200, however the real response is generated over the course of a few minutes and is then available at another endpoint. My transport abstracts that away so that you just make a request and the transport completes whenever a response is available at the given endpoint. Progress in that case may not mean what it does for XHR, but maybe it just means "I checked the endpoint for a response, nothing is available yet".

Also, I wonder if only a single argument should be passed to the progress handler since that's how real promises behave. I know the existing promises/callbacks all accept multiple args and can't be changed for back-compat reasons, but maybe the bleeding should stop here?

[dmethvin]

Currently other [transports] have no mechanism for progress reporting, but that doesn't break the spec, as it states that progress callback is called "zero or more" times.

True, it follows the letter of the docs but not the spirit. Users may intuitively expect that a progress handler will be called if they pass one in. This places a heavier burden on the documentation to disappoint the user in advance so they will not misunderstand. It may also be error prone since the user can do rather minor things (e.g., change the url) and suddenly invoke a transport that will not give any progress notifications.

As an example, I have an ajaxTransport ... My transport abstracts that away

Custom transports are definitely a rare exception rather than the rule. Since few people know how ajax transports work, it might be better from a maintainability perspective to avoid them so someone could see the full implementation on the app side rather than having the unusual request disappear into $.ajax and re-emerge in a custom transport. In that sense I see transports more like the special events and attrHooks interfaces where we don't expect most developers to use them or need to know how they work.

Also, I wonder if only a single argument should be passed to the progress handler since that's how real promises behave.

Real Promise as in the standard or /A+ doesn't have a progress handler, and it's not clear one will ever be defined. It's taken this long for the standard to reach a subset of $.Deferred functionality, which still isn't implemented in all the evergreen browsers. So I don't know that I'd let that affect any design decisions here. The semantics between the two differ enough that it won't be possible to swap out one for another in cases like this.

[dcherman]

Won't this only ever cause the progress function to be called, but have no effect on any functions attached via the progress promise interface?

[dmethvin]

Yes, it looks like it skips the Deferred progress but calls the callback and fires the (new, yet-to-be-documented) ajaxProgress event.

[nantunes]

I wasn't aware of deferred progress method. But what have we been discussing then?

I will push a commit with "deferred.progress( callbackContext, [ progress_ratio, jqXHR ] );".

[nantunes]

About "ajaxProgress" event, is just a global event like the others "ajaxXXXX" events.

[nantunes]

True, it follows the letter of the docs but not the spirit. Users may intuitively expect that a progress handler will be called if they pass one in.

Then maybe we should call the handler once, so the user knows the progress is indeterminate (which also can happen on XHR)?

My transport abstracts that away so that you just make a request and the transport completes whenever a response is available at the given endpoint. Progress in that case may not mean what it does for XHR, but maybe it just means "I checked the endpoint for a response, nothing is available yet".

This implementation focus on resource download, that's what the Progress Events spec covers. Bytes transferred, not work done.
The server could take 10 seconds to send the first byte of an image, and then 2 seconds to send the remaining 2MB. The progress would go from 0 to 100% in two seconds after the first ten.

Even the request's upload time isn't part of the progress.
(xhr.upload.onprogress exists, but is not covered in this issue)

This is similar to one of the main questions in the discussion of Promise progress handler (that was in "Promises/A proposal" but removed till clarification in "Promises/A+ open standard"): in case of propagation, should it report the progress during the life of a promise or the progress through the promise chain?

Also, I wonder if only a single argument should be passed to the progress handler since that's how real promises behave.

I created the progress_ratio, a single value: undefined if progress is indeterminate, from 0 to 1 if it is determinate.
I also store the bytesLoaded and bytesTotal in jqXHR because the info exists and can be useful for someone, but the callback is called with a single value (and jqXHR as with the other callbacks, but if a Promise existed it could receive a single value).

As the Promise progress handler is stil under discussion I think it should not be implemented for now.

[dcherman]

notify or notifyWith is actually the method you're looking for here, progress is used to attach handlers.

You may want to look at https://github.com/jquery/jquery/blob/master/src/ajax.js#L616-L618 and follow suit so that the ordering of callbacks is maintained, otherwise this would be backwards from the order others are fired. that would also allow you to delete the call to s.progress below

[nantunes]

Many thanks dcherman, I usually don't use jquery deferred, so made a little mess... And because I was calling the callback directly, nothing was wrong in the tests.

Please see if I'm closer to getting this right. ;-)

[dcherman]

No problem, @jaubourg is the master here though, I've just written a bunch of plugins for ajax so I'm familiar with it :) Code-wise it looks fine now to me though.

Custom transports are definitely a rare exception rather than the rule.

Sure, but there is almost no difference in code to support a generic progress handler. All you have to do is pass something like

function() {
  deferred.notifyWith( callbackContext, arguments );
}

As the progress argument for a transport. The XHR transport could then easily support what this PR does, but other transports could implement their own progress notifications however they see fit.

[nantunes]

I saw in @jaubourg profile that he did some contributions last week, after some time away, so I'm bumping this thread.

In my projects I'm currently adding the progress event listener in the settings' xhr callback as a workaround, so the patch isn't critical.
Just wanted to know if the request will be able to land or if it should be closed.

Thanks.

[dmethvin]

@nantunes don't worry about the long delays, I've tagged this for 1.12/2.2 which we haven't really started on. Right now we're working on the next patch releases. We wouldn't want to land this until we've finished the patch release.

As I mentioned above, my main concern would be the additional API and documentation. I don't think we need an ajaxProgress event which would reduce the size and docs a bit. Having this tied only to the XHR transport doesn't bother me. Also we could shrink this further (I think) by letting the callee calculate the ratio, just passing them the event object with the data perhaps?

@jaubourg your thoughts?

[dcherman]

@dmethvin FWIW A few minutes ago I had someone ask me to enhance the long-polling transport that I mentioned earlier to provide progress notifications, so there is an actual use case for progress notifications outside of XHR if that helps

[dmethvin]

I am concerned that the $.ajax() interface is already too complex, as evidenced by the documentation page. This adds quite a bit to it. At one point were were considering a simpler $.xhr() but at this point would prefer to build a simpler API based on the fetch spec. It might be possible to build a more convenient API over this polyfill as a jQuery plugin if it made sense.