-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
fix(check): move module not found errors to typescript diagnostics #27533
fix(check): move module not found errors to typescript diagnostics #27533
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
} | ||
|
||
/// Gets a hash of the inputs for type checking. This can then | ||
/// be used to tell | ||
fn get_check_hash( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Merging this into get_tsc_roots
is more reliable because then the code is the same. There were some slight discrepancies before.
.as_ref() | ||
.and_then(|d| d.dependency.ok()); | ||
} else if let Module::Wasm(module) = module { | ||
maybe_module_dependencies = Some(&module.dependencies); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a bug here where we weren't visiting wasm module dependencies.
|
||
if let Some(range) = error.maybe_range() { | ||
if mode == EnhanceGraphErrorMode::ShowRange | ||
&& !range.specifier.as_str().contains("/$deno$eval") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ouch 😬 not super important, but maybe we should list out these "magical" specifiers in const
s in a single module and use these consts instead of string literals?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code was already here so I'll leave it as-is for now.
// todo(dsherret): it looks like the remapped_specifiers and | ||
// root_map could be combined... what is the point of the separation? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most likely no point, just a legacy code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I will update it in a follow-up pr.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
…27533) Instead of hard erroring, we now surface module not found errors as TypeScript diagnostics (we have yet to show the source code of the error, but something we can improve over time).
TYSM @dsherret for fixing this issue! Ya'll rock. |
Instead of hard erroring, we now surface module not found errors as TypeScript diagnostics (we have yet to show the source code of the error, but something we can improve over time).
This doesn't work yet for npm modules, but it's what I'll work on next.
Closes #27188