feat: implement package manager#12
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request implements a comprehensive package manager for the Xmas.JS project, forked from the Cotton package manager. The implementation adds npm-compatible package management capabilities including dependency resolution, installation, and script execution.
Key changes:
- Integration of package manager commands (
/pmand/$) into the REPL - Complete package manager implementation with dependency resolution, caching, and installation
- Support for npm registry interactions, lockfiles, and binary setup
Reviewed changes
Copilot reviewed 29 out of 29 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| repl/src/lib.rs | Adds REPL commands for package manager (/pm) and shell execution (/$) |
| repl/Cargo.toml | Adds dependencies for package manager, clap, and deno_task_shell |
| package-manager/src/watch.rs | Implements file watching for hot-reload functionality |
| package-manager/src/util.rs | Utility functions for HTTP clients, version parsing, and JSON operations |
| package-manager/src/scoped_path.rs | Security module for safe path resolution (from safe-path crate) |
| package-manager/src/resolve.rs | Dependency graph resolution and lockfile management |
| package-manager/src/progress.rs | Progress bar and logging utilities |
| package-manager/src/plan.rs | Package installation planning and execution |
| package-manager/src/package.rs | Core package metadata structures and serialization |
| package-manager/src/npm.rs | NPM registry API interaction and package fetching |
| package-manager/src/lib.rs | Main library entry point and public API |
| package-manager/src/config.rs | Configuration file handling and registry authentication |
| package-manager/src/commands/*.rs | Command implementations (install, add, remove, run, etc.) |
| package-manager/src/cli.rs | CLI argument parsing with clap |
| package-manager/src/cache.rs | Async caching mechanism for package data |
| package-manager/Cargo.toml | Package manager dependencies |
| Cargo.toml | Workspace configuration updates |
| TODO.md | Updates showing completed package manager tasks |
| README.md | Credits Cotton as inspiration |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pub struct Cache<K: Eq + Hash + Clone + Send + Debug + 'static, V: Clone + Send + 'static> { | ||
| loader: Box<dyn Fn(K) -> BoxFuture<'static, V> + Send + Sync + 'static>, | ||
| map: DashMap<K, SharedBoxFuture<V>>, | ||
| } |
There was a problem hiding this comment.
Missing documentation for public API. The Cache struct and its methods are public but lack documentation comments explaining their purpose, usage, and behavior. Add doc comments to help users understand how to use this caching mechanism.
| indicatif = "0.18.0" | ||
| multimap = "0.10.0" | ||
| node-semver = { git = "https://github.com/danielhuang/node-semver-rs", rev = "bf4b103dc88b310c9dc049433aff1a14716e1e68" } | ||
| notify = "=8.2.0" |
There was a problem hiding this comment.
The version for notify is pinned with an exact match (=8.2.0) which is unusual and overly restrictive. This prevents any patch updates and makes dependency resolution more difficult. Consider using a more flexible semver constraint like "^8.2.0" unless there's a specific reason for exact pinning.
| notify = "=8.2.0" | |
| notify = "^8.2.0" |
| let (prefix, rest) = s | ||
| .split_once(':') | ||
| .ok_or_else(|| D::Error::custom("missing :"))?; | ||
| if prefix.starts_with("http") { |
There was a problem hiding this comment.
The prefix "http" check seems overly broad and could prevent legitimate use cases. This check will block any prefix starting with "http", including potentially valid ones like "https" or custom schemes like "http-git". Consider being more specific about what should be blocked, or provide clearer documentation about why HTTP URLs are not allowed in version specifiers.
| if prefix.starts_with("http") { | |
| if prefix == "http" { |
| move |res: notify::Result<Event>| { | ||
| futures::executor::block_on(async { | ||
| if let Ok(res) = res { | ||
| if res.kind.is_access() { |
There was a problem hiding this comment.
The is_access() filter appears to be the wrong event type check for file watching. The notify crate's is_access() checks for file access events (like reads), not modifications. For watching file changes, you typically want to check for modify events using kind.is_modify() or similar. This will cause the watcher to only trigger on file reads, not writes/modifications.
| if res.kind.is_access() { | |
| if res.kind.is_modify() { |
| "freebsd" => "freebsd", | ||
| "openbsd" => "openbsd", | ||
| "windows" => "win32", | ||
| _ => unreachable!(), |
There was a problem hiding this comment.
The unreachable! macro will panic if OS is not one of the expected values. This is problematic for portability. Consider using a default case or returning an error instead. Operating systems like NetBSD, Solaris, Android, iOS, or others would cause a panic here.
| _ => unreachable!(), | |
| _ => OS, |
| if args[0] == "pm" { | ||
| if let Ok(cmd) = xmas_package_manager::cli::Subcommand::try_parse_from(&args) { | ||
| let args = xmas_package_manager::Args { | ||
| verbose: true, | ||
| working_dir: std::env::current_dir().ok(), | ||
| immutable: false, | ||
| cmd | ||
| }; | ||
| let _ = xmas_package_manager::execute_command(&args).await; | ||
| } else { | ||
| eprintln!("{}: Invalid package manager command", "Error".red().bold()); | ||
| } | ||
| } | ||
| else if args[0] == "$" { | ||
| let shell_command = args[1..].join(" "); | ||
| let cwd = std::env::current_dir()?; | ||
| let mut new_env = std::collections::HashMap::new(); | ||
| new_env.insert(std::ffi::OsString::from("PATH"), xmas_package_manager::commands::new_path().map_err(|e| { | ||
| anyhow::anyhow!("Failed to construct PATH: {}", e) | ||
| })?); | ||
| let exit_code = xmas_package_manager::commands::exec::shell(&shell_command, cwd, new_env, deno_task_shell::KillSignal::default()).await.map_err(|e| { | ||
| anyhow::anyhow!("Failed to execute shell command: {}", e) | ||
| })?; | ||
| if exit_code != 0 { | ||
| eprintln!("{}: Shell command exited with code {}", "Error".red().bold(), exit_code); | ||
| } | ||
| } | ||
| else { | ||
| eprintln!("{}: Unknown command '{}'", "Error".red().bold(), cmd); | ||
| } |
There was a problem hiding this comment.
Potential panic due to unchecked array slice access. If args has only one element (e.g., args = ["$"]), then args[1..] will be valid but empty. However, if args is empty (which is possible from line 220), this will panic when trying to access args[0] on line 234 before reaching here. The real issue is that args length should be validated before any indexing.
| if args[0] == "pm" { | |
| if let Ok(cmd) = xmas_package_manager::cli::Subcommand::try_parse_from(&args) { | |
| let args = xmas_package_manager::Args { | |
| verbose: true, | |
| working_dir: std::env::current_dir().ok(), | |
| immutable: false, | |
| cmd | |
| }; | |
| let _ = xmas_package_manager::execute_command(&args).await; | |
| } else { | |
| eprintln!("{}: Invalid package manager command", "Error".red().bold()); | |
| } | |
| } | |
| else if args[0] == "$" { | |
| let shell_command = args[1..].join(" "); | |
| let cwd = std::env::current_dir()?; | |
| let mut new_env = std::collections::HashMap::new(); | |
| new_env.insert(std::ffi::OsString::from("PATH"), xmas_package_manager::commands::new_path().map_err(|e| { | |
| anyhow::anyhow!("Failed to construct PATH: {}", e) | |
| })?); | |
| let exit_code = xmas_package_manager::commands::exec::shell(&shell_command, cwd, new_env, deno_task_shell::KillSignal::default()).await.map_err(|e| { | |
| anyhow::anyhow!("Failed to execute shell command: {}", e) | |
| })?; | |
| if exit_code != 0 { | |
| eprintln!("{}: Shell command exited with code {}", "Error".red().bold(), exit_code); | |
| } | |
| } | |
| else { | |
| eprintln!("{}: Unknown command '{}'", "Error".red().bold(), cmd); | |
| } | |
| match args.first() { | |
| Some(&"pm") => { | |
| if let Ok(cmd) = xmas_package_manager::cli::Subcommand::try_parse_from(&args) { | |
| let args = xmas_package_manager::Args { | |
| verbose: true, | |
| working_dir: std::env::current_dir().ok(), | |
| immutable: false, | |
| cmd | |
| }; | |
| let _ = xmas_package_manager::execute_command(&args).await; | |
| } else { | |
| eprintln!("{}: Invalid package manager command", "Error".red().bold()); | |
| } | |
| } | |
| Some(&"$") => { | |
| if args.len() <= 1 { | |
| eprintln!("{}: No shell command provided", "Error".red().bold()); | |
| } else { | |
| let shell_command = args[1..].join(" "); | |
| let cwd = std::env::current_dir()?; | |
| let mut new_env = std::collections::HashMap::new(); | |
| new_env.insert(std::ffi::OsString::from("PATH"), xmas_package_manager::commands::new_path().map_err(|e| { | |
| anyhow::anyhow!("Failed to construct PATH: {}", e) | |
| })?); | |
| let exit_code = xmas_package_manager::commands::exec::shell(&shell_command, cwd, new_env, deno_task_shell::KillSignal::default()).await.map_err(|e| { | |
| anyhow::anyhow!("Failed to execute shell command: {}", e) | |
| })?; | |
| if exit_code != 0 { | |
| eprintln!("{}: Shell command exited with code {}", "Error".red().bold(), exit_code); | |
| } | |
| } | |
| } | |
| _ => { | |
| eprintln!("{}: Unknown command '{}'", "Error".red().bold(), cmd); | |
| } | |
| } |
| pub fn log_verbose(text: &str) { | ||
| // PROGRESS_BAR.suspend(|| println!("{} {}", " VERBOSE ".on_white(), text)); | ||
| } |
There was a problem hiding this comment.
The log_verbose function has an implementation that is commented out. This means verbose logging is completely disabled. Either implement the function properly to respect verbose flags, or remove it and its calls throughout the codebase if verbose logging is not needed.
| watcher.watch(path, RecursiveMode::Recursive)?; | ||
| } | ||
|
|
||
| Ok(rx.next().await.unwrap()) |
There was a problem hiding this comment.
The unwrap call will panic if rx.next() returns None. While this might be acceptable if the watcher is guaranteed to produce at least one event, it would be better to handle this case explicitly with proper error handling or documentation explaining why this is safe.
| let s = String::deserialize(deserializer)?; | ||
| let (name, rest) = s | ||
| .split_once('!') | ||
| .ok_or_else(|| de::Error::custom("Failed to parse version"))?; |
There was a problem hiding this comment.
The error message uses generic text "Failed to parse version" which doesn't provide enough context about what went wrong. Consider providing more specific error information, such as including the actual string being parsed and what format was expected (e.g., "Expected format: 'name!version' or 'name!version?'").
| .ok_or_else(|| de::Error::custom("Failed to parse version"))?; | |
| .ok_or_else(|| { | |
| de::Error::custom(format!( | |
| "Failed to parse package specifier '{}': expected format 'name!version' or 'name!version?'", | |
| s | |
| )) | |
| })?; |
| let shell_command = args[1..].join(" "); | ||
| let cwd = std::env::current_dir()?; | ||
| let mut new_env = std::collections::HashMap::new(); | ||
| new_env.insert(std::ffi::OsString::from("PATH"), xmas_package_manager::commands::new_path().map_err(|e| { | ||
| anyhow::anyhow!("Failed to construct PATH: {}", e) | ||
| })?); | ||
| let exit_code = xmas_package_manager::commands::exec::shell(&shell_command, cwd, new_env, deno_task_shell::KillSignal::default()).await.map_err(|e| { | ||
| anyhow::anyhow!("Failed to execute shell command: {}", e) | ||
| })?; | ||
| if exit_code != 0 { | ||
| eprintln!("{}: Shell command exited with code {}", "Error".red().bold(), exit_code); |
There was a problem hiding this comment.
Potential off-by-one error or unclear edge case handling. When the shell command is "$" with no additional arguments, args[1..] will be an empty slice, resulting in an empty shell_command string. Executing an empty shell command may produce unexpected behavior. Consider validating that the command is not empty before attempting to execute it.
| let shell_command = args[1..].join(" "); | |
| let cwd = std::env::current_dir()?; | |
| let mut new_env = std::collections::HashMap::new(); | |
| new_env.insert(std::ffi::OsString::from("PATH"), xmas_package_manager::commands::new_path().map_err(|e| { | |
| anyhow::anyhow!("Failed to construct PATH: {}", e) | |
| })?); | |
| let exit_code = xmas_package_manager::commands::exec::shell(&shell_command, cwd, new_env, deno_task_shell::KillSignal::default()).await.map_err(|e| { | |
| anyhow::anyhow!("Failed to execute shell command: {}", e) | |
| })?; | |
| if exit_code != 0 { | |
| eprintln!("{}: Shell command exited with code {}", "Error".red().bold(), exit_code); | |
| if args.len() <= 1 { | |
| eprintln!("{}: No shell command provided", "Error".red().bold()); | |
| } else { | |
| let shell_command = args[1..].join(" "); | |
| let cwd = std::env::current_dir()?; | |
| let mut new_env = std::collections::HashMap::new(); | |
| new_env.insert(std::ffi::OsString::from("PATH"), xmas_package_manager::commands::new_path().map_err(|e| { | |
| anyhow::anyhow!("Failed to construct PATH: {}", e) | |
| })?); | |
| let exit_code = xmas_package_manager::commands::exec::shell(&shell_command, cwd, new_env, deno_task_shell::KillSignal::default()).await.map_err(|e| { | |
| anyhow::anyhow!("Failed to execute shell command: {}", e) | |
| })?; | |
| if exit_code != 0 { | |
| eprintln!("{}: Shell command exited with code {}", "Error".red().bold(), exit_code); | |
| } |
No description provided.