search

josdejong / workerpool

Offload tasks to a pool of workers on node.js and in the browser
Apache License 2.0
2.04k stars 148 forks source link

feat: allow for capture of stdout/stderr from worker #425

Closed cpendery closed 7 months ago

cpendery commented 7 months ago

This PR adds support for capturing a worker's stdout/stderr during a task and providing it as messages back from the worker. This is necessary because there is no supported way in Node.js to capture stdout/stderr from the worker's side.

Example use case: A test worker that doesn't want to display any stdout during the test execution, but instead display it in an error view if the test fails.

Closes #423

josdejong commented 7 months ago

Thanks, this looks neat and well tested!

A few feedbacks:

  1. Can you describe the new options in the README.md? (I see you added them to the TypeScript types already 👌)
  2. I think it would be helpful to create a small example in the example directory demonstrating how to use these options, can you create that?
  3. I'm not sure if there are tests covering the case that emitStdout and emitStdError are false or not provided. If indeed so, is there a way to test that?
  4. A thought: is there actually a use case where you would want to configure emitStdout !== emitStdError? If not, we could unify them in a single option?
cpendery commented 7 months ago

Thanks, this looks neat and well tested!

A few feedbacks:

  1. Can you describe the new options in the README.md? (I see you added them to the TypeScript types already 👌)
  2. I think it would be helpful to create a small example in the example directory demonstrating how to use these options, can you create that?
  3. I'm not sure if there are tests covering the case that emitStdout and emitStdError are false or not provided. If indeed so, is there a way to test that?
  4. A thought: is there actually a use case where you would want to configure emitStdout !== emitStdError? If not, we could unify them in a single option?

1/2. Sounds good, I'll add those! 👍

  1. I can add some specific cases for that
  2. I can't think of a use case for that, since they are already returned as separate events, so I think unifying them makes sense. I'll put them together under emitStdStreams
josdejong commented 7 months ago

Thanks Chapman, this looks good 👍!

I'll merge and publish your new feature now.

josdejong commented 7 months ago

Published now in v9.1.0. Thanks again!