Fixes #16913: Don't use fetch() in Node.js environment

[simnalamburt]

Node.js v18.1.0 has implemented WebAssembly.instantiateStreaming API, and it broked all emscriptened js codes since emscripten is using WebAssembly.instantiateStreaming as a criterion to distinguish whether it is Node.js environment or a browser environment.

The issue will be fixed if we use ENVIRONMENT_IS_NODE as a proper criterion to distinguish Node.js environment.

Fixes #16913, antelle/argon2-browser#81.

References

• lib,src: implement WebAssembly Web API nodejs/node#42701
• Fails with Node.js v18.1.0 antelle/argon2-browser#81
• https://github.com/simnalamburt/argon2-browser-error-repro
• Node.js implemented WebAssembly.instantiateStreaming in v18.1.0 #16913

[kripken]

Interesting, thanks for filing...

I'd hope we can actually use instantiateStreaming if node implemented it, but looks like in #16913 that the blocker is that node's fetch does not work yet. Sounds good to avoid that path for now.

[kripken]

Also please add a comment, as we'd like to remove this workaround some day. Perhaps something like this:

// Avoid instantiateStreaming() on node for now, as while newer node implements it,
// it does not have a full fetch() implementation yet.
// [..link to emscripten and/or node bug..]

[simnalamburt]

Done!

[juj]

What will be the path towards removing this check?

It looks like there is no way to feature test what the bad Node.js versions are/will be?

Does node.js have some kind of synchronous version test that could be used, e.g. "if node.js version >= x.y.z.w (regression first introduced) and node.js version < X.Y.Z.W (bugfix first appeared in), then disable streaming instantiation"?

If not, it will be really hard to evolve out of this check since users of older Node.js versions might break again if this check was ever removed (after latest Node.js fixes things up)?

[simnalamburt]

We have several options.

1. In my opinion, there's no big point with using WebAssembly.instantiateStreaming in Node.js environment, so we might keep this forever.
2. We can remove this check and wrap fetch() call with try { ... } catch() clause. This will add small overhead to old Node.js codes.
3. We can check process.version only in ENVIRONMENT_IS_NODE.
4. etc?

[juj]

Does node.js have some kind of synchronous version test that could be used, e.g. "if node.js version >= x.y.z.w (regression first introduced) and node.js version < X.Y.Z.W (bugfix first appeared in), then disable streaming instantiation"?

E.g. something like

if (!ENVIRONMENT_IS_NODE || (
    versionGreaterOrEqualTo(process.versions.node, [18,1,0]) &&
    versionLessThan(process.versions.node, [999,999,999]
    /* TODO: When Node.js lands a fix, update version range to match accordingly*/)))
   /* enable streaming instantiation */
else
   /* disable streaming instantiation */

[simnalamburt]

There is no built-in function to compare versions in Node.js, so we have to modify the code a bit much to get the behavior you want. Since applying this patch does not cause problems in future Node.js versions, why don't we discuss the issue of removing this patch again in another PR or another issue?

[kripken]

Makes sense to me to land this, and consider improvements later.

[RReverser]

I'd hope we can actually use instantiateStreaming if node implemented it

FWIW, out of curiosity, I tried to use streaming compilation with manual conversion from Node.js file streams, and it does work, but is ~2x slower than the buffer-based variant: https://twitter.com/RReverser/status/1593330290491068416

It's said to be due to the current implementation of Web streams in Node being experimental and not optimised yet, so it might change in the future, but, at least, for now there is no advantage in using streaming compilation.

[simnalamburt]

Thanks for the research @RReverser! Looks like we'd be better to sustain this patch for a long time in that case.