feat: enable wasm32 compilation by making xx crate conditional#641
feat: enable wasm32 compilation by making xx crate conditional#641andreaTP wants to merge 1 commit into
xx crate conditional#641Conversation
Make the xx crate dependency conditional on non-wasm32 targets and replace xx::regex!, xx::file, and xx::process usages with std equivalents (LazyLock<Regex>, std::fs, std::process) so the codebase compiles for wasm32-wasip1. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Greptile SummaryThis PR makes the
Confidence Score: 4/5Safe to merge; all functional changes are correct and the wasm stub behaviour is well-scoped. The core wasm32 gating logic is sound across both crates, and the std-equivalent replacements are semantically identical to the xx helpers they replace. The only items worth a second look are a lost doc-comment in complete_word.rs and the mismatched error types on the two cfg-variants of the local sh function — neither affects runtime behaviour on any current target. cli/src/cli/complete_word.rs — removed doc-comments and mismatched sh return types worth tidying before the PR lands. Important Files Changed
Reviews (1): Last reviewed commit: "feat: enable wasm32 compilation by makin..." | Re-trigger Greptile |
| @@ -393,12 +390,14 @@ fn zsh_shell_quote(s: &str) -> String { | |||
| if !s.is_empty() && s.chars().all(safe) { | |||
| return s.to_string(); | |||
| } | |||
| // Wrap in single quotes; close-open dance escapes any internal apostrophes. | |||
| let escaped = s.replace('\'', "'\\''"); | |||
| format!("'{escaped}'") | |||
There was a problem hiding this comment.
Doc-comment inadvertently removed
The three-line /// doc-comment for zsh_shell_quote and the inline // Wrap in single quotes… comment inside the body were deleted in this diff, but they are unrelated to the wasm32 changes. The doc-comment explains why the function chooses consistent single-quote quoting over zsh's default mix of styles — useful context for anyone working on completion output later.
| usage-lib = { workspace = true, features = ["clap", "docs", "unstable_choices_env"] } | ||
| [target.'cfg(not(target_arch = "wasm32"))'.dependencies] | ||
| xx = "2" |
There was a problem hiding this comment.
Missing blank line before the new
[target.…] section makes the TOML harder to read and deviates from the project's existing style (compare with the [target.'cfg(unix)'.dependencies] section below, which has a blank line before it).
| usage-lib = { workspace = true, features = ["clap", "docs", "unstable_choices_env"] } | |
| [target.'cfg(not(target_arch = "wasm32"))'.dependencies] | |
| xx = "2" | |
| usage-lib = { workspace = true, features = ["clap", "docs", "unstable_choices_env"] } | |
| [target.'cfg(not(target_arch = "wasm32"))'.dependencies] | |
| xx = "2" |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| versions = "7" | ||
| [target.'cfg(not(target_arch = "wasm32"))'.dependencies] | ||
| xx = "2" |
There was a problem hiding this comment.
Same missing blank-line issue as in
cli/Cargo.toml — the new [target.…] section header is run directly into the last [dependencies] entry without a blank line separator.
| versions = "7" | |
| [target.'cfg(not(target_arch = "wasm32"))'.dependencies] | |
| xx = "2" | |
| versions = "7" | |
| [target.'cfg(not(target_arch = "wasm32"))'.dependencies] | |
| xx = "2" |
| @@ -413,3 +412,8 @@ fn sh(script: &str) -> XXResult<String> { | |||
| let stdout = String::from_utf8(output.stdout).expect("stdout is not utf-8"); | |||
| Ok(stdout) | |||
| } | |||
|
|
|||
| #[cfg(target_arch = "wasm32")] | |||
| fn sh(_script: &str) -> miette::Result<String> { | |||
| Err(miette::miette!("shell execution is not supported on wasm")) | |||
| } | |||
There was a problem hiding this comment.
Inconsistent return types between target variants of
sh
The non-wasm32 variant returns xx::XXResult<String> (Result<String, xx::XXError>) while the wasm32 variant returns miette::Result<String> (Result<String, miette::Report>). Both happen to coerce correctly through ? into the caller's miette::Result, but the two arms of the same logical function having different error types can be surprising. For consistency with lib/src/sh.rs — and to make future callers easier — consider giving the wasm32 arm the same outer return-type shape, either by defining a local alias or by aligning on miette::Result in both arms.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Code Review
This pull request refactors the codebase to support wasm32 targets by making the xx crate dependency optional and replacing its utilities—such as file I/O and regex macros—with standard library functions and the regex crate using LazyLock. It also introduces conditional compilation for shell execution and error types. Feedback focuses on unifying return types for target-specific functions, restoring accidentally removed documentation, adopting more idiomatic error handling with into_diagnostic(), and moving lazily initialized regexes to the module level for improved performance and readability.
| use xx::{XXError, XXResult}; | ||
|
|
||
| #[cfg(not(target_arch = "wasm32"))] | ||
| pub(crate) fn sh(script: &str) -> XXResult<String> { |
There was a problem hiding this comment.
The return type of the non-wasm sh implementation (XXResult<String>) differs from the wasm implementation (Result<String, UsageErr>). For consistency and to avoid issues in target-agnostic callers, both should return Result<String, UsageErr>. Since UsageErr already implements From<XXError>, this change is straightforward.
| pub(crate) fn sh(script: &str) -> XXResult<String> { | |
| pub(crate) fn sh(script: &str) -> Result<String, crate::error::UsageErr> { |
| let stdout = sh(&run)?; | ||
| // trace!("stdout: {stdout}"); | ||
| let re = regex!(r"[^\\]:"); | ||
| static COLON_RE: LazyLock<Regex> = LazyLock::new(|| Regex::new(r"[^\\]:").unwrap()); |
There was a problem hiding this comment.
| fn zsh_shell_quote(s: &str) -> String { | ||
| fn safe(c: char) -> bool { | ||
| matches!(c, |
There was a problem hiding this comment.
The documentation comments for zsh_shell_quote were accidentally removed. These provide valuable context on why this specific quoting strategy is used for zsh completions and should be restored.
/// Wrap a completion value in single quotes if any character would otherwise
/// be interpreted by the shell. The result is meant to be inserted by
/// compadd -Q verbatim, so the user sees consistent single-quote quoting
/// instead of zsh's default mix of backslash and single-quote styles.
fn zsh_shell_quote(s: &str) -> String {References
- Prefer Rust-side shell quoting (e.g., zsh_shell_quote) over shell-side manual unescaping logic for completion scripts to ensure robustness and maintainability.
| std::fs::create_dir_all(parent).map_err(|e| miette::miette!("{e}"))?; | ||
| } | ||
| std::fs::write(path, format!("{}\n", md.trim())) | ||
| .map_err(|e| miette::miette!("{e}"))?; |
There was a problem hiding this comment.
Instead of manually mapping the error to a miette report using a string, you can use into_diagnostic() which is more idiomatic in this codebase and preserves the underlying error context.
if let Some(parent) = path.parent() {
std::fs::create_dir_all(parent).into_diagnostic()?;
}
std::fs::write(path, format!("{}\n", md.trim()))
.into_diagnostic()?;| static RE: LazyLock<Regex> = LazyLock::new(|| Regex::new(r"(`[^`]*`)|(<)").unwrap()); | ||
| &RE | ||
| } |
| if full.starts_with("#!") { | ||
| let usage_regex = xx::regex!(r"^(?:#|//|::)(?:USAGE| ?\[USAGE\])"); | ||
| if full.lines().any(|l| usage_regex.is_match(l)) { | ||
| static USAGE_RE: LazyLock<Regex> = LazyLock::new(|| Regex::new(r"^(?:#|//|::)(?:USAGE| ?\[USAGE\])").unwrap()); |
Supersedes #570 and #561 — this PR contains only the Rust-level changes needed for wasm32 support to avoid having to patch on top downstream.
The
xxcrate doesn't support wasm32. This makesxxa conditional dependency (cfg(not(target_arch = "wasm32"))) and replaces its usages (xx::regex!,xx::file,xx::process) with std equivalents.shexecution is disabled on wasm (returns an error).CI now includes a minimal
cargo build --target wasm32-wasip1check to prevent regressions and this is all I need to bundle and ship ausage4jlibrary usable and embeddable in Java.Please @jdx have a look and let me know your thoughts 🙏