Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[compat] when using dynamic import in Node compat mode and module is not found, err.code needs to be set to 'ERR_MODULE_NOT_FOUND' #12588

Closed
kt3k opened this issue Oct 29, 2021 · 2 comments · Fixed by #12633
Assignees
Labels
bug Something isn't working correctly cli related to cli/ dir

Comments

@kt3k
Copy link
Member

kt3k commented Oct 29, 2021

In Node.js, it's common to check err.code to detect the kind of errors:

if (err.code === 'ERR_MODULE_NOT_FOUND') {
  // do something
}

std/node simulates these error codes in API layer (ref: https://github.com/denoland/deno_std/blob/0b31c1647649f9b6aa53432370daab4483803e77/node/_errors.ts#L289-L304 )

But in the case of dynamic import, the above shim can't be used. So err.code needs to be set to ERR_MODULE_NOT_FOUND directly in CLI runtime when --compat flag is specified.

This feature is used in mocha when looking up the test files (ref: https://github.com/mochajs/mocha/blob/97b84708afb42e552cb906a54f9f2aa2e6a98ba4/lib/nodejs/esm-utils.js#L47-L56 )

related: #12577

@bartlomieju bartlomieju self-assigned this Oct 29, 2021
@bartlomieju
Copy link
Member

I'm a bit at loss how to tackle this problem right now...

Module loading is done by deno_core and any additional fields on the error instance would need to be created there, but deno_core is completely agnostic of compat layer.

Moreover we need to decide how are we going to handle this situation in wider context - ie. in runtime Deno APIs (ref #12591).

@kt3k
Copy link
Member Author

kt3k commented Oct 31, 2021

TypeError for dynamic import rejection seems created here (hard-coded to type_error)

deno/core/runtime.rs

Lines 1145 to 1152 in e06515c

let exception = err
.downcast_ref::<ErrWithV8Handle>()
.map(|err| err.get_handle(scope))
.unwrap_or_else(|| {
let message = err.to_string();
let message = v8::String::new(scope, &message).unwrap();
v8::Exception::type_error(scope, message)
});

But that err comes from cli/proc_state.rs

deno/cli/proc_state.rs

Lines 528 to 531 in e06515c

return Err(custom_error(
"NotFound",
format!("Cannot load module \"{}\".", specifier),
));
and it has class_name at the creation, but that class_name is not used in core/runtime.rs. I guess we can use that class name and swap the error constructor in core/runtime.rs

(pseudo code)

let value = execute_script(get_custom_error_class(err).unwrap());
let err = value.to_object(scope).call_as_constructor(1, err_message);
resolver.reject(scope, err).unwrap();

@lucacasonato lucacasonato added bug Something isn't working correctly cli related to cli/ dir labels Nov 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working correctly cli related to cli/ dir
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants