adds worker-side on(message) events

[flipswitchingmonkey]

This PR is related to #370

A rather small change that adds a second parameter to workerpool.worker() function, which accepts an object where each key is the receiver's name of an event, plus a payload.

It also adds an emit() function to the Promise object which will send a postMessage() call to the worker instance.

These changes should be non-breaking.

Example:
workers/myworker.js

workerpool.worker({
  fibonacci: fibonacci,
}, {
  callme: function (payload) {
    makeThingsHappen(payload);
  }
});

And to call the event:

const pool = new Pool(__dirname + "/workers/myworker.js");

// main difference here is that the handler, a Promise, is picked up from the exec() command first
const handler = pool.exec('fibonacci', [15]);

// now continue the chaining
handler
  .then(function (result) {
    assert.strictEqual(result, 610);
  })
  .catch(function (err) {
  })
  .then(function () {
    pool.terminate(); // terminate all workers when done
  })
  
// the Promise now has a function Promise.emit(name, paramerers) 
handler.emit('callme', {say: 'O hai!'});

[josdejong]

Thanks, this looks really promising @flipswitchingmonkey! I like the workerpool.worker(methods, events) API, that makes sense.

I made a couple of inline comments, can you have a look at those?

And lastly, we should document the new feature in the README.md.

[josdejong]

I have the feeling that the unit test testing the behavior can be simplified a bit, rather than indirectly testing for the worker killing itself as a result of receiving a message. How about changing it to something more direct like:

const handler = pool.exec('getEmittedMessage', [])
  .then(function (result) {
    assert.strictEqual(result, 'hello world')
  })
    
handler.emit('myCustomEvent', 'hello world')

What do you think?

[josdejong]

Right now, an emitted event will be silently ignored when a worker is not yet attached (i.e. the task is queued). I think it would be better to throw an exception, or queue the message and execute. Or maybe return a boolean indicating whether the message was sent or not. What do you think?

[josdejong]

Can you rename this to function (eventName, eventParams) { ?

[josdejong]

Personal preference: I find the name addOnEvents a bit confusing. How about naming it registerEvents or registerEventListeners for example?

[josdejong]

Instead of adding a new listener worker.on('message', ...) for every event, it maybe a a good idea to extend the existing worker.on('message', function (request) { ... } instead, so all the logic for handling incoming messages is in one place, so you can clearly see that it is matching methods and events.

We could restructure the handler to do that explicitly, like

worker.on('message', function (request) { 
  try {
    if (isMethod(request)) {
      onMethod(request.method, request.params)
    } else if (isEvent(request)) {
      onEvent(request.eventName, request.eventParams)
    } else {
      // throw error
    }
  } catch (err) { 
    // ...
  }
}

What do you think?

[blordpluto]

The functionality here is crucial for my team to use this package. This PR appears to have been abandoned, and now unsurprisingly conflicts greatly. @josdejong what would be your level of interest in re-approaching this?

[josdejong]

@blordpluto thanks for your input. I would love to see this feature finished. The PR was under review but not yet there. Would you be interested in creating a new PR implementing this feature?