fix(wasm): rejecting with error on node.js file read

[nickbabcock]

Rollup Plugin Name: wasm

This PR contains:

• bugfix

Are tests included?

• yes

Breaking Changes?

• no

Description

When node.js is reading the wasm file and encounters an error (like if the file doesn't exist or user doesn't have permissions to read). It will still try and call _instantiateOrCompile with undefined, which results in an uncaught exception, possibly obscuring the error. As instead of the file read error, the following error will return:

TypeError: WebAssembly.compile(): Argument 0 must be a buffer source

This can be replicated with the following script (don't execute at a REPL):

(async () => {
  await new Promise((resolve, reject) => {
    reject(new Error("permission error"));
    resolve(WebAssembly.compile(undefined));
  });
})();

Node.js will exit with an error code and print only the type error to stderr.

If we try and catch the error at the outer level

(async () => {
  try {
    await new Promise((resolve, reject) => {
      reject(new Error("permission error"));
      resolve(WebAssembly.compile(undefined));
    });
  } catch (ex) {
    console.log(`this is the error: ${ex}`)
  }
})();

The log statement will exclude the type error:

this is the error: Error: permission error

But node.js will still exit with an error code and the type error is printed to stderr.

This is the behavior of node.js default uncaught exception handler

By default, Node.js handles such exceptions by printing the stack trace to stderr and exiting with code 1

This commit avoids the uncaught exception by only invoking resolve behind a conditional on an absent error.

This commit doesn't change the status of the promise as calling resolve after reject, while legal, doesn't really do anything (ref):

Only the first call to resolveFunc or rejectFunc affects the promise's eventual state, and subsequent calls to either function can neither change the fulfillment value/rejection reason nor toggle its eventual state from "fulfilled" to "rejected" or opposite

So depending on the runtime and how the user is overriding the default handler, uncaught exceptions may be handled differently

The test executes in a worker so that it can require the appropriate node js modules. I saw that another Wasm test used a worker, but would never execute as require is not defined, so this commit also fixes that test case.

[shellscape]

I'm secretly hoping that the changes are trivial enough to not warrant a test as this bugfix revolves around the error message that is propagated and may be runtime dependent.

What can we do to protect against a regression (e.g. "hey this doesn't do anything on the runtime I'm using, let's remove useless code, for some reason") without a test?

At the very least, we should have a thorough code comment that documents what runtimes will do what behavior and a warning not to remove the else case.

[nickbabcock]

I was able to write a test, but it required fixing an existing test that wasn't executing. Hopefully this is ok.

I've updated the description so that it is more accurate (the problem is that the type error is an uncaught exception, which can obscure the rejected promise).

Thanks for the patience 😄

[shellscape]

Thanks for putting the work in on the plugin, and apologies for the delay in review.