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

fix: pool.exec(<function>) fails to emit message event to the pool. #439

Closed w1nklr closed 4 months ago

w1nklr commented 4 months ago

When using worker-pool in a browser environment, worker function registration does not work. Only pool.exec(<function>) can be run, but does not handle the on message callbacks.

This fix proposes to send the pool.exec options to the execution script.

Referenced doc: https://github.com/josdejong/workerpool#events Playground: https://codesandbox.io/p/sandbox/workerpool-in-a-react-project-forked-dgjxxc

Possible fix for issue #438

josdejong commented 4 months ago

Thanks @t0oF-azpn for reporting this issue and working out a fix!

Would it be possible to write a unit test to ensure we can't get any regressions in the future?

w1nklr commented 4 months ago

Thanks @t0oF-azpn for reporting this issue and working out a fix!

Would it be possible to write a unit test to ensure we can't get any regressions in the future?

I tried first to add/update the unit tests. But I didn't manage to make it run. In this particular case, we need to be in browser context. I tried to force to use web workers (var pool = createPool({ workerType: 'web' });), but this fails at worker creation in ensureWebWorker() function.

If there is any way to set the test to web browser context, I would take it ;)

josdejong commented 4 months ago

OK let's just merge this PR as is, ok? The fix makes sense.

I do think though that referring to worker in a standalone function is a bit of a tricky case: you're relying on an undocumented global variable exposed by a library. The neat solution would be to create a dedicated worker where you import worker explicitly.

w1nklr commented 4 months ago

I'm fine with merging This PR as is ! I don't have the rights to do it, though ;)

josdejong commented 4 months ago

👍 I'll merge and publish your PR first half of next week.

josdejong commented 4 months ago

I found a moment, just published v9.1.1 containing your fix. Thanks again!