Add progress events #322

vadimdemedes · 2017-06-01T16:36:56Z

This PR adds upload and download progress events.

got.post('https://website.com', {body: fs.createReadStream('file')})
  .on('uploadProgress', progress => {
    // `progress` looks like:
    // {
    //   percent: 0.24,
    //   transferred: 24518,
    //   total: 50000
    // }
    }
  })
  .on('downloadProgress', progress => {
    // `progress` has the same shape as in `uploadProgress` event
  });

Initial upload/download progress event with percent equal to 0 is always emitted. If it's not possible to retrieve the total upload/download body size, percent and size.total are equal to null.

Hack for receiving upload progress events when uploading buffers or strings

When sending Buffer as body, there are only 2 uploadProgress events, because the whole buffer gets written to the connection, so req.connection.bytesWritten becomes equal to buffer length. To work around this limitation and receive meaningful progress events, intoStream module is used to convert buffer into stream.

One more quirk. Since after conversion, there's no more access to buffer's size, I'm making it accessible by assigning it to _buffer on the stream here https://github.com/sindresorhus/got/pull/322/files#diff-168726dbe96b3ce427e7fedce31bb0bcR525 and getting it later again here https://github.com/sindresorhus/got/pull/322/files#diff-168726dbe96b3ce427e7fedce31bb0bcR76.

TODO

Write a description for this PR
Explain the hack behind wrapping body buffer into a stream when uploading
Update readme

sindresorhus · 2017-07-01T23:36:17Z

index.js

+						transferred: uploaded,
+						total: uploadBodySize
+					});
+				}, 150);


Why did we go with 150? Can you move 150 into a variable with a descriptive name of why it's 150.

We just picked a random number for frequency of upload progress events.

Should definitely be clearer in the code then. 150 is very specific, wonder why we didn't just go with 100.

sindresorhus · 2017-07-01T23:41:52Z

index.js

+		}
+
+		if (isFormData(body)) {
+			body.getLength((err, size) => {


Would be nicer to use pify here and on line 66, so we don't have to wrap the whole thing in the Promise constructor.

sindresorhus · 2017-07-01T23:43:13Z

index.js

+		getBodySize(opts)
+			.then(size => {
+				uploadBodySize = size;
+


Remove the empty line here.

sindresorhus · 2017-07-01T23:43:43Z

index.js

+
+	promise.on = (name, fn) => {
+		proxy.on(name, fn);
+


Remove the empty line here.

sindresorhus · 2017-07-01T23:44:02Z

index.js

@@ -388,7 +568,7 @@ function got(url, opts) {
 	}
 }

-got.stream = (url, opts) => asStream(normalizeArguments(url, opts));
+got.stream = (url, opts) => asStream(Object.assign({}, normalizeArguments(url, opts), {stream: true}));


Would be better to do the Object.assign thing inside the asStream function.

sindresorhus · 2017-07-01T23:45:21Z

test/progress.js

+		t.is(lastEvent.percent, 0);
+	}
+
+	events.forEach((event, index) => {


for (const [index, event] of events.entries()) {

sindresorhus · 2017-07-01T23:51:02Z

test/progress.js

+		.on('end', () => {
+			checkEvents(t, events);
+			t.end();
+		});


You could use https://github.com/sindresorhus/get-stream here to be able to use async/await instead of test.cb.

sindresorhus · 2017-07-01T23:51:59Z

test/progress.js

+	body.append('key', 'value');
+	body.append('file', file);
+
+	const size = await pify(body.getLength.bind(body))();


pify binds to the original object by default, so the bind here is moot I think.

Nope, it fails without bind.

vadimdemedes · 2017-07-02T08:23:48Z

Thanks @sindresorhus, feedback addressed. Had to rebase and force push, because of a conflict.

sindresorhus · 2017-07-02T09:43:58Z

@vadimdemedes Thanks for the quick turn-around.

sindresorhus · 2017-07-02T09:45:34Z

Can you apply #322 (comment) to the other test.cb tests too?

vadimdemedes · 2017-07-02T10:57:53Z

@sindresorhus Oh yeah, missed the other ones.

kevva · 2017-07-06T10:55:41Z

readme.md

@@ -202,6 +203,21 @@ got.stream('github.com')

 `redirect` event to get the response object of a redirect. The second argument is options for the next request to the redirect location.

+##### .on('uploadProgress', progress)
+##### .on('downloadProgress', progress)


Should probably clarify that these can be used with the Promise interface too. It's a little ambiguous now since they're documented under got.stream.

sindresorhus · 2017-07-15T16:32:21Z

@alextes @floatdrop I'll merge this on monday unless it gets any objections by then ;)

SamVerschueren · 2017-07-15T17:56:17Z

index.js

+	const body = opts.body;
+
+	if (opts.headers['content-length']) {
+		return Number(opts.headers['content-length']);


Just generally interested what the benefit is of wrapping this in a Number object?

It's not wrapping it in a Number object, it's coercing it to a number.

Any benefit over parseInt? I never used Number directly so that's why I'm asking :).

Mostly just consistency. I also use Boolean() to coerce something to a Boolean. Same with String().

SamVerschueren · 2017-07-15T17:58:26Z

index.js

+			clearInterval(progressInterval);
+
+			ee.emit('uploadProgress', {
+				percent: 1,


Why is percent set to 1? Probably missing something here :).

I'm also wondering why it is emitted? If some wants to calculate the percentage, he can easily do transferred / total. I would rather not have it emitted, but that might just be me :).

Is percent emitted because total can be null when streaming?

Why is percent set to 1? Probably missing something here :).

It's calculating percent to be from 0 to 1. So 1 means it's the last upload progress event and it's done.

I'm also wondering why it is emitted?

Initially it was only for convenience. But there are also cases where total can be unavailable (like you said, when streaming), so got makes sure that percent is always available.

SamVerschueren · 2017-07-15T18:03:39Z

readme.md

+
+```js
+{
+	percent: 0.24,


1024 bytes send against 10240 in total means 10% and thus percent should be 0.1.

sindresorhus · 2017-07-23T12:37:30Z

Yay! Vadim, your sambuca-powered work is finally merged. Thanks for working on it. Good times.

sindresorhus · 2017-07-23T12:38:18Z

@floatdrop @alextes Feel free to add your review still. I'm not gonna do a release yet.

kevva · 2017-07-23T14:43:04Z

alextes · 2017-07-23T16:33:44Z

index.js

+					const headersSize = Buffer.byteLength(req._header);
+					uploaded = req.connection.bytesWritten - headersSize;
+
+					// Prevent the known issue of `bytesWritten` being larger than body size


@vadimdemedes if you have a link to an issue explaining what exactly is amiss and how simply ignoring any bytes uploaded beyond the body size is the right solution. Do share, I'd be happy to add it to the comment. If you don't let me know and I'll go looking myself.

There's no explanation, I just observed this bug randomly and couldn't find a source of the issue.

alextes · 2017-07-23T16:42:49Z

index.js

+			});
+
+			req.connection.on('connect', () => {
+				const uploadEventFrequency = 150;


Just curious, is there a reason behind this number? 6 2/3 times a second?

No reason. It was just a guess on how to report progress events not too frequently, but still enough.

alextes · 2017-07-23T16:57:22Z

index.js

+
+				progressInterval = setInterval(() => {
+					const lastUploaded = uploaded;
+					const headersSize = Buffer.byteLength(req._header);


Can header size change during upload? Otherwise, I don't see the point of putting this inside the interval or even the on 'connect'.

It can't change during upload, but there's no way to know when headers are sent, is there? So I placed it here to correctly calculate the body size.

vadimdemedes · 2017-07-24T21:20:35Z

@sindresorhus haha, best times!

I don't know why, but when I searched "sambucca" on Giphy, this is what it gave me, so now I'm posting this instead of my favorite drink to celebrate the merge:

marianoguerra · 2017-08-21T09:11:51Z

is there any plan to make a release with this change?

sindresorhus · 2017-08-21T09:25:12Z

Yes

brings npm pkg from 0.75MB to 0.45MB after yarn autoclean --force, down to 0.25MB from this PR: sindresorhus#284 next big savings is removing progress, from sindresorhus#322

vadimdemedes force-pushed the progress-events branch 2 times, most recently from 63d6189 to 7af37a0 Compare June 1, 2017 18:42

vadimdemedes changed the title ~~[WIP] Add progress events~~ Add progress events Jun 1, 2017

sindresorhus mentioned this pull request Jun 2, 2017

Use mimic-response sindresorhus/clone-response#1

Closed

vadimdemedes force-pushed the progress-events branch from 3f4dd94 to 6fe4220 Compare June 18, 2017 08:27

sindresorhus requested changes Jul 1, 2017

View reviewed changes

sindresorhus requested review from floatdrop, kevva, SamVerschueren and alextes July 1, 2017 23:54

Vadim Demedes and others added 10 commits July 2, 2017 11:00

add progress events

da96bb5

support progress events for got.stream

d1d3c80

reshape progress events

177cfd6

improvements

ca6a1af

update readme

ad1dc2d

fix downloading after decompress PR merging

4663e5f

use mimic-response

8fc75ab

Update readme.md

285f95b

Update readme.md

a96067a

fix feedback

88fc78b

vadimdemedes force-pushed the progress-events branch from f0b3a3c to 88fc78b Compare July 2, 2017 08:21

minor changes

b12672f

fix feedback

24b8381

kevva reviewed Jul 6, 2017

View reviewed changes

SamVerschueren reviewed Jul 15, 2017

View reviewed changes

Vadim Demedes and others added 2 commits July 22, 2017 13:08

fix feedback

c8745fd

Update readme.md

24eda21

sindresorhus approved these changes Jul 23, 2017

View reviewed changes

sindresorhus merged commit be7b802 into sindresorhus:master Jul 23, 2017

alextes reviewed Jul 23, 2017

View reviewed changes

vadimdemedes deleted the progress-events branch July 24, 2017 21:16

lukechilds mentioned this pull request Nov 27, 2017

Uncaught TypeError: Cannot read property 'once' of undefined at EventEmitter.ee.once.req #427

Closed

Vivalize mentioned this pull request May 31, 2021

uploadProgress for non-stream bodies #1735

Closed

2 tasks

Add progress events #322

Add progress events #322

Conversation

vadimdemedes commented Jun 1, 2017 • edited Loading

Hack for receiving upload progress events when uploading buffers or strings

TODO

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

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

vadimdemedes commented Jul 2, 2017 • edited Loading

sindresorhus commented Jul 2, 2017 • edited Loading

sindresorhus commented Jul 2, 2017

vadimdemedes commented Jul 2, 2017

Choose a reason for hiding this comment

sindresorhus commented Jul 15, 2017

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

Choose a reason for hiding this comment

sindresorhus commented Jul 23, 2017

sindresorhus commented Jul 23, 2017

kevva commented Jul 23, 2017

alextes Jul 23, 2017 • edited Loading

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

vadimdemedes commented Jul 24, 2017

marianoguerra commented Aug 21, 2017

sindresorhus commented Aug 21, 2017

vadimdemedes commented Jun 1, 2017 •

edited

Loading

vadimdemedes commented Jul 2, 2017 •

edited

Loading

sindresorhus commented Jul 2, 2017 •

edited

Loading

alextes Jul 23, 2017 •

edited

Loading