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

refactor: more specific error handling #512

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
2 changes: 1 addition & 1 deletion lib/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ impl Loader for JsLoader {
};
response
.map(|value| serde_wasm_bindgen::from_value(value).unwrap())
.map_err(|_| anyhow!("load rejected or errored"))
.map_err(|_| anyhow!("load rejected or errored").into())
};
Box::pin(f)
}
Expand Down
33 changes: 17 additions & 16 deletions src/graph.rs
Original file line number Diff line number Diff line change
Expand Up @@ -173,15 +173,15 @@ pub enum JsrLoadError {
#[error("Loader should never return an external specifier for a jsr: specifier content load.")]
ContentLoadExternalSpecifier,
#[error(transparent)]
ContentLoad(Arc<anyhow::Error>),
ContentLoad(Arc<LoadError>),
#[error("JSR package manifest for '{}' failed to load. {:#}", .0, .1)]
PackageManifestLoad(String, Arc<anyhow::Error>),
PackageManifestLoad(String, Arc<LoadError>),
#[error("JSR package not found: {}", .0)]
PackageNotFound(String),
#[error("JSR package version not found: {}", .0)]
PackageVersionNotFound(PackageNv),
#[error("JSR package version manifest for '{}' failed to load: {:#}", .0, .1)]
PackageVersionManifestLoad(PackageNv, Arc<anyhow::Error>),
PackageVersionManifestLoad(PackageNv, Arc<LoadError>),
#[error("JSR package version manifest for '{}' failed to load: {:#}", .0, .1)]
PackageVersionManifestChecksumIntegrity(PackageNv, ChecksumIntegrityError),
#[error(transparent)]
Expand Down Expand Up @@ -225,7 +225,7 @@ pub enum ModuleLoadError {
#[error(transparent)]
Decode(Arc<std::io::Error>),
#[error(transparent)]
Loader(Arc<anyhow::Error>),
Loader(Arc<LoadError>),
#[error(transparent)]
Jsr(#[from] JsrLoadError),
#[error(transparent)]
Expand Down Expand Up @@ -2004,7 +2004,10 @@ fn resolve(
use SpecifierError::*;
let res_ref = response.as_ref();
if matches!(res_ref, Err(Specifier(ImportPrefixMissing { .. })))
|| matches!(res_ref, Err(Other(e)) if matches!(e.downcast_ref::<ImportMapError>(), Some(&ImportMapError::UnmappedBareSpecifier(_, _))))
|| matches!(
res_ref,
Err(ImportMap(ImportMapError::UnmappedBareSpecifier(_, _)))
)
{
if let Ok(specifier) =
ModuleSpecifier::parse(&format!("node:{}", specifier_text))
Expand Down Expand Up @@ -4370,24 +4373,22 @@ impl<'a, 'graph> Builder<'a, 'graph> {
load_specifier.clone(),
maybe_range.cloned(),
)),
Err(err) => match err.downcast::<ChecksumIntegrityError>() {
// try to return the context of a checksum integrity error
// so that it can be more easily enhanced
Ok(err) => Err(ModuleError::LoadingErr(
Err(LoadError::ChecksumIntegrity(err)) => {
Err(ModuleError::LoadingErr(
load_specifier.clone(),
maybe_range.cloned(),
if maybe_version_info.is_some() {
JsrLoadError::ContentChecksumIntegrity(err).into()
} else {
ModuleLoadError::HttpsChecksumIntegrity(err)
},
)),
Err(err) => Err(ModuleError::LoadingErr(
load_specifier.clone(),
maybe_range.cloned(),
ModuleLoadError::Loader(Arc::new(err)),
)),
},
))
}
Err(err) => Err(ModuleError::LoadingErr(
load_specifier.clone(),
maybe_range.cloned(),
ModuleLoadError::Loader(Arc::new(err)),
)),
}
}

Expand Down
10 changes: 5 additions & 5 deletions src/jsr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,8 @@ use crate::packages::JsrPackageVersionInfo;
use crate::rt::spawn;
use crate::rt::JoinHandle;
use crate::source::CacheSetting;
use crate::source::ChecksumIntegrityError;
use crate::source::JsrUrlProvider;
use crate::source::LoadError;
use crate::source::LoadOptions;
use crate::source::LoadResponse;
use crate::source::Loader;
Expand Down Expand Up @@ -154,15 +154,15 @@ impl JsrMetadataStore {
{
let package_nv = package_nv.clone();
|e| {
match e.downcast::<ChecksumIntegrityError>() {
Ok(err) => {
match e {
LoadError::ChecksumIntegrity(err) => {
// use a more specific variant in order to allow the
// cli to enhance this error message
JsrLoadError::PackageVersionManifestChecksumIntegrity(
package_nv, err,
)
}
Err(err) => JsrLoadError::PackageVersionManifestLoad(
err => JsrLoadError::PackageVersionManifestLoad(
package_nv,
Arc::new(err),
),
Expand All @@ -187,7 +187,7 @@ impl JsrMetadataStore {
cache_setting: CacheSetting,
maybe_expected_checksum: Option<LoaderChecksum>,
handle_content: impl FnOnce(&[u8]) -> Result<T, serde_json::Error> + 'static,
create_failed_load_err: impl FnOnce(anyhow::Error) -> JsrLoadError + 'static,
create_failed_load_err: impl FnOnce(LoadError) -> JsrLoadError + 'static,
create_not_found_error: impl FnOnce() -> JsrLoadError + 'static,
) -> PendingResult<T> {
let fut = services.loader.load(
Expand Down
8 changes: 3 additions & 5 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1260,8 +1260,7 @@ console.log(a);
fn resolve_builtin_node_module(
&self,
specifier: &deno_ast::ModuleSpecifier,
) -> anyhow::Result<Option<String>, source::UnknownBuiltInNodeModuleError>
{
) -> Result<Option<String>, source::UnknownBuiltInNodeModuleError> {
if specifier.to_string() == "node:path" {
Ok(Some("path".to_string()))
} else {
Expand Down Expand Up @@ -1317,12 +1316,11 @@ console.log(a);
_mode: ResolutionMode,
) -> Result<deno_ast::ModuleSpecifier, source::ResolveError> {
use import_map::ImportMapError;
Err(source::ResolveError::Other(
Err(source::ResolveError::ImportMap(
ImportMapError::UnmappedBareSpecifier(
specifier_text.to_string(),
Some(referrer_range.specifier.to_string()),
)
.into(),
),
))
}

Expand Down
26 changes: 14 additions & 12 deletions src/source/file_system.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,17 @@ pub enum DirEntryKind {
File,
Dir,
Symlink,
Error(anyhow::Error),
Error(DirEntryError),
}

#[derive(Debug, thiserror::Error)]
pub enum DirEntryError {
#[error("Failed to read directory.")]
Dir(std::io::Error),
#[error("Failed to read file type.")]
FileType(std::io::Error),
#[error("Failed to read entry in directory.")]
DirEntry(std::io::Error),
crowlKats marked this conversation as resolved.
Show resolved Hide resolved
}

#[derive(Debug)]
Expand Down Expand Up @@ -132,10 +142,7 @@ impl FileSystem for RealFileSystem {
}
Err(err) => {
return vec![DirEntry {
kind: DirEntryKind::Error(
anyhow::Error::from(err)
.context("Failed to read directory.".to_string()),
),
kind: DirEntryKind::Error(DirEntryError::Dir(err)),
url: dir_url.clone(),
}];
}
Expand All @@ -162,9 +169,7 @@ impl FileSystem for RealFileSystem {
continue;
}
Err(err) => DirEntry {
kind: DirEntryKind::Error(
anyhow::Error::from(err).context("Failed to read file type."),
),
kind: DirEntryKind::Error(DirEntryError::FileType(err)),
url: ModuleSpecifier::from_file_path(entry.path()).unwrap(),
},
}
Expand All @@ -179,10 +184,7 @@ impl FileSystem for RealFileSystem {
}
Err(err) => DirEntry {
url: dir_url.clone(),
kind: DirEntryKind::Error(
anyhow::Error::from(err)
.context("Failed to read entry in directory.".to_string()),
),
kind: DirEntryKind::Error(DirEntryError::DirEntry(err)),
},
};

Expand Down
35 changes: 25 additions & 10 deletions src/source/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,6 @@ use async_trait::async_trait;
use deno_ast::MediaType;
use deno_semver::package::PackageNv;

use anyhow::anyhow;
use anyhow::Error;
use data_url::DataUrl;
use deno_ast::ModuleSpecifier;
use deno_semver::package::PackageReq;
Expand Down Expand Up @@ -85,7 +83,21 @@ pub enum LoadResponse {
},
}

pub type LoadResult = Result<Option<LoadResponse>, anyhow::Error>;
#[derive(Debug, Error)]
pub enum LoadError {
#[error("Unsupported scheme: {0}")]
UnsupportedScheme(String),
#[error(transparent)]
ChecksumIntegrity(#[from] ChecksumIntegrityError),
#[error(transparent)]
JsonParse(#[from] serde_json::Error),
#[error(transparent)]
DataUrl(std::io::Error),
#[error(transparent)]
Other(#[from] anyhow::Error),
}

pub type LoadResult = Result<Option<LoadResponse>, LoadError>;
pub type LoadFuture = LocalBoxFuture<'static, LoadResult>;

#[derive(Debug, Copy, Clone, PartialEq, Eq)]
Expand Down Expand Up @@ -122,7 +134,8 @@ pub static DEFAULT_JSR_URL: Lazy<Url> =
Lazy::new(|| Url::parse("https://jsr.io").unwrap());

#[derive(Debug, Clone, Error)]
#[error("Integrity check failed.\n\nActual: {}\nExpected: {}", .actual, .expected)]
#[error("Integrity check failed.\n\nActual: {}\nExpected: {}", .actual, .expected
)]
pub struct ChecksumIntegrityError {
pub actual: String,
pub expected: String,
Expand Down Expand Up @@ -361,6 +374,8 @@ pub enum ResolveError {
#[error(transparent)]
Specifier(#[from] SpecifierError),
#[error(transparent)]
ImportMap(#[from] import_map::ImportMapError),
#[error(transparent)]
Other(#[from] anyhow::Error),
}

Expand Down Expand Up @@ -488,8 +503,8 @@ pub trait NpmResolver: fmt::Debug {

pub fn load_data_url(
specifier: &ModuleSpecifier,
) -> Result<Option<LoadResponse>, anyhow::Error> {
let data_url = RawDataUrl::parse(specifier)?;
) -> Result<Option<LoadResponse>, LoadError> {
let data_url = RawDataUrl::parse(specifier).map_err(LoadError::DataUrl)?;
let (bytes, headers) = data_url.into_bytes_and_headers();
Ok(Some(LoadResponse::Module {
specifier: specifier.clone(),
Expand Down Expand Up @@ -563,7 +578,7 @@ fn get_mime_type_charset(mime_type: &str) -> Option<&str> {
/// ahead of time. This is useful for testing or
#[derive(Default)]
pub struct MemoryLoader {
sources: HashMap<ModuleSpecifier, Result<LoadResponse, Error>>,
sources: HashMap<ModuleSpecifier, Result<LoadResponse, anyhow::Error>>,
cache_info: HashMap<ModuleSpecifier, CacheInfo>,
}

Expand All @@ -575,11 +590,11 @@ pub enum Source<S> {
},
Redirect(S),
External(S),
Err(Error),
Err(anyhow::Error),
}

impl<S: AsRef<str>> Source<S> {
fn into_result(self) -> Result<LoadResponse, Error> {
fn into_result(self) -> Result<LoadResponse, anyhow::Error> {
match self {
Source::Module {
specifier,
Expand Down Expand Up @@ -699,7 +714,7 @@ impl Loader for MemoryLoader {
) -> LoadFuture {
let response = match self.sources.get(specifier) {
Some(Ok(response)) => Ok(Some(response.clone())),
Some(Err(err)) => Err(anyhow!("{}", err)),
Some(Err(err)) => Err(LoadError::Other(anyhow::anyhow!("{}", err))),
None if specifier.scheme() == "data" => load_data_url(specifier),
_ => Ok(None),
};
Expand Down
7 changes: 3 additions & 4 deletions tests/ecosystem_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -178,16 +178,15 @@ impl deno_graph::source::Loader for Loader<'_> {
specifier: specifier.clone(),
})),
Err(err) if err.kind() == std::io::ErrorKind::NotFound => Ok(None),
Err(err) => Err(anyhow::Error::from(err)),
Err(err) => Err(anyhow::Error::from(err).into()),
}
}
"data" => deno_graph::source::load_data_url(specifier),
"jsr" | "npm" | "node" => Ok(Some(LoadResponse::External {
specifier: specifier.clone(),
})),
_ => Err(anyhow::anyhow!(
"Unsupported scheme: {}",
specifier.scheme()
_ => Err(deno_graph::source::LoadError::UnsupportedScheme(
specifier.scheme().to_string(),
)),
};
async move { res }.boxed()
Expand Down