Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: workerEmit, closes #51 #227

Merged
merged 10 commits into from
Jan 31, 2021
Merged

feat: workerEmit, closes #51 #227

merged 10 commits into from
Jan 31, 2021

Conversation

Akryum
Copy link
Contributor

@Akryum Akryum commented Jan 18, 2021

Closes #51

Try it by installing yarn add @akryum/workerpool instead of workerpool.

import { worker, workerEmit } from '@akryum/workerpool'

function sendEvent() {
  return new Promise(function (resolve) {
    workerEmit('test-event', {
      foo: 'bar',
    })
    resolve('done')
  })
}

// create a worker and register some functions
worker({
  sendEvent,
})
const pool = new Pool(__dirname + '/workers/emit.js')

pool.exec('sendEvent', [], {
  on: (eventType, payload) => {
    if (eventType === 'test-event') {
      // Do something
      console.log(payload)
    }
  }
})
.then(function (result) {
  // Done
})

@josdejong
Copy link
Owner

Thanks @Akryum , I'll look into your PR in detail soon. Looks very good at first sight 👍

@josdejong
Copy link
Owner

Thanks @Akryum, I'm really happy with this PR. It looks very neat and straightforward. Using the global variable currentRequestId feels a bit tricky, but I also do not see an other solution.

A few feedbacks:

  1. Can you describe the new functionality in the README.md? With a minimal example maybe.
  2. Currently, two arguments are passed to the on callback on: function (eventType, payload) {...}. Since the workerpool isn't really doing anything itself with eventType, it may be even simpler to just pass payload. You can pass and object containing eventType and other stuff as payload if you want. What do you think?
  3. There are now two types of messages send from the worker: results of a request, and emitted events. It is a bit implicit which of the two is the case, this is now checked as response.eventType != null, so as user I could trick the system by sending workerEmit(null, ...). I think it would be good pass an explict property instead of relying on eventType, something like response.isEvent === true.

@Akryum
Copy link
Contributor Author

Akryum commented Jan 28, 2021

@josdejong Should be good now

@josdejong
Copy link
Owner

Thanks for the updates, the docs are really clear and complete, nice job 👍

Merging your PR now.

@josdejong josdejong merged commit 3e87faf into josdejong:master Jan 31, 2021
@josdejong
Copy link
Owner

Available now in v6.1.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Send progress/status information from worker to main process
2 participants