Adding upload progress reporting, issue #285 #326

rorticus · 2017-04-28T11:49:24Z

Type: feature

The following has been addressed in the PR:

There is a related issue
All code matches the style guide
Unit or Functional tests are included 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 · 2017-04-28T11:55:21Z

CI failures seem to be an unrelated issue 😄

codecov · 2017-04-28T19:45:10Z

Codecov Report

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

@@            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%)`	⬇️
src/request/providers/xhr.ts	`87.66% <100%> (+1.84%)`	⬆️
src/request/SubscriptionPool.ts	`100% <100%> (ø)`
src/request.ts	`96.87% <100%> (+0.72%)`	⬆️
src/request/providers/node.ts	`89.42% <100%> (+0.53%)`	⬆️
src/QueuingEvented.ts	`90.62% <0%> (-6.25%)`	⬇️

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.

dylans

one minor nit from me, this looks reasonable though I'm sure someone else should also review this.

dylans · 2017-04-29T12:19:30Z

docs/request.md

+});
+```
+
+Note that while the node provider will emit a single `upload` event when it is done uploading, it cannot emit more granular upload events with `string` or `Buffer` body types. To receive more frequent upload events, you can use the `bodyStream` option to provide a `Readable` with the body content. Upload events will be emitted as the data is read from the stream.


node -> Node.js

kitsonk

A few comments.

kitsonk · 2017-05-01T06:11:08Z

src/request.ts

@@ -35,6 +35,16 @@ const request: {
 	});
 });

+[ 'POST', 'PUT' ].forEach(method => {
+	Object.defineProperty(request, method.toLowerCase(), {
+		value(url: string, options: RequestOptions = {}): UploadObservableTask<Response> {


We don't have a test for request options default to nothing, therefore we are missing a branch in our testing.

kitsonk · 2017-05-01T06:11:31Z

src/request/SubscriptionPool.ts

+		this._observers.push(subscription);
+
+		return () => {
+			this._observers.splice(this._observers.indexOf(subscription), 1);


Not being covered in the tests.

kitsonk · 2017-05-01T06:11:49Z

src/request/SubscriptionPool.ts

+	}
+
+	complete() {
+		this._observers.forEach(observer => {


Not covered in testing.

kitsonk · 2017-05-01T06:12:52Z

src/request/providers/node.ts

 import Response from '../Response';
 import TimeoutError from '../TimeoutError';
 import { Readable } from 'stream';
+import { UploadObserverEvent } from '../interfaces';
+import Observable from '../../Observable';


Codecov says this file isn't covered at all.

kitsonk · 2017-05-01T06:15:12Z

src/request.ts

+		value(url: string, options: RequestOptions = {}): UploadObservableTask<Response> {
+			options = Object.create(options);
+			options.method = method;
+			return <UploadObservableTask<Response>> request(url, options);


While it isn't a big deal, we are trying to move to return request(url, options) as UploadObservableTask<Response>; The TypeScript team is recommending it to avoid issues with JSX. Though we don't plan to use JSX, it still feels cleaner/more readable.

rorticus · 2017-05-05T14:27:31Z

@dylans Updated to use Node.js 👍

rorticus · 2017-05-05T14:28:29Z

@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.

kitsonk · 2017-05-10T13:02:25Z

src/request/SubscriptionPool.ts

+	}
+
+	complete() {
+		this._observers.forEach(observer => {


kitsonk · 2017-05-10T13:02:28Z

src/request/SubscriptionPool.ts

+
+			// when the queue is full, get rid of the first ones
+			while (this._queue.length > this._queueMaxLength) {
+				this._queue.shift();


Not covered in testing.

kitsonk

We should cover the missing line and branches.

rorticus · 2017-05-12T12:10:35Z

@kitsonk Added a few more tests for better coverage

dylans

My feedback has been addressed... obviously Kit needs to verify that his feedback has as well before this is landed.

rorticus · 2017-05-12T12:28:11Z

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 · 2017-05-12T12:33:30Z

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 · 2017-05-12T13:08:56Z

@kitsonk That line should be covered now!

kitsonk

Happy camper now! 😂 ⛺️

rorticus requested review from agubler and matt-gadd April 28, 2017 11:49

rorticus requested a review from kitsonk April 28, 2017 11:55

dylans suggested changes Apr 29, 2017

View reviewed changes

dylans modified the milestones: 2017.04, 2017.05 Apr 29, 2017

kitsonk suggested changes May 1, 2017

View reviewed changes

rorticus force-pushed the issue-285 branch 2 times, most recently from edd9752 to b6749ed Compare May 5, 2017 14:27

kitsonk reviewed May 10, 2017

View reviewed changes

kitsonk approved these changes May 10, 2017

View reviewed changes

kitsonk suggested changes May 10, 2017

View reviewed changes

rorticus added 6 commits May 12, 2017 07:56

Adding upload progress reporting, issue dojo#285

a9c9ae4

Using observables for all request/response events, issue dojo#285

bce9996

Making SubscriptionPool queueing

9aa9f7d

More test coverage

d47f271

More tests

82b5884

Better coverage for QueuingEvented

aee8c77

rorticus force-pushed the issue-285 branch from 7b4cbb4 to aee8c77 Compare May 12, 2017 12:02

dylans approved these changes May 12, 2017

View reviewed changes

Updating test coverage

57e3335

kitsonk approved these changes May 12, 2017

View reviewed changes

rorticus merged commit a51adb2 into dojo:master May 12, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding upload progress reporting, issue #285 #326

Adding upload progress reporting, issue #285 #326

rorticus commented Apr 28, 2017

rorticus commented Apr 28, 2017

codecov bot commented Apr 28, 2017 •

edited

Loading

dylans left a comment

dylans Apr 29, 2017

kitsonk left a comment

kitsonk May 1, 2017

kitsonk May 1, 2017

kitsonk May 1, 2017

kitsonk May 10, 2017

kitsonk May 1, 2017

kitsonk May 1, 2017

rorticus commented May 5, 2017

rorticus commented May 5, 2017

kitsonk May 10, 2017

kitsonk May 10, 2017

kitsonk left a comment

rorticus commented May 12, 2017

dylans left a comment

rorticus commented May 12, 2017

kitsonk commented May 12, 2017

rorticus commented May 12, 2017

kitsonk left a comment

Adding upload progress reporting, issue #285 #326

Adding upload progress reporting, issue #285 #326

Conversation

rorticus commented Apr 28, 2017

rorticus commented Apr 28, 2017

codecov bot commented Apr 28, 2017 • edited Loading

Codecov Report

dylans left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kitsonk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rorticus commented May 5, 2017

rorticus commented May 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kitsonk left a comment

Choose a reason for hiding this comment

rorticus commented May 12, 2017

dylans left a comment

Choose a reason for hiding this comment

rorticus commented May 12, 2017

kitsonk commented May 12, 2017

rorticus commented May 12, 2017

kitsonk left a comment

Choose a reason for hiding this comment

codecov bot commented Apr 28, 2017 •

edited

Loading