using top level context to determine node vs browser environment

[sprockow]

Possible solution for:
#349

Ran into this issue because of my node environment's fetch polyfill. To get around this, I used a solution inspired by the solution proposed in this stack overflow question to fix this for myself.

[coveralls]

Pull Request Test Coverage Report for Build 495

• 3 of 3(100.0%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage decreased (-0.02%) to 83.898%

---

Totals
Change from base Build 494:	-0.02%
Covered Lines:	910
Relevant Lines:	1053

---

💛 - Coveralls

[sprockow]

So, this environment detection function is kind of hard to test. It's by definition very tightly coupled with the environment in runs on. It's written to avoid being mocked (so that a node environment with a mocked window global, like with jsdom, doesn't trigger a false positive browser detection).

I'm open to an argument as to how/why to test this function.

[tromey]

This looks reasonable to me, but it occurred to me that I didn't know whether it would work properly in the weird environment that the devtools run in. So, I think someone else ought to review it, like maybe @codehag

[codehag]

This looks good to me 👍

[loganfsmyth]

Another option here would be to use a replacement file for browser builds, e.g.

in package.json

"browser": {
"lib/read-wasm.js": "lib/read-wasm-browser.js"
}

then the alternate file would be used when the library is bundled with Webpack and such, and it would be up to the consumers of the package to choose the build target.

[sprockow]

@loganfsmyth - this seems like a much cleaner approach. I haven't done too much with npm publishing, so I'm unfamiliar with this environment specific configuration. Is there a good example/best practice of how webpack projects that target these sorts alternative builds?

[loganfsmyth]

Webpack has its target option and that trickles back to the default value for resolve.mainFields. Since Webpack defaults to "web" as it's target, the default behavior is to use the browser field to swap out files for alternatives.

I think it'd be a good approach to handle to existing way that this code works. That said, I do want to clarify that I have reservations in general about how this all works, since I think it is a little scary for a library to behave in two very different ways depending on how it is used, since a you have to pass the worker URL in one case but not the other. It might be nicer in the long run for the node build to use fs and the browser build to load a JS file containing the Base64-encoded WASM instead of having to use fetch, with fetch usage being a general opt-in behavior if people do want it still.