Avoid crashing on ES module resolution when module not found #1546
Conversation
kevinkassimo
force-pushed
the
esmodule/resolve_err
branch
from
91364a1
to
bb2b11b
Compare
| @@ -0,0 +1 @@ | |||
| NotFound: Cannot resolve module "./bad-module.js" from "[WILDCARD]/tests/error_009_missing_js_module.js" | |||
There was a problem hiding this comment.
Shouldn't we get a line number in the exception?
There was a problem hiding this comment.
It seems that custom exception gets empty stack trace, especially in ResolveCallback (GetCurrentStackTrace would not help either. I believe this is due to module resolution happening before running the code so there is not any actual stack trace)
(We can inject fake ones with correct import location hints, but this requires some extra changes to our binding layer)
For Node’s ESM solution, they do have a stack trace, but the trace is all about the JS side loader code which calls to the bindings without any information on the line/column/file of the problematic import statement
There was a problem hiding this comment.
Ok. I'm doing this big refactor of the module code right now - I'm going to delay landing this and/or work it into my patch...
There was a problem hiding this comment.
Changed my mind. I will just land now to avoid this crash - I doubt I will get my module stuff landed in time for the next release. I just anticipate changing this stuff soon.
Thanks for the patch!
| @@ -558,7 +558,11 @@ v8::MaybeLocal<v8::Module> ResolveCallback(v8::Local<v8::Context> context, | |||
|
|
|||
| if (d->resolve_module_.IsEmpty()) { | |||
| // Resolution Error. | |||
| isolate->ThrowException(v8_str("module resolution error")); | |||
| std::stringstream err_ss; | |||
| err_ss << "NotFound: Cannot resolve module \"" << specifier_c | |||
There was a problem hiding this comment.
I don't like that this is faking an ErrorKind::NotFound...
There was a problem hiding this comment.
I just tried to simulation TS compiler error format. The problem is there is no real stack trace I could find, and there is no error passing from Rust to JS in this step (so no ErrorKind is involved). As mentioned above I could have used the two module request location calls to fake one (to show on which line of import that this error relates to) but this requires injecting something into the generated error JSON object — there is no API to directly modify the Exception object itself
Closes #1545 .
See discussion #1545 (comment)
Ideally, could improve error message by providing more accurate location of
importstatement (usingGetModuleRequestsLength()andGetModuleRequestLocation()) (However, exception value cannot be directly modified and thus we might need to introduce custom location toEncodeExceptionAsJSON, which is out of scope of this PR and might not be very useful)