diff --git a/.changeset/astro-lockfile-resolution-parity.md b/.changeset/astro-lockfile-resolution-parity.md new file mode 100644 index 0000000000..d54ca6ad04 --- /dev/null +++ b/.changeset/astro-lockfile-resolution-parity.md @@ -0,0 +1,8 @@ +--- +"pacquet": patch +--- + +Two `pnpm install` resolution fixes that made large workspaces such as [Astro](https://github.com/withastro/astro) produce a different `pnpm-lock.yaml` than pnpm 11 [#13334](https://github.com/pnpm/pnpm/issues/13334): + +- A scoped workspace package referenced through the `file:` protocol (`"@test/pkg": "file:./pkg"`) is recorded as a `link:` again instead of being copied in as a `file:` snapshot. +- `bundledDependencies` / `bundleDependencies` are no longer resolved as dependencies of their own. npm ships them inside the package's tarball, so installing them again added packages the lockfile should not contain (for example `napi-wasm` under `@parcel/watcher-wasm`). diff --git a/pnpm/crates/resolving-deps-resolver/src/dedupe_injected_deps.rs b/pnpm/crates/resolving-deps-resolver/src/dedupe_injected_deps.rs index 5d828f3120..a3e5d00654 100644 --- a/pnpm/crates/resolving-deps-resolver/src/dedupe_injected_deps.rs +++ b/pnpm/crates/resolving-deps-resolver/src/dedupe_injected_deps.rs @@ -129,16 +129,17 @@ fn child_matches_target( /// The resolver's `pkg.id` for a `file:` workspace pick is /// emitted as `@file:` once the manifest name is in scope /// (see `build_pkg_id_with_patch_hash`) and as the bare `file:` -/// before that — accept both shapes. +/// before that — accept both shapes. Splitting on the whole `@file:` +/// separator rather than on `@` alone keeps scoped names +/// (`@scope/name@file:`) matching, whose leading `@` would +/// otherwise be taken for the separator. fn injected_workspace_target( node: &crate::dependencies_graph::DependenciesGraphNode, workspace_project_ids: &HashSet, ) -> Option { let raw = node.resolved_package_id.as_str(); - let path = raw.strip_prefix("file:").or_else(|| { - let after_at = raw.split_once('@').map(|(_, rest)| rest)?; - after_at.strip_prefix("file:") - })?; + let path = + raw.strip_prefix("file:").or_else(|| raw.split_once("@file:").map(|(_, path)| path))?; workspace_project_ids.contains(path).then(|| path.to_string()) } diff --git a/pnpm/crates/resolving-deps-resolver/src/dedupe_injected_deps/tests.rs b/pnpm/crates/resolving-deps-resolver/src/dedupe_injected_deps/tests.rs index 845359957a..fb5fbf9fd8 100644 --- a/pnpm/crates/resolving-deps-resolver/src/dedupe_injected_deps/tests.rs +++ b/pnpm/crates/resolving-deps-resolver/src/dedupe_injected_deps/tests.rs @@ -63,6 +63,36 @@ fn rewrites_childless_injected_dep_to_link() { assert!(graph.is_empty(), "unreachable file: snapshot should be pruned"); } +// Regression test for pnpm/pnpm#13334: the resolver prefixes a `file:` +// workspace pick with the manifest name, so a scoped package's id reads +// `@scope/name@file:`. Its leading `@` must not be mistaken for the +// name/id separator, or the dep never dedupes back to `link:`. +#[test] +fn rewrites_scoped_injected_dep_to_link() { + let lockfile_dir = PathBuf::from("/ws"); + let host_root = lockfile_dir.join("fixtures/host"); + let pkg_root = host_root.join("pkg"); + + let mut graph: DependenciesGraph = std::collections::HashMap::new(); + let injected = DepPath::from("@test/pkg@file:fixtures/host/pkg".to_string()); + graph.insert(injected.clone(), make_node("@test/pkg@file:fixtures/host/pkg", BTreeMap::new())); + + let mut direct: DirectByImporter = BTreeMap::new(); + direct + .insert("fixtures/host".to_string(), BTreeMap::from([("@test/pkg".to_string(), injected)])); + direct.insert("fixtures/host/pkg".to_string(), BTreeMap::new()); + + let mut roots = BTreeMap::new(); + roots.insert("fixtures/host".to_string(), host_root); + roots.insert("fixtures/host/pkg".to_string(), pkg_root); + + dedupe_injected_deps(&mut graph, &mut direct, &roots, &lockfile_dir); + + let after = direct.get("fixtures/host").unwrap().get("@test/pkg").unwrap(); + assert_eq!(after.as_str(), "link:pkg"); + assert!(graph.is_empty(), "unreachable file: snapshot should be pruned"); +} + #[test] fn leaves_injected_dep_when_children_differ() { let lockfile_dir = PathBuf::from("/ws"); diff --git a/pnpm/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs b/pnpm/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs index 953bfa1041..a4917ecd51 100644 --- a/pnpm/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs +++ b/pnpm/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs @@ -3064,25 +3064,52 @@ fn render_specifier(wanted: &WantedDependency) -> String { /// `optionalDependencies`. The walker propagates this through /// `current_is_optional` so [`ResolvedPackage::optional`] reflects /// whether every path to the node went through an optional edge. +/// +/// Names the manifest bundles are dropped: npm ships them inside the +/// package's own tarball, so resolving them again would install a +/// second copy the package never loads. fn extract_children( result: &pacquet_resolving_resolver_base::ResolveResult, ) -> Result, ResolveDependencyTreeError> { let Some(manifest) = result.manifest.as_ref() else { return Ok(Vec::new()) }; let parent = render_parent(result); + let bundled = bundled_dependency_names(manifest); let mut out = Vec::new(); - collect_deps(manifest, "dependencies", false, &parent, &mut out)?; - collect_deps(manifest, "optionalDependencies", true, &parent, &mut out)?; + collect_deps(manifest, "dependencies", false, &parent, &bundled, &mut out)?; + collect_deps(manifest, "optionalDependencies", true, &parent, &bundled, &mut out)?; for (name, specifier) in engines_runtime_dependencies(manifest, "engines", "dependencies") { out.push((name.to_string(), specifier, false)); } Ok(out) } +/// The dependency names a manifest declares as bundled, read from +/// `bundledDependencies` with `bundleDependencies` as the fallback +/// spelling. `true` stands for "every entry in `dependencies`". +fn bundled_dependency_names(manifest: &Value) -> HashSet<&str> { + let bundled = ["bundledDependencies", "bundleDependencies"] + .into_iter() + .find_map(|key| manifest.get(key).filter(|value| !value.is_null())); + match bundled { + Some(Value::Bool(true)) => manifest + .get("dependencies") + .and_then(Value::as_object) + .map(|map| map.keys().map(String::as_str).collect()) + .unwrap_or_default(), + Some(Value::Array(names)) => names.iter().filter_map(Value::as_str).collect(), + _ => HashSet::new(), + } +} + +/// Dependency names are validated before the bundled filter runs, so a +/// manifest with an unusable alias is rejected whether or not the +/// package bundles that alias. fn collect_deps( manifest: &Value, key: &str, optional: bool, parent: &str, + bundled: &HashSet<&str>, out: &mut Vec, ) -> Result<(), ResolveDependencyTreeError> { let Some(map) = manifest.get(key).and_then(Value::as_object) else { return Ok(()) }; @@ -3094,6 +3121,9 @@ fn collect_deps( alias: name.clone(), }); } + if bundled.contains(name.as_str()) { + continue; + } out.push((name.clone(), range_str.to_string(), optional)); } } diff --git a/pnpm/crates/resolving-deps-resolver/src/resolve_dependency_tree/tests.rs b/pnpm/crates/resolving-deps-resolver/src/resolve_dependency_tree/tests.rs index 353aaf3698..26089a1b4a 100644 --- a/pnpm/crates/resolving-deps-resolver/src/resolve_dependency_tree/tests.rs +++ b/pnpm/crates/resolving-deps-resolver/src/resolve_dependency_tree/tests.rs @@ -41,6 +41,88 @@ fn dependency_engines_runtime_is_walked_as_a_runtime_dependency() { ); } +fn manifest_result(manifest: serde_json::Value) -> ResolveResult { + ResolveResult { + id: PkgResolutionId::from("parent@1.0.0"), + name_ver: None, + latest: None, + published_at: None, + manifest: Some(std::sync::Arc::new(manifest)), + resolution: LockfileResolution::Directory(DirectoryResolution { + directory: "parent".to_string(), + }), + resolved_via: "npm-registry".to_string(), + normalized_bare_specifier: None, + alias: Some("parent".to_string()), + policy_violation: None, + } +} + +// Regression test for pnpm/pnpm#13334: npm ships bundled dependencies +// inside the package's own tarball, so they must not be resolved as +// edges of their own. +#[test] +fn bundled_dependencies_are_not_walked() { + let result = manifest_result(serde_json::json!({ + "name": "parent", + "version": "1.0.0", + "dependencies": { "bundled-dep": "^1.0.0", "regular-dep": "^2.0.0" }, + "optionalDependencies": { "bundled-optional": "^3.0.0" }, + "bundledDependencies": ["bundled-dep", "bundled-optional"], + })); + assert_eq!( + extract_children(&result).unwrap(), + vec![("regular-dep".to_string(), "^2.0.0".to_string(), false)], + ); +} + +#[test] +fn bundle_dependencies_spelling_is_honored() { + let result = manifest_result(serde_json::json!({ + "name": "parent", + "version": "1.0.0", + "dependencies": { "bundled-dep": "^1.0.0", "regular-dep": "^2.0.0" }, + "bundleDependencies": ["bundled-dep"], + })); + assert_eq!( + extract_children(&result).unwrap(), + vec![("regular-dep".to_string(), "^2.0.0".to_string(), false)], + ); +} + +#[test] +fn bundled_dependencies_true_bundles_every_dependency() { + let result = manifest_result(serde_json::json!({ + "name": "parent", + "version": "1.0.0", + "dependencies": { "one": "^1.0.0", "two": "^2.0.0" }, + "optionalDependencies": { "three": "^3.0.0" }, + "bundledDependencies": true, + })); + assert_eq!( + extract_children(&result).unwrap(), + vec![("three".to_string(), "^3.0.0".to_string(), true)], + ); +} + +// `bundledDependencies: true` names the `dependencies` keys, and upstream +// filters the merged `{...optionalDependencies, ...dependencies}` map, so an +// alias listed in both groups is dropped from both. +#[test] +fn bundled_dependencies_true_also_drops_the_optional_duplicate() { + let result = manifest_result(serde_json::json!({ + "name": "parent", + "version": "1.0.0", + "dependencies": { "both": "^1.0.0" }, + "optionalDependencies": { "both": "^1.0.0", "optional-only": "^3.0.0" }, + "bundledDependencies": true, + })); + assert_eq!( + extract_children(&result).unwrap(), + vec![("optional-only".to_string(), "^3.0.0".to_string(), true)], + ); +} + fn key(raw: &str) -> PkgNameVerPeer { raw.parse().expect("parse snapshot key") }