Deno: catch of error from dynamic import does not halt propagation

[nitsky]

I am observing that an error thrown by a dynamic import always results in an "uncaught error". Here is an example:

// a.ts
try {
  await import('./b.ts')
} catch {
  console.log('catch')
}

// b.ts
throw Error()

Expected Output:

catch

Actual Output:

catch
error: Uncaught Error
    at file:///b.ts:1:7

[kitsonk]

This is a bug. An example working in Chrome as expected: https://codesandbox.io/s/amazing-cdn-ss5yj

[bartlomieju]

CC @piscisaureus related to module loader

[ry]

We have a similar test to this already:
https://github.com/denoland/deno/blob/v0.36.0/cli/tests/error_014_catch_dynamic_import_error.js
but it doesn't use top-level await. So we probably need to duplicate that test but remove the closure.

[bartlomieju]

@mylesborins we talked about issue with instantiation of modules with TLA, could you take a look if provided example covers the same issue?

[MylesBorins]

I'm unsure how you are handling the flag internally but module execute returns a promise when TLA is enabled and that promise needs to be handled by the engine.

/cc @devsnek

[devsnek]

Here is my patch for node: https://github.com/nodejs/node/pull/30370/files

The linking phase remains unchanged, but you'll have to watch out for the promise returned from Module::Evaluate() (note that when the harmony flag isn't set, you need to ignore the return value from Module::Evaluate()).

If you aren't handling that promise it will end up unhandled like any other promise.

[nitsky]

@bartlomieju thank you!! :)