diff --git a/docs/pm/cli/pm.mdx b/docs/pm/cli/pm.mdx index 94b615753b85..1c6a2d4706ba 100644 --- a/docs/pm/cli/pm.mdx +++ b/docs/pm/cli/pm.mdx @@ -272,7 +272,7 @@ $ bun pm diff react-dom@18.2.0 18.3.1 '*.min.js' $ bun pm diff react-dom@18.2.0 18.3.1 'cjs/**/*.production.min.js' ``` -A local folder that has a `package.json` is read the way `bun pm pack` would publish it — the `files` field, `.npmignore` / `.gitignore`, `bin` — so diffing a checkout against the registry compares what would ship, not `node_modules/`, `vendor/` or build output. +A local folder that has a `package.json` is read the way `bun pm pack` would publish it — the `files` field, `.npmignore` / `.gitignore`, `bin` — so diffing a checkout against the registry compares what would ship, not `node_modules/`, `vendor/` or build output. Against a tarball or a published version, the `workspace:` and `catalog:` dependency versions in that `package.json` are compared as the versions `bun pm pack` writes in their place (from the folder's `bun.lock`), so a workspace package shows no dependency change against its own published copy. Two folders are compared as written. Piped or with `NO_COLOR`, the output is a plain unified patch of the real file contents (so `bun pm diff a b > changes.patch` applies), and `--raw` turns the re-print off on a terminal too. diff --git a/src/parsers/json.rs b/src/parsers/json.rs index 60b64891831a..6eed61816931 100644 --- a/src/parsers/json.rs +++ b/src/parsers/json.rs @@ -798,7 +798,8 @@ fn array_next_item(contents: &[u8], item: usize) -> Option { (!matches!(contents[p], b']' | b',')).then_some(p) } -fn skip_string_token(contents: &[u8], start: usize) -> Option { +/// Offset one past the closing quote of the string token whose opening quote is at `start`. +pub fn skip_string_token(contents: &[u8], start: usize) -> Option { let quote = *contents.get(start)?; if quote != b'"' && quote != b'\'' { return None; diff --git a/src/runtime/cli/pack_command.rs b/src/runtime/cli/pack_command.rs index c523a546df1d..6673f96d4b96 100644 --- a/src/runtime/cli/pack_command.rs +++ b/src/runtime/cli/pack_command.rs @@ -3325,6 +3325,80 @@ fn add_archive_entry( Ok(entry.clear()) } +/// Why `published_version` has no version for a `workspace:` or `catalog:` dependency. +#[derive(Clone, Copy)] +pub(crate) enum UnresolvedVersion { + /// `workspace:^`, `workspace:~` or `workspace:*`, and the lockfile has no version for that workspace. + Workspace, + /// `catalog:`, and there is no lockfile to read the catalogs from. + CatalogWithoutLockfile, + /// `catalog:`, and that catalog has no entry for the dependency. + CatalogEntry, +} + +impl UnresolvedVersion { + /// The error `bun pm pack` stops with. The `{}`s are the dependency's name and its package.json section. + fn message(self) -> &'static str { + match self { + UnresolvedVersion::Workspace => { + "Failed to resolve workspace version for \"{}\" in `{}`. Run `bun install` and try again." + } + UnresolvedVersion::CatalogWithoutLockfile => { + "Failed to resolve catalog version for \"{}\" in `{}` (catalogs require a lockfile)." + } + UnresolvedVersion::CatalogEntry => { + "Failed to resolve catalog version for \"{}\" in `{}` (no matching catalog dependency)." + } + } + } +} + +/// What the tarball's package.json gets in place of a `workspace:` or `catalog:` version. `None`: `spec` uses neither. +pub(crate) fn published_version( + maybe_lockfile: Option<&Lockfile>, + dependency_name: &[u8], + spec: &[u8], +) -> Option, UnresolvedVersion>> { + if let Some(range) = strings::without_prefix_if_possible_comptime(spec, b"workspace:") { + // Only a bare `^`, `~` or `*` takes the workspace's version. Any other range is published as written. + let prefix: &[u8] = match range { + b"^" => b"^", + b"~" => b"~", + b"*" => b"", + _ => return Some(Ok(range.to_vec())), + }; + let Some(lockfile) = maybe_lockfile else { + return Some(Err(UnresolvedVersion::Workspace)); + }; + let Some(workspace_version) = lockfile + .workspace_versions + .get(&Semver::string::Builder::string_hash(dependency_name)) + else { + return Some(Err(UnresolvedVersion::Workspace)); + }; + return Some(Ok(format!( + "{}{}", + bstr::BStr::new(prefix), + workspace_version.fmt(lockfile.buffers.string_bytes.as_slice()), + ) + .into_bytes())); + } + + let catalog_name = strings::without_prefix_if_possible_comptime(spec, b"catalog:")?; + let Some(lockfile) = maybe_lockfile else { + return Some(Err(UnresolvedVersion::CatalogWithoutLockfile)); + }; + let map_buf: &[u8] = lockfile.buffers.string_bytes.as_slice(); + let catalog_name = strings::trim(catalog_name, &strings::WHITESPACE_CHARS); + let Some(dep) = lockfile + .catalogs + .find(map_buf, catalog_name, dependency_name) + else { + return Some(Err(UnresolvedVersion::CatalogEntry)); + }; + Some(Ok(dep.version.literal.slice(map_buf).to_vec())) +} + /// Strips workspace and catalog protocols from dependency versions then /// returns the printed json fn edit_root_package_json( @@ -3344,85 +3418,26 @@ fn edit_root_package_json( if let Some(dependencies_expr) = json.root.get(dependency_group) { if let ExprData::EObject(mut dependencies) = dependencies_expr.data { for dependency in dependencies.properties.slice_mut() { - if dependency.key.is_none() { - continue; - } - if dependency.value.is_none() { - continue; - } - - let Some(package_spec) = dependency - .value - .as_ref() - .expect("infallible: prop has value") - .as_utf8_string_literal() - else { + let (Some(dependency_name), Some(package_spec)) = ( + dependency + .key + .as_ref() + .and_then(Expr::as_utf8_string_literal), + dependency + .value + .as_ref() + .and_then(Expr::as_utf8_string_literal), + ) else { continue; }; - if let Some(without_workspace_protocol) = - strings::without_prefix_if_possible_comptime(package_spec, b"workspace:") - { - // TODO: make semver parsing more strict. `^`, `~` are not valid - if without_workspace_protocol.len() == 1 { - // TODO: this might be too strict - let c = without_workspace_protocol[0]; - if c == b'^' || c == b'~' || c == b'*' { - let dependency_name = match dependency - .key - .as_ref() - .expect("infallible: prop has key") - .as_utf8_string_literal() - { - Some(n) => n, - None => { - Output::err_generic( - "expected string value for dependency name in \"{}\"", - format_args!("{}", bstr::BStr::new(dependency_group)), - ); - Global::crash(); - } - }; - - let resolved = 'failed_to_resolve: { - // find the current workspace version and append to package spec without `workspace:` - let Some(lockfile) = maybe_lockfile else { - break 'failed_to_resolve false; - }; - let Some(workspace_version) = lockfile.workspace_versions.get( - &Semver::string::Builder::string_hash(dependency_name), - ) else { - break 'failed_to_resolve false; - }; - let prefix: &[u8] = match c { - b'^' => b"^", - b'~' => b"~", - b'*' => b"", - _ => unreachable!(), - }; - // Format on the heap then copy into the - // pack arena; `EString::init` erases the - // lifetime. - let tmp = format!( - "{}{}", - bstr::BStr::new(prefix), - workspace_version - .fmt(lockfile.buffers.string_bytes.as_slice()), - ); - let data = pack_bump().alloc_slice_copy(tmp.as_bytes()); - dependency.value = Some(Expr::init( - E::EString::init(data), - Default::default(), - )); - true - }; - if resolved { - continue; - } - - // only produce this error only when we need to get the workspace version + let version = + match published_version(maybe_lockfile, dependency_name, package_spec) { + None => continue, + Some(Ok(version)) => version, + Some(Err(unresolved)) => { Output::err_generic( - "Failed to resolve workspace version for \"{}\" in `{}`. Run `bun install` and try again.", + unresolved.message(), ( bstr::BStr::new(dependency_name), bstr::BStr::new(dependency_group), @@ -3430,55 +3445,11 @@ fn edit_root_package_json( ); Global::crash(); } - } - - let dup = pack_bump().alloc_slice_copy(without_workspace_protocol); - dependency.value = - Some(Expr::init(E::EString::init(dup), Default::default())); - } else if let Some(catalog_name_str) = - strings::without_prefix_if_possible_comptime(package_spec, b"catalog:") - { - let dep_name_str = dependency - .key - .as_ref() - .expect("infallible: prop has key") - .as_utf8_string_literal() - .expect("infallible: is_string checked"); - - let lockfile = match maybe_lockfile { - Some(l) => l, - None => { - Output::err_generic( - "Failed to resolve catalog version for \"{}\" in `{}` (catalogs require a lockfile).", - ( - bstr::BStr::new(dep_name_str), - bstr::BStr::new(dependency_group), - ), - ); - Global::crash(); - } }; - let map_buf: &[u8] = lockfile.buffers.string_bytes.as_slice(); - let catalog_name = - strings::trim(catalog_name_str, &strings::WHITESPACE_CHARS); - let Some(dep) = lockfile.catalogs.find(map_buf, catalog_name, dep_name_str) - else { - Output::err_generic( - "Failed to resolve catalog version for \"{}\" in `{}` (no matching catalog dependency).", - ( - bstr::BStr::new(dep_name_str), - bstr::BStr::new(dependency_group), - ), - ); - Global::crash(); - }; - - let literal = - pack_bump().alloc_slice_copy(dep.version.literal.slice(map_buf)); - dependency.value = - Some(Expr::init(E::EString::init(literal), Default::default())); - } + // `EString::init` erases the lifetime, so the bytes go into the pack arena. + let data = pack_bump().alloc_slice_copy(&version); + dependency.value = Some(Expr::init(E::EString::init(data), Default::default())); } } } diff --git a/src/runtime/cli/pm_diff_command.rs b/src/runtime/cli/pm_diff_command.rs index 6a0a3bf9c792..cc62a8d9846e 100644 --- a/src/runtime/cli/pm_diff_command.rs +++ b/src/runtime/cli/pm_diff_command.rs @@ -15,7 +15,7 @@ use bun_install::dependency; use bun_install::lockfile::LoadResult; use bun_install::lockfile::package::PackageColumns as _; use bun_install::npm::{self, PackageManifest}; -use bun_install::{PackageManager, resolution}; +use bun_install::{Lockfile, PackageManager, resolution}; use bun_libarchive::lib::{ArchiveIterator, IteratorResult as ArchiveIterResult}; use bun_semver as Semver; use bun_sys::{Fd, FdExt as _, dir_iterator as DirIterator}; @@ -101,13 +101,15 @@ pub(crate) fn exec( .collect(); let (mut left_spec, mut right_spec) = resolve_sides(pm, &args, original_cwd); + // Two folders compare as written: the patch between them has to apply to the files on disk. + let as_published = !matches!((&left_spec, &right_spec), (Spec::Dir(_), Spec::Dir(_))); // `bun pm diff ./pkg` / `./pkg 2.0.0`: a registry side without a name takes it from the local side's package.json. let mut left_early: Option = None; if let (Spec::Dir(_) | Spec::Tarball(_), Spec::Registry { name, .. }) = (&left_spec, &mut right_spec) { if name.is_empty() { - let local = materialize(pm, &left_spec)?; + let local = materialize(pm, &left_spec, as_published)?; *name = package_name_in(&local).unwrap_or_else(|| { Status::clear(); Output::err_generic( @@ -119,7 +121,7 @@ pub(crate) fn exec( left_early = Some(local); } } - let mut right = materialize(pm, &right_spec)?; + let mut right = materialize(pm, &right_spec, as_published)?; if let Spec::Registry { name, .. } = &mut left_spec { if name.is_empty() { *name = package_name_in(&right).unwrap_or_else(|| { @@ -134,7 +136,7 @@ pub(crate) fn exec( } let mut left = match left_early { Some(tree) => tree, - None => materialize(pm, &left_spec)?, + None => materialize(pm, &left_spec, as_published)?, }; // Show local paths the way they were typed, not as resolved against the invoking folder. for (tree, spec) in [(&mut left, &left_spec), (&mut right, &right_spec)] { @@ -454,7 +456,11 @@ impl Drop for Status { } } -fn materialize(pm: &mut PackageManager, spec: &Spec) -> Result { +fn materialize( + pm: &mut PackageManager, + spec: &Spec, + as_published: bool, +) -> Result { let _status = match spec { Spec::Registry { name, version } => { Status::show("fetching", &[&name[..], b"@", &version[..]].concat()) @@ -480,7 +486,7 @@ fn materialize(pm: &mut PackageManager, spec: &Spec) -> Result read_dir_tree(path), + Spec::Dir(path) => read_dir_tree(path, as_published), } } @@ -541,7 +547,7 @@ fn read_tarball_into(bytes: &[u8], tree: &mut Tree) -> Result<(), crate::Error> Ok(()) } -fn read_dir_tree(root: &[u8]) -> Result { +fn read_dir_tree(root: &[u8], as_published: bool) -> Result { let mut tree = Tree { label: root.to_vec(), files: BTreeMap::new(), @@ -569,6 +575,13 @@ fn read_dir_tree(root: &[u8]) -> Result { // bins — not the whole checkout with its vendor/ and build/. 'package: { if let Ok(pkg) = bun_sys::File::read_from(root_fd, b"package.json") { + // Before the parse below: loading a lockfile resets the AST store that parse allocates from. + let mut lockfile = Lockfile::default(); + let lockfile = if as_published { + lockfile_for(&mut lockfile, root, &pkg) + } else { + None + }; let bump = Bump::new(); let src: &[u8] = bump.alloc_slice_copy(&pkg); if let Ok(json) = bun_parsers::json::parse_utf8( @@ -613,6 +626,11 @@ fn read_dir_tree(root: &[u8]) -> Result { Err(err) => fail(err, rel), } } + let pkg = if as_published { + with_published_versions(pkg, &json, lockfile) + } else { + pkg + }; tree.files.insert(b"package.json".to_vec(), pkg); root_fd.close(); return Ok(tree); @@ -694,6 +712,140 @@ fn read_dir_tree(root: &[u8]) -> Result { Ok(tree) } +/// The lockfile `bun pm pack` reads in `dir`: the nearest one above it that lists `dir` as a workspace, else `dir`'s own. +fn lockfile_for<'a>( + lockfile: &'a mut Lockfile, + dir: &[u8], + manifest: &[u8], +) -> Option<&'a Lockfile> { + use bun_paths::resolve_path::{join_abs_string_buf, platform}; + // A JSON escape can spell either protocol, so a manifest with a backslash in it loads the lockfile too. + if !strings::contains(manifest, b"workspace:") + && !strings::contains(manifest, b"catalog:") + && !strings::contains_char(manifest, b'\\') + { + return None; + } + // `Some(true)`: loaded. `Some(false)`: there is one, and it does not load. `None`: there is none. + fn load(lockfile: &mut Lockfile, dir: &[u8]) -> Option { + let fd = bun_sys::open_dir_at(Fd::cwd(), dir).ok()?; + let loaded = match lockfile.load_from_dir::(fd, None, &mut bun_ast::Log::init()) { + LoadResult::Ok(_) => Some(true), + LoadResult::NotFound => None, + LoadResult::Err(_) => Some(false), + }; + fd.close(); + loaded + } + let (mut dir_buf, mut buf) = ( + bun_paths::path_buffer_pool::get(), + bun_paths::path_buffer_pool::get(), + ); + // Every path compared goes through the same join, so `./packages/a/` and `packages/a` are one folder. + let dir = strings::without_trailing_slash(join_abs_string_buf::( + dir, + &mut dir_buf[..], + &[b"."], + )); + // A workspace is packed from its project's root, so only a lockfile above `dir` that lists it counts here. + let mut root = dir; + let mut is_workspace = false; + while let Some(parent) = bun_paths::dirname(root) + .map(strings::without_trailing_slash) + .filter(|parent| parent.len() < root.len()) + { + root = parent; + if load(lockfile, root) != Some(true) { + continue; + } + let string_buf = lockfile.buffers.string_bytes.as_slice(); + is_workspace = lockfile.packages.items_resolution().iter().any(|res| { + res.tag == resolution::Tag::Workspace + && strings::without_trailing_slash(join_abs_string_buf::( + root, + &mut buf[..], + &[res.workspace().slice(string_buf)], + )) == dir + }); + if is_workspace { + break; + } + } + (is_workspace || load(lockfile, dir) == Some(true)).then_some(&*lockfile) +} + +/// Whether `token`, a JSON string with its quotes, decodes to `value`. `"workspace:\u005e"` reads as `workspace:^`. +fn json_string_reads_as(token: &[u8], value: &[u8]) -> bool { + let bump = Bump::new(); + bun_parsers::json::parse_utf8( + &bun_ast::Source::init_path_string(b"package.json", token), + &mut bun_ast::Log::init(), + &bump, + ) + .is_ok_and(|string| string.as_utf8_string_literal() == Some(value)) +} + +/// `manifest` (a package.json, parsed as `json`) with its `workspace:` and `catalog:` versions as `bun pm pack` publishes them. +fn with_published_versions( + manifest: Vec, + json: &bun_js_parser::Expr, + lockfile: Option<&Lockfile>, +) -> Vec { + // The string token of each version to replace (its start and end in `manifest`), and what replaces it. + let mut edits: Vec<(usize, usize, Vec)> = Vec::new(); + for section in bun_install_types::DependencyGroup::FOUR { + let Some(dependencies) = json.get_object(section.prop) else { + continue; + }; + let Some(dependencies) = dependencies.data.e_object() else { + continue; + }; + for dependency in dependencies.properties.slice() { + let (Some(name), Some(version)) = (&dependency.key, &dependency.value) else { + continue; + }; + let (Some(name), Some(spec)) = ( + name.as_utf8_string_literal(), + version.as_utf8_string_literal(), + ) else { + continue; + }; + // A version that does not resolve stays as written: pack refuses to publish it. + let Some(Ok(published)) = + crate::cli::pack_command::published_version(lockfile, name, spec) + else { + continue; + }; + // Only a token that reads as the value the parser gave is replaced. + let token = usize::try_from(version.loc.start).ok().and_then(|at| { + let end = bun_parsers::json::skip_string_token(&manifest, at)?; + json_string_reads_as(&manifest[at..end], spec).then_some((at, end)) + }); + if let Some((at, end)) = token { + edits.push((at, end, published)); + } + } + } + if edits.is_empty() { + return manifest; + } + edits.sort_unstable_by_key(|&(at, ..)| at); + edits.dedup_by_key(|&mut (at, ..)| at); + let mut out: Vec = Vec::with_capacity(manifest.len()); + let mut copied = 0usize; + for (at, end, published) in &edits { + out.extend_from_slice(&manifest[copied..*at]); + let _ = write!( + out, + "{}", + bun_core::fmt::format_json_string_utf8(published, Default::default()) + ); + copied = *end; + } + out.extend_from_slice(&manifest[copied..]); + out +} + /// Fetches `name`'s manifest, resolves `version` (exact, range, or dist-tag), downloads that tarball and unpacks it in memory. fn fetch_registry_tree( pm: &mut PackageManager, diff --git a/test/cli/install/bun-pm-diff.test.ts b/test/cli/install/bun-pm-diff.test.ts index 70d18fe8d43e..d5e51327728d 100644 --- a/test/cli/install/bun-pm-diff.test.ts +++ b/test/cli/install/bun-pm-diff.test.ts @@ -270,6 +270,109 @@ diffme@1.0.0 → diffme@2.0.0 expect(exitCode).toBe(0); }); + test("a workspace package's workspace: and catalog: versions are compared as `bun pm pack` publishes them", async () => { + const manifest = (pkg: object) => JSON.stringify(pkg, null, 2) + "\n"; + // One version is spelled with a JSON escape: pack resolves what the string decodes to. + const a = manifest({ + name: "ws-a", + version: "1.0.0", + dependencies: { "ws-b": "workspace:^", diffme: "catalog:" }, + devDependencies: { "ws-c": "workspace:2.x" }, + peerDependencies: { "ws-b": "workspace:*", diffme: "catalog:legacy" }, + optionalDependencies: { "ws-d": "workspace:~" }, + }).replace('"workspace:~"', '"workspace:\\u007e"'); + expect(a).toContain('"ws-d": "workspace:\\u007e"'); + using dir = tempDir("pm-diff-workspace", { + "package.json": manifest({ + name: "ws-root", + private: true, + workspaces: { + packages: ["packages/*"], + catalog: { diffme: "^1.0.0" }, + catalogs: { legacy: { diffme: "1.0.0" } }, + }, + }), + "packages/a/package.json": a, + "packages/a/index.js": "module.exports = 1;\n", + // Pack reads the lockfile of the project root, so these two must not get in the way: one in the + // workspace's own folder that does not load, one between it and the root that lists nothing. + "packages/a/bun.lock": "{ not a lockfile", + "packages/bun.lock": manifest({ lockfileVersion: 1, workspaces: { "": { name: "stray" } }, packages: {} }), + "packages/b/package.json": manifest({ name: "ws-b", version: "1.0.1" }), + "packages/c/package.json": manifest({ name: "ws-c", version: "2.3.4" }), + "packages/d/package.json": manifest({ name: "ws-d", version: "3.0.0" }), + // The same files outside the `workspaces` globs: `bun pm pack` has no lockfile to resolve these from. + "vendor/a/package.json": a, + "vendor/a/index.js": "module.exports = 1;\n", + "out/.keep": "", + }); + await using install = Bun.spawn({ + cmd: [bunExe(), "install"], + cwd: String(dir), + env: { ...bunEnv, NPM_CONFIG_REGISTRY: registry, BUN_CONFIG_REGISTRY: registry }, + stdout: "pipe", + stderr: "pipe", + }); + const [, installErr, installExit] = await Promise.all([ + install.stdout.text(), + install.stderr.text(), + install.exited, + ]); + expect(installErr).not.toContain("error:"); + expect(installExit).toBe(0); + const member = join(String(dir), "packages", "a"); + const tgz = await pack(member, join(String(dir), "out")); + + // The dependency versions pack just wrote are the ones this folder shows, from inside the package and from the root. + expect(await diff([tgz, "."], member)).toEqual({ + stdout: `${tgz} → .\nNo differences (2 files)\n`, + stderr: "", + exitCode: 0, + }); + expect(await diff([tgz, "./packages/a/"], String(dir))).toEqual({ + stdout: `${tgz} → ./packages/a/\nNo differences (2 files)\n`, + stderr: "", + exitCode: 0, + }); + + // The lockfile is the folder's own, not the one of the project the command runs in. + expect(await diff([tgz, "../../packages/a"], join(String(dir), "vendor", "a"))).toEqual({ + stdout: `${tgz} → ../../packages/a\nNo differences (2 files)\n`, + stderr: "", + exitCode: 0, + }); + // An absolute path is used as typed, so this one reaches the lockfile lookup with its `..` still in it. + const roundabout = `${String(dir)}/packages/b/../a`; + expect(await diff([tgz, roundabout], join(String(dir), "vendor", "a"))).toEqual({ + stdout: `${tgz} → ${roundabout}\nNo differences (2 files)\n`, + stderr: "", + exitCode: 0, + }); + + // A folder the nearest lockfile does not list keeps what it says: `workspace:2.x` is published as `2.x` + // anywhere, and the others are not taken from a workspace that has the same name. + const vendored = await diff([tgz, "./vendor/a", "--json"], String(dir)); + expect(vendored.stderr).toBe(""); + expect(JSON.parse(vendored.stdout)).toMatchObject({ + files: [{ path: "package.json", linesAdded: 5, linesRemoved: 5 }], + notes: [ + "dependencies ws-b: ^1.0.1 → workspace:^", + "dependencies diffme: ^1.0.0 → catalog:", + "optionalDependencies ws-d: ~3.0.0 → workspace:~", + "peerDependencies ws-b: 1.0.1 → workspace:*", + "peerDependencies diffme: 1.0.0 → catalog:legacy", + ], + }); + expect(vendored.exitCode).toBe(0); + + // Two folders compare as written, so the patch between them applies to the files on disk. + expect(await diff(["./vendor/a", "./packages/a"], String(dir))).toEqual({ + stdout: "./vendor/a → ./packages/a\nNo differences (2 files)\n", + stderr: "", + exitCode: 0, + }); + }); + test("patch output marks a side that ends without a newline", async () => { using dir = tempDir("pm-diff-nonl", { "a/package.json": `{"name":"x","version":"1.0.0"}`,