Add progress reporting #9

YurySolovyov · 2016-07-14T11:09:47Z

API is likely to change, submitting PR to get some feedback.
Related to sindresorhus/cpy#33

YurySolovyov · 2016-07-14T11:13:01Z

Tests pass locally without xo, and there are some errors even in clean repo.

YurySolovyov · 2016-07-14T11:18:24Z

index.js

+					var remains = progress.stat.size - write.bytesWritten;
+					var percent = ((written / progress.stat.size) * 100).toPrecision(2);
+					opts.onProgress({
+						file: src,


maybe add both files here like src + dest ?

👍 src and dest instead of file

schnittstabil · 2016-07-14T11:26:39Z

index.js

-	return new Promise(function startRead(resolve, reject) {
-		var read = fs.createReadStream(src);
+function createReadPromise(src) {
+	var read = fs.createReadStream(src);


Move this back into the Promise constructor argument (line 26), because fs.createReadStream might throw some error (maybe in the future), e.g.
fs.createReadStream('/dev/null', {flags: 'unicorns'})

schnittstabil · 2016-07-14T11:39:54Z

Tests pass locally without xo, and there are some errors even in clean repo.

xo is undergoing continual improvements, some style violations are introduced by your PR though

YurySolovyov · 2016-07-14T11:41:07Z

I think I can fix all of them, since I'm here anyway.

schnittstabil · 2016-07-14T11:50:38Z

remains and percent, are a nice ideas. However, I think remains isn't necessary in most use cases, and percent isn't very useful: some require values between 0 and 1, some need a different precision and some don't care for percent at all. – Personally, I would drop both.

YurySolovyov · 2016-07-14T11:53:01Z

@schnittstabil I'd kept percent, but made it between 0 and 1.
Also what do you think of #9 (comment)

schnittstabil · 2016-07-14T11:57:03Z

index.js

+						file: src,
+						written: written,
+						remains: remains,
+						percent: percent


size would be nice, to calculate the total e.g. in cpy – or eion of course

that would also allow to drop remains, since that would be very easy to calc it yourself.

YurySolovyov · 2016-07-14T17:14:40Z

Updated. Will take a look at 0.10 failure a bit later.

YurySolovyov · 2016-07-14T18:03:03Z

I have no idea what happens with 0.10 to be honest. Seems like something with encodings?

YurySolovyov · 2016-07-15T08:35:34Z

@sindresorhus @schnittstabil fixed 0.10 by using readable-stream module to wrap old-style streams.
I think this is due to how old streams treat adding data events.

sindresorhus · 2016-07-16T18:25:46Z

index.js

+	return fsP.stat(src).then(function (stat) {
+		progress.stat = stat;
+	}).catch(function (err) {
+		Promise.reject(new CpFileError('NO_ENTRY', err));


This isn't doing anything as it's not returned. Needs a regression test for that.

The correct solution is to just throw the error.

YurySolovyov · 2016-07-16T19:36:42Z

@sindresorhus now that I tried this, I would say that maybe EventEmitter idea was better. Do you have any preference here?

sindresorhus · 2016-07-16T20:29:27Z

I would personally prefer EventEmitter as it's more Node'y.

YurySolovyov · 2016-07-17T07:45:41Z

@sindresorhus Re-implemented with EventEmitter.Though it is not really pretty either.

I wonder if we can somehow make return values of .on and .then to return object that has both EventEmitter and Promise methods so we can nicely chain it.

Edit: something like carrack?

schnittstabil · 2016-07-17T08:29:56Z

index.js

@@ -80,6 +119,10 @@ module.exports = function (src, dest, opts) {
 			});
 		}
 	});
+
+	promise.events = progress.emitter;


As far as I understand, @sindresorhus suggested sth. like (sindresorhus/cpy#33 (comment)):

promise.on = progress.emitter.on.bind(progress.emitter);

So, one can use it that way:

const p = cpFile(…); p.on(…); p.then(…)

We may also do this:

promise.on = function () { progress.emitter.on.apply(progress.emitter, arguments); return promise; };

So, one can use it this way:

cpFile(…) .on(…) .then(…); // but not this way (!) cpFile(…) .then(…) .on(…);

YurySolovyov · 2016-07-17T16:51:30Z

Implemented @schnittstabil suggestions.

schnittstabil · 2016-07-18T08:50:53Z

index.js

+	var progress = {
+		stat: null,
+		emitter: new EventEmitter()
+	};


Hence we have cpFile.on(…), stat and emitter aren't intended only for progress stuff anymore.
We should drop progress.… by:

var stat = null; var eventEmitter = new EventEmitter();

but if I drop stat event, it would be the only for progress, right?

Yes, but IMO it is easier to read and to maintain, e.g. we may emit other events in the future...

As we shouldn't reuse stat for updating the access-times etc., we may use only the size:

- var stat = null; + var size = null; // … var promise = fsP.stat(src).then(function (stat) { + size = stat.size;

maybe init it with 0 then? :)

I should have thought first – the right thing would be simply:

var size;

0 means the file is empty

null means we don't care

undefined means the size is not yet known

@schnittstabil while this is nice, I have question if we want to optimize for this at all, is 1 stat call worth additional branching and checking? I'm ok with either answer, just wanted to raise a question. Even if it is, we might be able to avoid it in some other way.

I've thought about that, but we have:

cpFile(…) // returns the promise .on(…) // but the listeners are added afterwards

Therefore, a workaround to avoid the stat call would be rather awkward IMO, but if you have an idea…

that way, if we have to do stat anyway, why check?

, why check?

I do not know if I have understood you correctly.

I think we should always fs.lstat to determine the size. The alternative would be to postpone the call until a listener is added, which I think is too cumbersome.

Checking emitter.listenerCount('progress') > 0 before emitting is double workload, emitter.emit does a similar check.

Checking size !== null before emitting was based on a misconception (mentioned below), which have worked well in case of opts.onProgress 😒

Hence, I don't think we should check before emitting progress at all.

… progress event

YurySolovyov · 2016-07-18T18:27:16Z

@schnittstabil Updated.

Use write.on('close'). Actually works, thanks.
Report progress for empty files only on end.
Drop naive EE optimization

schnittstabil · 2016-07-21T07:33:54Z

@YurySolovyov Sorry for the delay. LGTM, only documentation of .on(…) in readme.md remains.

YurySolovyov · 2016-07-21T08:04:39Z

@schnittstabil Added docs

schnittstabil · 2016-07-21T08:23:18Z

@sindresorhus Do you have further suggestions?

sindresorhus · 2016-07-22T14:47:00Z

index.js

+					dest: dest,
+					size: size,
+					written: written,
+					percent: percent


I think we should round this to 2 decimals. E.g. 0.45.

What if user wants a better precision?
Say if you have a progress bar with value in pixels, and you calc done value like

const done = progress * fullWidth;

Given that file can be big, you can have multiple progress events that rounds for same 0.45 value, making it stay at same position during multiple progress events.

Yeah, sure, not a big deal.

YurySolovyov · 2016-07-22T18:59:23Z

@sindresorhus Updated.

sindresorhus · 2016-07-22T20:28:27Z

Awesome! Thanks for working on this @YurySolovyov. It turned out great :)

YurySolovyov · 2016-07-22T20:50:07Z

@sindresorhus and @schnittstabil, thanks for your patience!

sindresorhus · 2016-07-22T20:51:12Z

3.2.0 published. Now on to cpy ✨

schnittstabil · 2016-07-23T08:27:52Z

Thanks a lot for your thorough work @YurySolovyov!

Add progress reporting

b408aaa

YurySolovyov reviewed Jul 14, 2016
View reviewed changes

Do not nest tests

6a08f25

schnittstabil reviewed Jul 14, 2016
View reviewed changes

Fix review and eslint

795efa2

Fix 0.10.x by using readable-stream module.

ae817a1

sindresorhus reviewed Jul 16, 2016
View reviewed changes

Fix more review

0cb6740

YurySolovyov added 2 commits July 17, 2016 10:37

Use EventEmitter for reporting progress

be46e13

Fix method context

61f61b4

schnittstabil reviewed Jul 17, 2016
View reviewed changes

Make API more fluent

e627ac8

schnittstabil reviewed Jul 18, 2016
View reviewed changes

Drop naive EE optimization. Use write.on('close', ...) event for last…

ba95304

… progress event

Add docs for progress reporting

a9e51b7

sindresorhus reviewed Jul 22, 2016
View reviewed changes

Use absolute paths for progress events

d672fa5

sindresorhus merged commit 25a281c into sindresorhus:master Jul 22, 2016

YurySolovyov deleted the progress branch July 22, 2016 20:51

Add progress reporting #9

Add progress reporting #9

Conversation

YurySolovyov commented Jul 14, 2016

YurySolovyov commented Jul 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schnittstabil commented Jul 14, 2016

YurySolovyov commented Jul 14, 2016

schnittstabil commented Jul 14, 2016

YurySolovyov commented Jul 14, 2016

schnittstabil Jul 14, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YurySolovyov commented Jul 14, 2016

YurySolovyov commented Jul 14, 2016

YurySolovyov commented Jul 15, 2016

Choose a reason for hiding this comment

YurySolovyov commented Jul 16, 2016

sindresorhus commented Jul 16, 2016

YurySolovyov commented Jul 17, 2016 • edited

Choose a reason for hiding this comment

schnittstabil Jul 17, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YurySolovyov commented Jul 17, 2016

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

schnittstabil Jul 18, 2016 • edited

Choose a reason for hiding this comment

YurySolovyov commented Jul 18, 2016

schnittstabil commented Jul 21, 2016

YurySolovyov commented Jul 21, 2016

schnittstabil commented Jul 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YurySolovyov commented Jul 22, 2016

sindresorhus commented Jul 22, 2016

YurySolovyov commented Jul 22, 2016

sindresorhus commented Jul 22, 2016

schnittstabil commented Jul 23, 2016

schnittstabil Jul 14, 2016 •

edited

YurySolovyov commented Jul 17, 2016 •

edited

schnittstabil Jul 17, 2016 •

edited

schnittstabil Jul 18, 2016 •

edited