support dependencies across multiple namespaces #91
Conversation
macovedj
force-pushed
the
multi-namespace
branch
5 times, most recently
from
551e52a to
ceb9671
Compare
macovedj
force-pushed
the
multi-namespace
branch
from
ceb9671 to
8134a10
Compare
crates/wac-resolver/src/registry.rs
Outdated
| self.client | ||
| .get_warg_registry(id.namespace()) | ||
| .await | ||
| .map_err(|e| CommandError::WargClient(e))? |
There was a problem hiding this comment.
it would be nice if these errors were more context aware. Instead of just storing the warg error, we should add some additional context, let that this is fetching the registry.
There was a problem hiding this comment.
Thanks! The biggest issue I can see with this change currently is that the changes to error handling seem to be more invasive than they need to be. It would be nice if we could slot the new error conditions into the existing Error enum - these errors are still errors in resolution, so I'm not sure why we can't just expand the current error handling.
| @@ -38,7 +39,7 @@ tokio-util = "0.7.10" | |||
| tempdir = "0.3.7" | |||
|
|
|||
| [features] | |||
| default = ["registry"] | |||
| default = [] | |||
There was a problem hiding this comment.
Unless it makes it harder to implement, I'd rather we leave the registry feature in in this PR. We can discuss it's removal either in an issue or in a PR specific to its removal.
| use indexmap::IndexMap; | ||
| use miette::{Diagnostic, SourceSpan}; | ||
| use wac_parser::Document; | ||
| use thiserror::Error; |
There was a problem hiding this comment.
nit: elsewhere we're using thiserror::Error qualified instead of through a use. Can you replace Error everywhere with thiserror::Error?
|
|
||
| #[derive(Debug, Error)] | ||
| /// Error for WAC commands | ||
| pub enum CommandError { |
There was a problem hiding this comment.
This doesn't seem right. Why weren't error variants just added to the Error enum?
| .map_err(|e: anyhow::Error| Error::PackageResolutionFailure { | ||
| name: key.name.to_string(), | ||
| span: *span, | ||
| source: e, | ||
| })?; |
There was a problem hiding this comment.
Removing this seems like a regression.
| .map_err(|e| Error::PackageResolutionFailure { | ||
| name: key.name.to_string(), | ||
| span: *span, | ||
| source: e, | ||
| })? |
There was a problem hiding this comment.
Same here - I'm not sure why this was removed.
| .get_warg_registry(id.namespace()) | ||
| .await | ||
| .map_err(|e| { | ||
| CommandError::WargClient( |
There was a problem hiding this comment.
It seems like adding an Error::WargRegistryResolutionError (or similar) would be a better solution rather than having a CommandError type.
| @@ -217,15 +225,20 @@ impl RegistryPackageResolver { | |||
| bar.finish(); | |||
| } | |||
| } | |||
| Err(ClientError::PackageDoesNotExist { name }) => { | |||
There was a problem hiding this comment.
Same here - I'm not sure why it's an improvement to remove these errors.
|
Closing in favor of Calvin's new PR, which incorporates new warg changes which should simplify quite a bit. |
This leverages bytecodealliance/registry#263 from warg-client enabling wac to have dependencies across multiple registries/namespaces. The CLI now takes advantage of the namespace mappings that the warg client provides in its local storage.
Worth noting... if a user attempts to depend on a package that doesn't exist in the registry they're using, but does exist in another registry that their registry is aware of, it can provide a hint indicating that the user can update their namespace mappings to enable communication with the proper registry. This use case also required the addition of logic to retry many of the CLI commands after updating local namespace storage with the proper hint, preserving the same user experience that the warg CLI has.
I also figured it makes sense at this point to remove the feature flag that the registry support is behind, unless I missed some rationale for the flag. Figured I could easily add it back if there are any reasons not to.