feat(ajax): Add option for streaming progress #6001
Conversation
|
There will be a follow up refactor to make this smaller as well. |
|
Blocked on merging #6002 |
benlesh
force-pushed
the
feat-issue-2833-ajax-with-progress
branch
from
b5edbe3
to
35169c6
Compare
|
|
||
| expect(results).to.deep.equal([ | ||
| { | ||
| type: 'download_loadstart', |
There was a problem hiding this comment.
nit: Is this an enum? Can we export one if not?
There was a problem hiding this comment.
I thought about it. I think that we have to do enum and not const enum, and therefore we'd end up exporting an object. That doesn't really mesh with how events work elsewhere. I'll consider this a vote in favor of an enum, though.
There was a problem hiding this comment.
FWIW, my vote would be against exporting an enum. If something is to be exported, I'd vote for a type that's a union of literals, rather than an enum.
| @@ -1124,10 +1032,365 @@ describe('ajax', () => { | |||
| }); | |||
| }); | |||
| }); | |||
|
|
|||
| describe('with includeDownloadProgress', () => { | |||
There was a problem hiding this comment.
Are there any tests that have a binary type? Not for this PR but may be something to consider adding
There was a problem hiding this comment.
Binary, as in streaming data? XHR doesn't really support that. Fetch does, though.
If you mean "binary" as in a binary data type like a "Content-Type": "image/png" or something... All of that is handled directly by the XHR we're using under the hood, and I'm not sure how I'd test that without an end-to-end test of some sort, which we aren't quite set up for. Right now we're mocking the XHR. I could test that within the confines of our mocks though, I guess.
There was a problem hiding this comment.
All of that said, it should "just work", because in the event of binary data like that, the XHR in the browser will have a responseType of "arraybuffer", and the response itself would be something like Uint8Array, which is used directly in our AjaxResponse, without any additional processing.
- Adds configuration for `includeDownloadProgress` and `includeUploadProgress` that will add streaming download and upload events (respectively) to the resulting observable. - Removes two tests that are no longer relevant. - Adds some additional documentation. - Updates ajax tests to support EventTarget APIs on the mock XHR impl. - Attempts to reduce the size of the ajax implementation.
benlesh
force-pushed
the
feat-issue-2833-ajax-with-progress
branch
from
35169c6
to
c7ed50a
Compare
| @@ -948,85 +948,6 @@ describe('ajax', () => { | |||
| delete root.XMLHttpRequest.prototype.ontimeout; | |||
| }); | |||
|
|
|||
| it('should work fine when XMLHttpRequest onprogress property is monkey patched', function () { | |||
There was a problem hiding this comment.
These tests are no longer relevant, as they were only for when we were using the property method versions of event handlers like xhr.onprogress. Now we're using xhr.addEventListener('progress', ...), so it's a non-issue. Since Zone.js is patching EventTarget, this should "just work". Also, hopefully in the not-so-distant future, Zone.js goes away.
| * If both this and {@link includeDownloadProgress} are `false`, then only the {@link AjaxResponse} will | ||
| * be emitted from the resulting observable. | ||
| */ | ||
| includeUploadProgress?: boolean; |
There was a problem hiding this comment.
When this option is true, shouldn't ajax return Observable<AjaxUploadProgressEvents | AjaxDownloadCompleteEvent | AjaxResponse<T>> instead of Observable<AjaxResponse<T>>? Likewise for includeDownloadProgress and AjaxProgressEvents. /cc @benlesh
If that makes sense I can try to send a PR or file an issue.
Edit: I've moved the spec changes to another PR that should be reviewed first here: #6002
includeUploadProgressandincludeDownloadProgressthat will add additional events to the output stream before the event that is usually emitted.Resolves #2833
NOTE: We still need to add an example of usage.