mirror of
https://github.com/pnpm/pnpm.git
synced 2026-07-18 19:52:31 -04:00
fix(pacquet/lockfile): accept link: refs in snapshot dependencies (#11788)
* fix(lockfile): accept link: refs in snapshot dependencies An injected workspace package's snapshot can carry a `link:<path>` dependency value when the dep is a workspace sibling. Pnpm accepts the shape — `refToRelative` short-circuits to `null` for `link:` — but pacquet's `SnapshotDepRef` only handled the plain/alias shapes and rejected `link:` at parse time. Add a `SnapshotDepRef::Link(String)` variant mirroring `ImporterDepVersion::Link`, return `None` from `resolve` / `ver_peer` for it, and skip it at every consumer (matching upstream's `if (childDepPath)` guards in `getChildren` / `lockfileDepsToGraphChildren`). Fixes https://github.com/pnpm/pnpm/issues/11775. * test(lockfile): trim redundant doc comments on link snapshot dep tests Drop test doc-comments that restated the assertion. The test name + asserts already convey the intent; the upstream-parity reason lives at the `SnapshotDepRef::resolve` / `Link` variant docs.
This commit is contained in:
@@ -1,5 +1,6 @@
|
||||
use crate::{
|
||||
DirectoryResolution, ImporterDepVersion, Lockfile, LockfileResolution, PackageKey, PkgName,
|
||||
SnapshotDepRef,
|
||||
};
|
||||
use pretty_assertions::assert_eq;
|
||||
use tempfile::tempdir;
|
||||
@@ -215,3 +216,68 @@ snapshots:
|
||||
let snapshots = lockfile.snapshots.as_ref().expect("snapshots present");
|
||||
assert!(snapshots.contains_key(&key));
|
||||
}
|
||||
|
||||
/// Regression test for <https://github.com/pnpm/pnpm/issues/11775>.
|
||||
/// An injected workspace package's snapshot can hold a `link:<path>`
|
||||
/// value in its `dependencies:` map when the dep is a workspace
|
||||
/// sibling. Pnpm's own parser accepts the shape — `refToRelative`
|
||||
/// short-circuits to `null` for `link:` references at use time — so
|
||||
/// pacquet must too.
|
||||
#[test]
|
||||
fn parses_link_dep_in_injected_snapshot() {
|
||||
let lockfile_text = text_block! {
|
||||
"lockfileVersion: '9.0'"
|
||||
""
|
||||
"settings:"
|
||||
" autoInstallPeers: true"
|
||||
" excludeLinksFromLockfile: false"
|
||||
""
|
||||
"importers:"
|
||||
""
|
||||
" .: {}"
|
||||
""
|
||||
" packages/a:"
|
||||
" dependencies:"
|
||||
" b:"
|
||||
" specifier: workspace:^"
|
||||
" version: file:packages/b"
|
||||
" dependenciesMeta:"
|
||||
" b:"
|
||||
" injected: true"
|
||||
""
|
||||
" packages/b:"
|
||||
" dependencies:"
|
||||
" c:"
|
||||
" specifier: workspace:^"
|
||||
" version: link:../c"
|
||||
""
|
||||
" packages/c: {}"
|
||||
""
|
||||
"packages:"
|
||||
""
|
||||
" b@file:packages/b:"
|
||||
" resolution: {directory: packages/b, type: directory}"
|
||||
""
|
||||
"snapshots:"
|
||||
""
|
||||
" b@file:packages/b:"
|
||||
" dependencies:"
|
||||
" c: link:packages/c"
|
||||
};
|
||||
let tmp = write_lockfile(lockfile_text);
|
||||
let virtual_store_dir = tmp.path().join("node_modules").join(".pacquet");
|
||||
|
||||
let lockfile = Lockfile::load_current_from_virtual_store_dir(&virtual_store_dir)
|
||||
.expect("load lockfile with link: snapshot dep")
|
||||
.expect("lockfile should be present");
|
||||
|
||||
let snapshots = lockfile.snapshots.as_ref().expect("snapshots present");
|
||||
let b_key: PackageKey = "b@file:packages/b".parse().expect("parse b key");
|
||||
let b_snapshot = snapshots.get(&b_key).expect("b snapshot present");
|
||||
let deps = b_snapshot.dependencies.as_ref().expect("b deps present");
|
||||
|
||||
let c_name = PkgName::parse("c").expect("parse c");
|
||||
let c_ref = deps.get(&c_name).expect("c entry present");
|
||||
assert_eq!(c_ref, &SnapshotDepRef::Link("packages/c".to_string()));
|
||||
assert_eq!(c_ref.resolve(&c_name), None);
|
||||
}
|
||||
|
||||
@@ -48,8 +48,10 @@ pub struct ResolvedDependencySpec {
|
||||
///
|
||||
/// Snapshot-level dependencies (the values inside `snapshots.*.dependencies`)
|
||||
/// use [`crate::SnapshotDepRef`] instead, which carries the same
|
||||
/// plain/alias distinction but never holds a `link:` value — `link:`
|
||||
/// only appears at the importer level.
|
||||
/// plain / alias / link distinction. `link:` can appear at the snapshot
|
||||
/// level too, for injected workspace packages whose own dependencies
|
||||
/// resolve to other workspace projects (see
|
||||
/// [`crate::SnapshotDepRef::Link`]).
|
||||
#[derive(Debug, Clone, PartialEq, Eq, Hash)]
|
||||
pub enum ImporterDepVersion {
|
||||
/// Bare semver-with-peer; resolves to a snapshot in `snapshots:`
|
||||
|
||||
@@ -6,7 +6,7 @@ use std::{borrow::Cow, str::FromStr};
|
||||
/// Value of a single entry in [`SnapshotEntry::dependencies`](crate::SnapshotEntry::dependencies)
|
||||
/// (or `optional_dependencies`).
|
||||
///
|
||||
/// A snapshot dependency can be written in one of two forms:
|
||||
/// A snapshot dependency can be written in one of three forms:
|
||||
///
|
||||
/// * A bare version with an optional peer-dependency suffix — the dependency
|
||||
/// resolves to `<alias-name>@<version>` in the `snapshots:` map.
|
||||
@@ -27,9 +27,24 @@ use std::{borrow::Cow, str::FromStr};
|
||||
/// string-width-cjs: string-width@4.2.3
|
||||
/// ```
|
||||
///
|
||||
/// Detection mirrors pnpm's `refToRelative`: a reference is an alias when a
|
||||
/// package name appears before the version separator (either the first `@`
|
||||
/// occurs before any `(` and `:`, or the reference begins with `@`).
|
||||
/// * A `link:<path>` value — the dependency is a workspace sibling at
|
||||
/// `<path>` relative to the lockfile root. Pnpm writes this shape for
|
||||
/// injected workspace packages whose own dependencies resolve to other
|
||||
/// workspace projects (and for which `excludeLinksFromLockfile` is off);
|
||||
/// the dependency lives outside the virtual store and gets a direct
|
||||
/// directory symlink at install time.
|
||||
///
|
||||
/// ```yaml
|
||||
/// b@file:packages/b:
|
||||
/// dependencies:
|
||||
/// c: link:packages/c
|
||||
/// ```
|
||||
///
|
||||
/// Detection mirrors pnpm's `refToRelative`: a reference starting with
|
||||
/// `link:` short-circuits to the link variant (and `refToRelative` returns
|
||||
/// `null`); a reference is an alias when a package name appears before the
|
||||
/// version separator (either the first `@` occurs before any `(` and `:`,
|
||||
/// or the reference begins with `@`).
|
||||
///
|
||||
/// Reference: <https://github.com/pnpm/pnpm/blob/1819226b51/deps/path/src/index.ts>
|
||||
#[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize, Deserialize)]
|
||||
@@ -37,26 +52,46 @@ use std::{borrow::Cow, str::FromStr};
|
||||
pub enum SnapshotDepRef {
|
||||
Plain(PkgVerPeer),
|
||||
Alias(PkgNameVerPeer),
|
||||
Link(String),
|
||||
}
|
||||
|
||||
impl SnapshotDepRef {
|
||||
/// Resolve this reference to the `snapshots:` / `packages:` key it points
|
||||
/// to. `alias_name` is the key of the dependency entry (the name under
|
||||
/// which the package is linked into `node_modules`).
|
||||
pub fn resolve(&self, alias_name: &PkgName) -> PkgNameVerPeer {
|
||||
///
|
||||
/// Returns `None` for [`SnapshotDepRef::Link`] entries — `link:` deps
|
||||
/// don't live in the virtual store and have no snapshot key. Mirrors
|
||||
/// upstream's `refToRelative` returning `null` for `link:` references.
|
||||
pub fn resolve(&self, alias_name: &PkgName) -> Option<PkgNameVerPeer> {
|
||||
match self {
|
||||
SnapshotDepRef::Plain(ver_peer) => {
|
||||
PkgNameVerPeer::new(alias_name.clone(), ver_peer.clone())
|
||||
Some(PkgNameVerPeer::new(alias_name.clone(), ver_peer.clone()))
|
||||
}
|
||||
SnapshotDepRef::Alias(key) => key.clone(),
|
||||
SnapshotDepRef::Alias(key) => Some(key.clone()),
|
||||
SnapshotDepRef::Link(_) => None,
|
||||
}
|
||||
}
|
||||
|
||||
/// Accessor for the version-with-peer part of this reference.
|
||||
pub fn ver_peer(&self) -> &'_ PkgVerPeer {
|
||||
///
|
||||
/// Returns `None` for [`SnapshotDepRef::Link`] entries — a `link:` dep
|
||||
/// has no version slot.
|
||||
pub fn ver_peer(&self) -> Option<&'_ PkgVerPeer> {
|
||||
match self {
|
||||
SnapshotDepRef::Plain(ver_peer) => ver_peer,
|
||||
SnapshotDepRef::Alias(key) => &key.suffix,
|
||||
SnapshotDepRef::Plain(ver_peer) => Some(ver_peer),
|
||||
SnapshotDepRef::Alias(key) => Some(&key.suffix),
|
||||
SnapshotDepRef::Link(_) => None,
|
||||
}
|
||||
}
|
||||
|
||||
/// `Some(target)` when this reference is a `link:` workspace sibling;
|
||||
/// `None` otherwise. The returned string is the path portion *without*
|
||||
/// the `link:` prefix.
|
||||
pub fn as_link_target(&self) -> Option<&'_ str> {
|
||||
match self {
|
||||
SnapshotDepRef::Plain(_) | SnapshotDepRef::Alias(_) => None,
|
||||
SnapshotDepRef::Link(target) => Some(target.as_str()),
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -66,6 +101,7 @@ impl std::fmt::Display for SnapshotDepRef {
|
||||
match self {
|
||||
SnapshotDepRef::Plain(ver_peer) => ver_peer.fmt(f),
|
||||
SnapshotDepRef::Alias(key) => key.fmt(f),
|
||||
SnapshotDepRef::Link(target) => write!(f, "link:{target}"),
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -97,6 +133,9 @@ pub enum ParseSnapshotDepRefError {
|
||||
impl FromStr for SnapshotDepRef {
|
||||
type Err = ParseSnapshotDepRefError;
|
||||
fn from_str(value: &str) -> Result<Self, Self::Err> {
|
||||
if let Some(target) = value.strip_prefix("link:") {
|
||||
return Ok(SnapshotDepRef::Link(target.to_string()));
|
||||
}
|
||||
if looks_like_alias(value) {
|
||||
let key =
|
||||
value.parse::<PkgNameVerPeer>().map_err(ParseSnapshotDepRefError::ParseAlias)?;
|
||||
|
||||
@@ -47,17 +47,23 @@ fn parse_alias_with_peer_suffix() {
|
||||
#[test]
|
||||
fn resolve_plain_uses_alias_key_as_target_name() {
|
||||
let dep: SnapshotDepRef = "5.1.2".parse().unwrap();
|
||||
let resolved = dep.resolve(&pkg_name("string-width"));
|
||||
let resolved = dep.resolve(&pkg_name("string-width")).expect("plain resolves");
|
||||
assert_eq!(resolved.to_string(), "string-width@5.1.2");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn resolve_alias_uses_alias_target_name_not_key() {
|
||||
let dep: SnapshotDepRef = "string-width@4.2.3".parse().unwrap();
|
||||
let resolved = dep.resolve(&pkg_name("string-width-cjs"));
|
||||
let resolved = dep.resolve(&pkg_name("string-width-cjs")).expect("alias resolves");
|
||||
assert_eq!(resolved.to_string(), "string-width@4.2.3");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn resolve_link_returns_none() {
|
||||
let dep: SnapshotDepRef = "link:packages/c".parse().unwrap();
|
||||
assert_eq!(dep.resolve(&pkg_name("c")), None);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn display_roundtrip() {
|
||||
for input in [
|
||||
@@ -66,6 +72,8 @@ fn display_roundtrip() {
|
||||
"string-width@4.2.3",
|
||||
"@types/react@17.0.49",
|
||||
"react-dom@17.0.2(react@17.0.2)",
|
||||
"link:packages/c",
|
||||
"link:../sibling",
|
||||
] {
|
||||
let dep: SnapshotDepRef = input.parse().unwrap();
|
||||
assert_eq!(dep.to_string(), input);
|
||||
@@ -78,12 +86,20 @@ fn deserialize_ok() {
|
||||
("5.1.2", "5.1.2"),
|
||||
("string-width@4.2.3", "string-width@4.2.3"),
|
||||
(r#""17.0.2(react@17.0.2)""#, "17.0.2(react@17.0.2)"),
|
||||
("link:packages/c", "link:packages/c"),
|
||||
] {
|
||||
let dep: SnapshotDepRef = serde_saphyr::from_str(yaml).unwrap();
|
||||
assert_eq!(dep.to_string(), expected);
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn parse_link_workspace_path() {
|
||||
let dep: SnapshotDepRef = "link:packages/c".parse().unwrap();
|
||||
assert_eq!(dep, SnapshotDepRef::Link("packages/c".to_string()));
|
||||
assert_eq!(dep.as_link_target(), Some("packages/c"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn looks_like_alias_rules() {
|
||||
for (input, expected) in [
|
||||
@@ -104,14 +120,18 @@ fn looks_like_alias_rules() {
|
||||
/// `ver_peer` accesses the version-with-peer portion of either
|
||||
/// variant: the inner `PkgVerPeer` for `Plain`, and the alias's
|
||||
/// `suffix` for `Alias`. Used by snapshot lookups that only care
|
||||
/// about the version slot.
|
||||
/// about the version slot. Returns `None` for the `Link` variant —
|
||||
/// `link:` deps have no version slot.
|
||||
#[test]
|
||||
fn ver_peer_returns_inner_version_for_each_variant() {
|
||||
let plain: SnapshotDepRef = "17.0.2(react@17.0.2)".parse().unwrap();
|
||||
assert_eq!(plain.ver_peer().to_string(), "17.0.2(react@17.0.2)");
|
||||
assert_eq!(plain.ver_peer().map(ToString::to_string), Some("17.0.2(react@17.0.2)".to_string()));
|
||||
|
||||
let alias: SnapshotDepRef = "react-dom@17.0.2(react@17.0.2)".parse().unwrap();
|
||||
assert_eq!(alias.ver_peer().to_string(), "17.0.2(react@17.0.2)");
|
||||
assert_eq!(alias.ver_peer().map(ToString::to_string), Some("17.0.2(react@17.0.2)".to_string()));
|
||||
|
||||
let link: SnapshotDepRef = "link:packages/c".parse().unwrap();
|
||||
assert_eq!(link.ver_peer(), None);
|
||||
}
|
||||
|
||||
/// `From<PkgVerPeer>` is the infallible promotion path used by
|
||||
|
||||
@@ -102,7 +102,9 @@ fn build_children_map(
|
||||
[snap.dependencies.as_ref(), snap.optional_dependencies.as_ref()].into_iter().flatten()
|
||||
{
|
||||
for (alias, dep_ref) in deps {
|
||||
let resolved = dep_ref.resolve(alias);
|
||||
let Some(resolved) = dep_ref.resolve(alias) else {
|
||||
continue;
|
||||
};
|
||||
if snapshots.contains_key(&resolved) {
|
||||
child_keys.push(resolved);
|
||||
}
|
||||
|
||||
@@ -60,7 +60,12 @@ pub fn create_symlink_layout(
|
||||
if alias_name == self_name {
|
||||
return Ok(());
|
||||
}
|
||||
let target = dep_ref.resolve(alias_name);
|
||||
// `link:` deps point at a workspace sibling outside the
|
||||
// virtual store; the symlink-direct-dependencies stage
|
||||
// installs those for the importer, not here.
|
||||
let Some(target) = dep_ref.resolve(alias_name) else {
|
||||
return Ok(());
|
||||
};
|
||||
if skipped.contains(&target) {
|
||||
return Ok(());
|
||||
}
|
||||
|
||||
@@ -211,7 +211,10 @@ fn resolve_child(
|
||||
snapshots: &HashMap<PackageKey, SnapshotEntry>,
|
||||
skipped: &SkippedSnapshots,
|
||||
) -> Option<PackageKey> {
|
||||
let resolved = dep_ref.resolve(alias);
|
||||
// `link:` deps live outside the virtual store and have no
|
||||
// snapshot to reach — they aren't part of the reachable-snapshot
|
||||
// graph this helper computes.
|
||||
let resolved = dep_ref.resolve(alias)?;
|
||||
if skipped.contains(&resolved) {
|
||||
return None;
|
||||
}
|
||||
|
||||
@@ -150,8 +150,12 @@ fn build_children(snapshot: &SnapshotEntry) -> HashMap<String, PackageKey> {
|
||||
for (alias, dep_ref) in dep_entries {
|
||||
// `SnapshotDepRef::resolve` returns the `PkgNameVerPeer`
|
||||
// (= `PackageKey`) the alias points at in the `snapshots:`
|
||||
// map.
|
||||
let resolved: PackageKey = dep_ref.resolve(alias);
|
||||
// map. `link:` deps don't have a snapshot key — skip them,
|
||||
// mirroring upstream's `if (childDepPath)` guard in
|
||||
// `lockfileDepsToGraphChildren`.
|
||||
let Some(resolved) = dep_ref.resolve(alias) else {
|
||||
continue;
|
||||
};
|
||||
children.insert(alias.to_string(), resolved);
|
||||
}
|
||||
children
|
||||
|
||||
@@ -86,7 +86,11 @@ pub fn build_hoist_graph(
|
||||
.flat_map(|m| m.iter())
|
||||
.chain(snapshot.optional_dependencies.iter().flat_map(|m| m.iter()));
|
||||
let children: HashMap<String, PackageKey> = dep_entries
|
||||
.map(|(alias, dep_ref)| (alias.to_string(), dep_ref.resolve(alias)))
|
||||
// `dep_ref.resolve` is `None` for `link:` deps —
|
||||
// workspace siblings that live outside the virtual
|
||||
// store. Mirrors upstream's `if (childDepPath)`
|
||||
// check in `getChildren`.
|
||||
.filter_map(|(alias, dep_ref)| Some((alias.to_string(), dep_ref.resolve(alias)?)))
|
||||
.collect();
|
||||
Some((
|
||||
key.clone(),
|
||||
|
||||
@@ -816,7 +816,13 @@ fn compute_children(
|
||||
.flatten()
|
||||
.chain(snapshot.optional_dependencies.iter().flatten());
|
||||
for (alias_name, dep_ref) in dep_iter {
|
||||
let child_key = dep_ref.resolve(alias_name);
|
||||
// `link:` deps return `None` here — they live outside the
|
||||
// virtual store and don't show up in `pkg_locations`.
|
||||
// Mirrors upstream's `if (childDepPath && pkgLocations...)`
|
||||
// guard in `getChildren`.
|
||||
let Some(child_key) = dep_ref.resolve(alias_name) else {
|
||||
continue;
|
||||
};
|
||||
let child_dep_path = child_key.to_string();
|
||||
if let Some(locations) = pkg_locations.get(&child_dep_path)
|
||||
&& let Some(first) = locations.first()
|
||||
|
||||
@@ -1334,7 +1334,11 @@ pub(crate) fn find_own_runtime_node_major(snapshot: &SnapshotEntry) -> Option<u3
|
||||
if alias.scope.is_some() || alias.bare != "node" {
|
||||
continue;
|
||||
}
|
||||
let ver_peer = dep_ref.ver_peer();
|
||||
// `link:` deps have no version slot and can't carry a
|
||||
// `runtime:` pin — skip them.
|
||||
let Some(ver_peer) = dep_ref.ver_peer() else {
|
||||
continue;
|
||||
};
|
||||
if ver_peer.prefix() != Prefix::Runtime {
|
||||
continue;
|
||||
}
|
||||
|
||||
@@ -419,7 +419,11 @@ where
|
||||
// the self-shim. Mirror it here.
|
||||
let with_bin: Vec<(&PkgName, PackageKey)> = children
|
||||
.filter_map(|(alias, dep_ref)| {
|
||||
let child_key = dep_ref.resolve(alias);
|
||||
// `link:` deps live outside the virtual store and
|
||||
// expose their bins via the workspace project's
|
||||
// own `package.json`, not through a snapshot — skip
|
||||
// them here.
|
||||
let child_key = dep_ref.resolve(alias)?;
|
||||
let metadata_key = child_key.without_peer();
|
||||
let keep = match has_bin_set {
|
||||
Some(set) => set.contains(&metadata_key),
|
||||
|
||||
@@ -342,7 +342,10 @@ fn merge_into_children(
|
||||
deps: &HashMap<PkgName, SnapshotDepRef>,
|
||||
) {
|
||||
for (alias, dep_ref) in deps {
|
||||
let resolved = dep_ref.resolve(alias);
|
||||
// `link:` deps have no snapshot key — skip them.
|
||||
let Some(resolved) = dep_ref.resolve(alias) else {
|
||||
continue;
|
||||
};
|
||||
children.insert(alias.to_string(), resolved);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -501,7 +501,14 @@ fn collect_snapshot_deps(
|
||||
// snapshot lookup hits the right entry. The node's exposed
|
||||
// `name` stays `alias`; only the lookup uses the resolved
|
||||
// target name.
|
||||
let dep_key = dep_ref.resolve(alias);
|
||||
//
|
||||
// `link:` deps return `None` — they have no snapshot to
|
||||
// hoist (the install layer materialises them as direct
|
||||
// directory symlinks), so we skip them here, mirroring
|
||||
// upstream's `if (childDepPath)` check in `getChildren`.
|
||||
let Some(dep_key) = dep_ref.resolve(alias) else {
|
||||
continue;
|
||||
};
|
||||
let node = build_dep_node(alias, &dep_key, lockfile, opts, nodes)?;
|
||||
out.insert(RcByPtr(node));
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user