dojo / core

:rocket: Dojo 2 - language helpers and utilities.
http://dojo.io
Other
213 stars 58 forks source link

Adding upload progress reporting, issue #285 #326

Closed rorticus closed 7 years ago

rorticus commented 7 years ago

Type: feature

The following has been addressed in the PR:

Description:

Adds upload progress monitoring to the node & XHR request providers. It is implemented as a request option because by the time the returned promise is resolved, the uploads have already finished. Basic usage is,

const uploadObserver = new UploadObserver();
uploadObserver.on('upload', event => {
    console.log(`Total bytes uploaded: ${event.totalBytesUploaded}`);
});
request.get('http://www.example.com', {
    method: 'POST',
    body: aLotOfData,
    uploadObserver
});

XHR was easy as it emits upload events already. The node uploader was a bit more tricky, as there doesn't seem to be a way to find out how much data node has uploaded when using request.write. As such, I've added support for passing a readable stream to the node provider. As data is read from the stream, it is written to the request, and upload progress can be reported.

Resolves #285

rorticus commented 7 years ago

CI failures seem to be an unrelated issue 😄

codecov[bot] commented 7 years ago

Codecov Report

Merging #326 into master will increase coverage by 0.07%. The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #326      +/-   ##
==========================================
+ Coverage   93.97%   94.05%   +0.07%     
==========================================
  Files          39       40       +1     
  Lines        2142     2202      +60     
  Branches      407      408       +1     
==========================================
+ Hits         2013     2071      +58     
- Misses         51       52       +1     
- Partials       78       79       +1
Impacted Files Coverage Δ
src/request/Response.ts 78.57% <100%> (-0.74%) :arrow_down:
src/request/providers/xhr.ts 87.66% <100%> (+1.84%) :arrow_up:
src/request/SubscriptionPool.ts 100% <100%> (ø)
src/request.ts 96.87% <100%> (+0.72%) :arrow_up:
src/request/providers/node.ts 89.42% <100%> (+0.53%) :arrow_up:
src/QueuingEvented.ts 90.62% <0%> (-6.25%) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update b4492c3...57e3335. Read the comment docs.

rorticus commented 7 years ago

@dylans Updated to use Node.js 👍

rorticus commented 7 years ago

@kitsonk OK, I've refactored to use Observables for everything. See the README for details, but basically Request exposes upload and Response exposes download and data.

rorticus commented 7 years ago

@kitsonk Added a few more tests for better coverage

rorticus commented 7 years ago

We might be able to get rid of QueuingEvented, but I'm not sure if anyone is using it... It's no longer being used in core but it looks like it's been modified since creation, so I just left it alone.

kitsonk commented 7 years ago

The only thing I am missing is the full coverage on this line. If it isn't testable, then we don't need the default argument.

rorticus commented 7 years ago

@kitsonk That line should be covered now!