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

Improve the logic of isNode(). #421

Closed tamuratak closed 8 months ago

tamuratak commented 8 months ago

Improve the logic of isNode(). Close #320.

The trick of '[object process]' seems to be widely used.

Now, since the process.versions is empty on jspm-core, you could close the original issue without merging this PR.

josdejong commented 8 months ago

Thanks, this approach makes sense. How about removing the logic that checks .node.versions? I think that is redundant now. It can become just nodeProcess + "" === "[object process]", right ? Or String(nodeProcess) === "[object process]" to make it a bit more explicit.



> Now, since the `process.versions` is empty on `jspm-core`, you could close the original issue without merging this PR.

Do you mean that #320 _is_ already fixed without this PR?
tamuratak commented 8 months ago

How about removing the logic that checks .node.versions?

It would break workpool on the Electron renderer process, which has the built-in process although we should consider it as a browser platform.

Do you mean that #320 is already fixed without this PR?

Yes. This PR would be good for another polyfill library that will appear in the future.

josdejong commented 8 months ago

Ah, of course. In the isNodeJs function of pdf.js that you linked to there are also a couple of extra conditions to distinguish from Electron and NW I see. Ok let's keep the logic that we already had as is 👍 .

josdejong commented 8 months ago

Published now in v9.0.4, thanks again. I wish you nice holidays!