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.

Sign up for GitHub

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: npm resolution errors to tsc diagnostics #27548

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
+203 −87
Draft

fix: npm resolution errors to tsc diagnostics #27548

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
127 changes: 106 additions & 21 deletions cli/graph_util.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ use deno_semver::package::PackageNv;
use deno_semver::SmallStackString;
use import_map::ImportMapError;
use node_resolver::InNpmPackageChecker;
use sys_traits::FsMetadata;

use crate::args::config_to_deno_graph_workspace_member;
use crate::args::jsr_url;
Expand Down Expand Up @@ -146,6 +147,25 @@ pub fn graph_walk_errors<'a>(
roots: &'a [ModuleSpecifier],
options: GraphWalkErrorsOptions,
) -> impl Iterator<Item = AnyError> + 'a {
fn should_ignore_error(
sys: &CliSys,
graph_kind: GraphKind,
error: &ModuleGraphError,
) -> bool {
if graph_kind == GraphKind::TypesOnly
&& matches!(
error,
ModuleGraphError::ModuleError(ModuleError::UnsupportedMediaType(..))
)
{
return true;
}

// surface these as typescript diagnostics instead
graph_kind.include_types()
&& has_module_graph_error_for_tsc_diagnostic(sys, error)
}

graph
.walk(
roots.iter(),
Expand All @@ -158,6 +178,11 @@ pub fn graph_walk_errors<'a>(
)
.errors()
.flat_map(|error| {
if should_ignore_error(sys, graph.graph_kind(), &error) {
log::debug!("Ignoring: {}", error);
return None;
}

let is_root = match &error {
ModuleGraphError::ResolutionError(_)
| ModuleGraphError::TypesResolutionError(_) => false,
Expand All @@ -175,30 +200,90 @@ pub fn graph_walk_errors<'a>(
},
);

if graph.graph_kind() == GraphKind::TypesOnly
&& matches!(
error,
ModuleGraphError::ModuleError(ModuleError::UnsupportedMediaType(..))
)
{
log::debug!("Ignoring: {}", message);
return None;
}
Some(custom_error(get_module_graph_error_class(&error), message))
})
}

if graph.graph_kind().include_types()
&& (message.contains(RUN_WITH_SLOPPY_IMPORTS_MSG)
|| matches!(
error,
ModuleGraphError::ModuleError(ModuleError::Missing(..))
))
{
// ignore and let typescript surface this as a diagnostic instead
log::debug!("Ignoring: {}", message);
return None;
fn has_module_graph_error_for_tsc_diagnostic<'a>(
sys: &CliSys,
error: &'a ModuleGraphError,
) -> bool {
match error {
ModuleGraphError::ModuleError(error) => {
module_error_for_tsc_diagnostic(sys, error).is_some()
}
ModuleGraphError::TypesResolutionError(error) => {
resolution_error_for_tsc_diagnostic(error).is_some()
}
_ => false,
}
}

pub struct ModuleNotFoundGraphErrorRef<'a> {
pub specifier: &'a ModuleSpecifier,
pub maybe_range: Option<&'a deno_graph::Range>,
}

pub fn module_error_for_tsc_diagnostic<'a>(
sys: &CliSys,
error: &'a ModuleError,
) -> Option<ModuleNotFoundGraphErrorRef<'a>> {
match error {
ModuleError::Missing(specifier, maybe_range) => {
Some(ModuleNotFoundGraphErrorRef {
specifier,
maybe_range: maybe_range.as_ref(),
})
}
ModuleError::LoadingErr(
specifier,
maybe_range,
ModuleLoadError::Loader(_),
) => {
if let Ok(path) = deno_path_util::url_to_file_path(specifier) {
if sys.fs_is_dir_no_err(path) {
return Some(ModuleNotFoundGraphErrorRef {
specifier,
maybe_range: maybe_range.as_ref(),
});
}
}
None
}
_ => None,
}
}

Some(custom_error(get_module_graph_error_class(&error), message))
})
pub struct ModuleNotFoundNodeResolutionErrorRef<'a> {
pub specifier: &'a str,
pub maybe_range: Option<&'a deno_graph::Range>,
}

pub fn resolution_error_for_tsc_diagnostic<'a>(
error: &'a ResolutionError,
) -> Option<ModuleNotFoundNodeResolutionErrorRef<'a>> {
match error {
ResolutionError::ResolverError {
error,
specifier,
range,
} => match error.as_ref() {
ResolveError::Other(error) => {
// would be nice if there were an easier way of doing this
let text = error.to_string();
if text.contains("[ERR_MODULE_NOT_FOUND]") {
Some(ModuleNotFoundNodeResolutionErrorRef {
specifier,
maybe_range: Some(range),
})
} else {
None
}
}
_ => None,
},
_ => None,
}
}

#[derive(Debug, PartialEq, Eq)]
Expand Down
91 changes: 46 additions & 45 deletions cli/tools/check.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,7 @@ use deno_ast::MediaType;
use deno_ast::ModuleSpecifier;
use deno_core::error::AnyError;
use deno_graph::Module;
use deno_graph::ModuleError;
use deno_graph::ModuleGraph;
use deno_graph::ModuleLoadError;
use deno_terminal::colors;
use once_cell::sync::Lazy;
use regex::Regex;
Expand All @@ -29,6 +27,8 @@ use crate::cache::FastInsecureHasher;
use crate::cache::TypeCheckCache;
use crate::factory::CliFactory;
use crate::graph_util::maybe_additional_sloppy_imports_message;
use crate::graph_util::module_error_for_tsc_diagnostic;
use crate::graph_util::resolution_error_for_tsc_diagnostic;
use crate::graph_util::BuildFastCheckGraphOptions;
use crate::graph_util::ModuleGraphBuilder;
use crate::node::CliNodeResolver;
Expand Down Expand Up @@ -469,36 +469,20 @@ fn get_tsc_roots(
let module = match graph.try_get(specifier) {
Ok(Some(module)) => module,
Ok(None) => continue,
Err(ModuleError::Missing(specifier, maybe_range)) => {
Err(err) => {
if !is_dynamic {
result
.missing_diagnostics
.push(tsc::Diagnostic::from_missing_error(
specifier,
maybe_range.as_ref(),
maybe_additional_sloppy_imports_message(sys, specifier),
));
}
continue;
}
Err(ModuleError::LoadingErr(
specifier,
maybe_range,
ModuleLoadError::Loader(_),
)) => {
// these will be errors like attempting to load a directory
if !is_dynamic {
result
.missing_diagnostics
.push(tsc::Diagnostic::from_missing_error(
specifier,
maybe_range.as_ref(),
maybe_additional_sloppy_imports_message(sys, specifier),
));
if let Some(err) = module_error_for_tsc_diagnostic(sys, err) {
result.missing_diagnostics.push(
tsc::Diagnostic::from_missing_error(
err.specifier.as_str(),
err.maybe_range,
maybe_additional_sloppy_imports_message(sys, err.specifier),
),
);
}
}
continue;
}
Err(_) => continue,
};
if is_dynamic && !seen.insert(specifier) {
continue;
Expand Down Expand Up @@ -541,23 +525,40 @@ fn get_tsc_roots(
if let Some(deps) = maybe_module_dependencies {
for dep in deps.values() {
// walk both the code and type dependencies
if let Some(specifier) = dep.get_code() {
handle_specifier(
graph,
&mut seen,
&mut pending,
specifier,
dep.is_dynamic,
);
}
if let Some(specifier) = dep.get_type() {
handle_specifier(
graph,
&mut seen,
&mut pending,
specifier,
dep.is_dynamic,
);
match &dep.maybe_type {
deno_graph::Resolution::None => {
if let Some(specifier) = dep.get_code() {
handle_specifier(
graph,
&mut seen,
&mut pending,
specifier,
dep.is_dynamic,
);
}
}
deno_graph::Resolution::Ok(resolution) => {
handle_specifier(
graph,
&mut seen,
&mut pending,
&resolution.specifier,
dep.is_dynamic,
);
}
deno_graph::Resolution::Err(resolution_error) => {
if let Some(err) =
resolution_error_for_tsc_diagnostic(resolution_error)
{
result.missing_diagnostics.push(
tsc::Diagnostic::from_missing_error(
err.specifier,
err.maybe_range,
None,
),
);
}
}
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion cli/tsc/diagnostics.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ pub struct Diagnostic {

impl Diagnostic {
pub fn from_missing_error(
specifier: &ModuleSpecifier,
specifier: &str,
maybe_range: Option<&deno_graph::Range>,
additional_message: Option<String>,
) -> Self {
Expand Down
40 changes: 23 additions & 17 deletions resolvers/node/analyze.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
use std::borrow::Cow;
use std::collections::BTreeSet;
use std::collections::HashSet;
use std::path::Path;
use std::path::PathBuf;

use anyhow::Context;
Expand All @@ -20,6 +19,7 @@ use sys_traits::FsMetadata;
use sys_traits::FsRead;
use url::Url;

use crate::errors::ModuleNotFoundError;
use crate::npm::InNpmPackageCheckerRc;
use crate::resolution::NodeResolverRc;
use crate::IsBuiltInNodeModuleChecker;
Expand Down Expand Up @@ -319,7 +319,8 @@ impl<
if specifier.starts_with("./") || specifier.starts_with("../") {
if let Some(parent) = referrer_path.parent() {
return self
.file_extension_probe(parent.join(specifier), &referrer_path)
.file_extension_probe(parent.join(specifier), &referrer)
.map_err(AnyError::from)
.and_then(|p| url_from_file_path(&p).map_err(AnyError::from))
.map(Some);
} else {
Expand Down Expand Up @@ -389,7 +390,8 @@ impl<
return Ok(Some(url_from_file_path(&d.join("index.js").clean())?));
}
return self
.file_extension_probe(d, &referrer_path)
.file_extension_probe(d, &referrer)
.map_err(AnyError::from)
.and_then(|p| url_from_file_path(&p).map_err(AnyError::from))
.map(Some);
} else if let Some(main) =
Expand All @@ -414,20 +416,27 @@ impl<
} else {
parent.join("node_modules").join(specifier)
};
if let Ok(path) = self.file_extension_probe(path, &referrer_path) {
if let Ok(path) = self.file_extension_probe(path, &referrer) {
return Ok(Some(url_from_file_path(&path)?));
}
last = parent;
}

Err(not_found(specifier, &referrer_path))
Err(
ModuleNotFoundError {
specifier: specifier.to_string(),
typ: "module",
maybe_referrer: Some(referrer.clone()),
}
.into(),
)
}

fn file_extension_probe(
&self,
p: PathBuf,
referrer: &Path,
) -> Result<PathBuf, AnyError> {
referrer_url: &Url,
) -> Result<PathBuf, ModuleNotFoundError> {
let p = p.clean();
if self.sys.fs_exists_no_err(&p) {
let file_name = p.file_name().unwrap();
Expand Down Expand Up @@ -456,7 +465,13 @@ impl<
}
}
}
Err(not_found(&p.to_string_lossy(), referrer))
Err(ModuleNotFoundError {
specifier: deno_path_util::url_from_file_path(&p)
.map(|p| p.to_string())
.unwrap_or_else(|_| p.to_string_lossy().to_string()),
typ: "module",
maybe_referrer: Some(referrer_url.clone()),
})
}
}

Expand Down Expand Up @@ -617,15 +632,6 @@ fn parse_specifier(specifier: &str) -> Option<(String, String)> {
Some((package_name, package_subpath))
}

fn not_found(path: &str, referrer: &Path) -> AnyError {
let msg = format!(
"[ERR_MODULE_NOT_FOUND] Cannot find module \"{}\" imported from \"{}\"",
path,
referrer.to_string_lossy()
);
std::io::Error::new(std::io::ErrorKind::NotFound, msg).into()
}

fn to_double_quote_string(text: &str) -> String {
// serde can handle this for us
serde_json::to_string(text).unwrap()
Expand Down
2 changes: 1 addition & 1 deletion resolvers/node/errors.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -479,7 +479,7 @@ impl NodeJsErrorCoded for FinalizeResolutionError {
maybe_referrer.as_ref().map(|referrer| format!(" imported from '{}'", referrer)).unwrap_or_default()
)]
pub struct ModuleNotFoundError {
pub specifier: Url,
pub specifier: String,
pub maybe_referrer: Option<Url>,
pub typ: &'static str,
}
Expand Down
Loading
Loading