Adding progress

[mmcardle]

The purpose of this PR is to allow progress reports from jobs when they are executed and also when they are submitted to a Pool.

• Check if we need to remove the progress listener when the job is destroyed
• Provide summary of changes
• Add docs
• Add more tests for checking progress

[andywer]

The thread.off('progress') approach can be improved.

[andywer]

Might be better to remove the progress listener after being done, especially for garbage collection. Could do that using thread.promise().then(() => thread.off('progress'), () => thread.off('progress')).

But the thread.off('progress') behavior might cause side effects, since it does not only remove the listeners we set here in the job, but all others as well. thread.removeListener('progress', this.onProgress) would be preferable, but we would also need to move this.emit('progress', e) to onProgress and bind it beforehand.

[andywer]

Should use either arrow function or .bind() consistently here across all .on()/.once()

[andywer]

Hey @mmcardle.

Thanks for your PR! The idea is to make the job propagate the thread's progress event, right?

I reviewed the changes and identified an edge case. Nothing too serious, but might be worth looking into :)

[mmcardle]

thanks @andywer I shall give it a look

[mmcardle]

@andywer I have pushed a few changes, trying to get my head around the event emitter framework. Also updated the PR text so its clear what I am trying to achieve. Thanks

Edited a quick fix. Let's try that.

[andywer]

Hey @mmcardle.

I applied an easy fix and I hope that does the trick. Have a look at it and let's see if the tests are green, since I just edited here on github...

Thanks for your efforts so far :)

That event emitter / callback stuff can be really nasty. That's why I am already planning for a version 1.0 built on top of observables 😉

[mmcardle]

Hi @andywer Thats looks good, it functions correctly in my application, tests are red though, ill see if i can see the issue.

[mmcardle]

@andywer That last commit fixed the tests, do you want me to update README/CHANGELOG with changes?

[andywer]

Neat! 😊

Yeah, you can add a section ## Next release with a bullet point of your change if you want. Feel free to link to yourself like I did with previous contributors. Otherwise I will edit the changelog later.

Is there anything to update in the readme? Planning to merge & publish this today or tomorrow.

[andywer]

I am actually quite ok with the PR. There is just one thing I'd like to see:

An additional test case for Job, testing that the progress event is actually propagated.

FYI: Just opened another PR (#58) that adds support for async thread functions, in case you are interested. Promises are nicer than callbacks :)

[mmcardle]

I shall take a look at adding that test, and yes promises all the way!

[mmcardle]

@andywer updated

[andywer]

Ahh, did you still want to update the docs? Now I merged it already...

[mmcardle]

Ah no thats great! thanks for all your help!

[andywer]

Published as v0.8.0

[mmcardle]

@andywer just checking the npm build updated correctly? i have a few diffs between a local build and the one downloaded from npm

432c432,434
<     var _thread$once$once$run, _thread$once$once;
---
>     var _this2 = this,
>         _thread$on$once$once$,
>         _thread$on$once$once;
434c436,460
<     (_thread$once$once$run = (_thread$once$once = thread.once('message', this.emit.bind(this, 'done')).once('error', this.emit.bind(this, 'error'))).run.apply(_thread$once$once, this.runArgs)).send.apply(_thread$once$once$run, this.sendArgs);
---
>     var onProgress = function onProgress() {
>       for (var _len3 = arguments.length, args = Array(_len3), _key3 = 0; _key3 < _len3; _key3++) {
>         args[_key3] = arguments[_key3];
>       }
>
>       return _this2.emit.apply(_this2, ['progress'].concat(args));
>     };
>     var onMessage = function onMessage() {
>       for (var _len4 = arguments.length, args = Array(_len4), _key4 = 0; _key4 < _len4; _key4++) {
>         args[_key4] = arguments[_key4];
>       }
>
>       _this2.emit.apply(_this2, ['done'].concat(args));
>       thread.removeListener('progress', onProgress);
>     };
>     var onError = function onError() {
>       for (var _len5 = arguments.length, args = Array(_len5), _key5 = 0; _key5 < _len5; _key5++) {
>         args[_key5] = arguments[_key5];
>       }
>
>       _this2.emit.apply(_this2, ['error'].concat(args));
>       thread.removeListener('progress', onProgress);
>     };
>
>     (_thread$on$once$once$ = (_thread$on$once$once = thread.on('progress', onProgress).once('message', onMessage).once('error', onError)).run.apply(_thread$on$once$once, this.runArgs)).send.apply(_thread$on$once$once$, this.sendArgs);
442c468
<     var _this2 = this;
---
>     var _this3 = this;
447,449c473,475
<       if (!_this2.thread) {
<         _this2.once('threadChanged', function () {
<           resolve(_this2.thread.promise());
---
>       if (!_this3.thread) {
>         _this3.once('threadChanged', function () {
>           resolve(_this3.thread.promise());
452c478
<         resolve(_this2.thread.promise());
---
>         resolve(_this3.thread.promise());

[andywer]

Yeah, that does look fishy. My local transpiled files look fine, though:

  Job.prototype.executeOn = function executeOn(thread) {
    var _this2 = this,
        _thread$on$once$once$,
        _thread$on$once$once;

    var onProgress = function onProgress() {
      for (var _len3 = arguments.length, args = Array(_len3), _key3 = 0; _key3 < _len3; _key3++) {
        args[_key3] = arguments[_key3];
      }

      return _this2.emit.apply(_this2, ['progress'].concat(args));
    };
    var onMessage = function onMessage() {
      for (var _len4 = arguments.length, args = Array(_len4), _key4 = 0; _key4 < _len4; _key4++) {
        args[_key4] = arguments[_key4];
      }

      _this2.emit.apply(_this2, ['done'].concat(args));
      thread.removeListener('progress', onProgress);
    };
    var onError = function onError() {
      for (var _len5 = arguments.length, args = Array(_len5), _key5 = 0; _key5 < _len5; _key5++) {
        args[_key5] = arguments[_key5];
      }

      _this2.emit.apply(_this2, ['error'].concat(args));
      thread.removeListener('progress', onProgress);
    };

    (_thread$on$once$once$ = (_thread$on$once$once = thread.on('progress', onProgress).once('message', onMessage).once('error', onError)).run.apply(_thread$on$once$once, this.runArgs)).send.apply(_thread$on$once$once$, this.sendArgs);

    this.thread = thread;
    this.emit('threadChanged');
    return this;
  };

Very strange. I will have a look at that :-/

[andywer]

PS: Thanks for pointing out!

[andywer]

Observation 1:
The published transpiled files lack the progress fix, but the promise feature is there.

Observation 2:
Just did a test npm publish (version 0.8.0-publish-test, tag publish-test) and that looks fine now.

So I think I will just re-publish as 0.8.1 and it should be fine. But I really can't tell how something like this is even possible, esp. since there is the prepublish hook that runs babel and the tests automatically right before publishing.

[andywer]

Re-published as v0.8.1. Can you confirm that it works now?

[mmcardle]

@andywer that has fixed it, thanks!