josdejong / workerpool

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

Improve logic to determina whether in a browser or nodejs environment #320

Closed FallingSnow closed 9 months ago

FallingSnow commented 3 years ago

When using this with jspm.org and in the browser, the library thinks it's in node.

This is because the library is webpacked before distribution. Webpack adds

import process from "process";

to the top of the environment.js file. JSPM then goes through each dependency and tries to pull it from npm. And unfortunately there is an NPM module named process which it pulls. That module exports process to the browser. So in the browser you see process.versions.node. Screenshot from 2021-09-12 15-14-29

At least that's how I understand it so far.

I think something more than just process should be relied on, or not at all. Perhaps self.naviagator? It has good support: https://caniuse.com/mdn-api_navigator

josdejong commented 3 years ago

Looks like by default the nodejs code is loaded. You can try to load the bundle instead: workerpool/dist/workerpool.min.js

FallingSnow commented 3 years ago

I believe it is in fact pulling the bundle. This is the file that it is running: https://ga.jspm.io/npm:workerpool@6.1.5/dist/workerpool.js

Screenshot from 2021-09-13 12-57-28

If you look in the image in my previous post (in the top right tab), the file I'm looking at in the browser is workerpool.js.

josdejong commented 3 years ago

Hm, so Webpack or jspm is being too smart here, adding this import process from "process"; which it shouldn't. So is this something on the jspm side, or something in your project?

The library is isomorphic, and it contains some checks to figure out in what environment you're running (nodejs or browser). It is checking whether process exists for that. But also, process is actually used in case that you're running on nodejs, and window too (not available in nodejs), so rewriting just this test is not enough I think.

FallingSnow commented 3 years ago

Sorry you're right, jspm is the one adding this. I tried using skypack.dev for this and it worked. However I would point out that skypack too tries to define process, just not in as much detail as jspm which is why it succeeds (it doesn't mock node enough).

Skypack workerpool.js, look for var process =: https://cdn.skypack.dev/-/workerpool@v6.1.5-HL7HFWYxDYHc5NxiG5I3/dist=es2020,mode=imports/optimized/workerpool.js

So given that, I would suggest a positive and negative test for detecting environment. So to tell if we are in the browser we would test that something only node would have isn't there or something only the browser would have is there. Something like

if (!globalThis.process || globalThis.navigator) {
  // browser
}
else if (globalThis.process || !globalThis.navigator) {
  // node
}
else {
  // unknown
}

I've tested the above in chromium 93 and node and it works as expected.

But also, process is actually used in case that you're running on nodejs, and window too (not available in nodejs), so rewriting just this test is not enough I think.

If the test returns that process is not available then you should not be using it, same for window. Can you expand on this?

josdejong commented 2 years ago

Yes, would be great to improve determining the environment. We need to be careful there though (so many environments, and so many bundlers doing smart stuff on top of that when bundling things).

Anyone able to look into improving the code to determine whether running in the browser or nodejs? Help would be welcome.