Skip to content

fix(pnpm): write pnpmOverrides to pnpm-workspace.yaml #336

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
unstubbable wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(pnpm): write pnpmOverrides to pnpm-workspace.yaml #336

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 40 additions & 0 deletions src/commands/fix_test.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,46 @@ fn pnpm_fix_not_using_catalog() {
);
}

#[test]
fn pnpm_fix_updates_overrides_in_yaml() {
// pnpm-workspace.yaml `overrides` pins react below the version group's
// pinVersion. Fix must rewrite the override in the yaml — not just the
// package.json consumers. Regression test for `copy_expected_specifier_yaml`
// silently no-opping on non-catalog (versionsByName) PnpmWorkspace instances.
let yaml = "overrides:\n react: 19.2.6\n";
let ctx = TestBuilder::new()
.with_pnpm_catalogs(yaml)
.with_packages(vec![json!({
"name": "pkg-a",
"version": "0.0.0",
"dependencies": {"react": "19.2.6"},
})])
.with_version_group(json!({
"label": "pin react",
"dependencies": ["react"],
"pinVersion": "19.2.7",
}))
.build_and_visit_packages();
let ctx = run_fix_ok(ctx);

let yaml = pnpm_yaml(&ctx).unwrap();
let got = yaml
.contents
.get("overrides")
.and_then(|m| m.get("react"))
.and_then(|v| v.as_str())
.map(str::to_string);
assert_eq!(got, Some("19.2.7".to_string()), "override should be bumped to the pinned version");
assert!(yaml.is_dirty(), "yaml should be dirty after override rewrite");

let pkg = find_package(&ctx, "pkg-a");
assert_eq!(
pkg.contents.pointer("/dependencies/react").and_then(|v| v.as_str()),
Some("19.2.7"),
"consumer react should be pinned too"
);
}

#[test]
fn pnpm_fix_missing_from_catalog_inserts_definition() {
// Catalog exists but lacks `react`. Consumer has real specifier. Fix
Expand Down
244 changes: 150 additions & 94 deletions src/disk.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -737,77 +737,90 @@ pub fn insert_catalog_definition(file: &mut YamlFile, catalog_name: &str, dep_na
true
}

/// Determine the right `PendingYamlOp` for inserting `dep_name → next_value`
/// into a flat top-level block named `block_key` (e.g. `catalog`,
/// `overrides`), based on the pre-mutation state of `file.contents`. Returns
/// `None` when the existing value already equals `next_value`.
fn build_block_insert_patch(file: &YamlFile, block_key: &str, dep_name: &str, next_value: &YamlValue) -> Option<PendingYamlOp> {
let root = match &file.contents {
YamlValue::Mapping(map) => Some(map),
_ => None,
};
let existing_block = root.and_then(|r| r.get(block_key)).and_then(|v| v.as_mapping());
let existing_value = existing_block.and_then(|b| b.get(dep_name));
if existing_value == Some(next_value) {
return None;
}
if existing_value.is_some() {
return Some(PendingYamlOp::Replace {
segments: vec![block_key.to_string(), dep_name.to_string()],
value: next_value.clone(),
});
}
if existing_block.is_some() {
return Some(PendingYamlOp::Add {
segments: vec![block_key.to_string()],
key: dep_name.to_string(),
value: next_value.clone(),
});
}
let mut nested = Mapping::new();
nested.insert(YamlValue::String(dep_name.to_string()), next_value.clone());
Some(PendingYamlOp::Add {
segments: Vec::new(),
key: block_key.to_string(),
value: YamlValue::Mapping(nested),
})
}

/// Determine the right `PendingYamlOp` for an insert based on the
/// pre-mutation state of `file.contents`. Returns `None` when the
/// existing value already equals `next_value` (idempotent case).
fn build_insert_patch(file: &YamlFile, catalog_name: &str, dep_name: &str, next_value: &YamlValue) -> Option<PendingYamlOp> {
// The default catalog is a flat top-level `catalog:` block, identical in
// shape to `overrides:`; both go through `build_block_insert_patch`.
if catalog_name == "default" {
return build_block_insert_patch(file, "catalog", dep_name, next_value);
}
let root = match &file.contents {
YamlValue::Mapping(map) => Some(map),
_ => None,
};
if catalog_name == "default" {
let existing_block = root.and_then(|r| r.get("catalog")).and_then(|v| v.as_mapping());
let existing_value = existing_block.and_then(|b| b.get(dep_name));
if existing_value == Some(next_value) {
return None;
}
if existing_value.is_some() {
return Some(PendingYamlOp::Replace {
segments: vec!["catalog".to_string(), dep_name.to_string()],
value: next_value.clone(),
});
}
if existing_block.is_some() {
return Some(PendingYamlOp::Add {
segments: vec!["catalog".to_string()],
key: dep_name.to_string(),
value: next_value.clone(),
});
}
let mut nested = Mapping::new();
nested.insert(YamlValue::String(dep_name.to_string()), next_value.clone());
Some(PendingYamlOp::Add {
segments: Vec::new(),
key: "catalog".to_string(),
value: YamlValue::Mapping(nested),
})
} else {
let catalogs = root.and_then(|r| r.get("catalogs")).and_then(|v| v.as_mapping());
let existing_named = catalogs.and_then(|c| c.get(catalog_name)).and_then(|v| v.as_mapping());
let existing_value = existing_named.and_then(|n| n.get(dep_name));
if existing_value == Some(next_value) {
return None;
}
if existing_value.is_some() {
return Some(PendingYamlOp::Replace {
segments: vec!["catalogs".to_string(), catalog_name.to_string(), dep_name.to_string()],
value: next_value.clone(),
});
}
if existing_named.is_some() {
return Some(PendingYamlOp::Add {
segments: vec!["catalogs".to_string(), catalog_name.to_string()],
key: dep_name.to_string(),
value: next_value.clone(),
});
}
let mut named_map = Mapping::new();
named_map.insert(YamlValue::String(dep_name.to_string()), next_value.clone());
if catalogs.is_some() {
return Some(PendingYamlOp::Add {
segments: vec!["catalogs".to_string()],
key: catalog_name.to_string(),
value: YamlValue::Mapping(named_map),
});
}
let mut catalogs_map = Mapping::new();
catalogs_map.insert(YamlValue::String(catalog_name.to_string()), YamlValue::Mapping(named_map));
Some(PendingYamlOp::Add {
segments: Vec::new(),
key: "catalogs".to_string(),
value: YamlValue::Mapping(catalogs_map),
})
let catalogs = root.and_then(|r| r.get("catalogs")).and_then(|v| v.as_mapping());
let existing_named = catalogs.and_then(|c| c.get(catalog_name)).and_then(|v| v.as_mapping());
let existing_value = existing_named.and_then(|n| n.get(dep_name));
if existing_value == Some(next_value) {
return None;
}
if existing_value.is_some() {
return Some(PendingYamlOp::Replace {
segments: vec!["catalogs".to_string(), catalog_name.to_string(), dep_name.to_string()],
value: next_value.clone(),
});
}
if existing_named.is_some() {
return Some(PendingYamlOp::Add {
segments: vec!["catalogs".to_string(), catalog_name.to_string()],
key: dep_name.to_string(),
value: next_value.clone(),
});
}
let mut named_map = Mapping::new();
named_map.insert(YamlValue::String(dep_name.to_string()), next_value.clone());
if catalogs.is_some() {
return Some(PendingYamlOp::Add {
segments: vec!["catalogs".to_string()],
key: catalog_name.to_string(),
value: YamlValue::Mapping(named_map),
});
}
let mut catalogs_map = Mapping::new();
catalogs_map.insert(YamlValue::String(catalog_name.to_string()), YamlValue::Mapping(named_map));
Some(PendingYamlOp::Add {
segments: Vec::new(),
key: "catalogs".to_string(),
value: YamlValue::Mapping(catalogs_map),
})
}

/// Remove `dep_name` from the named catalog block. If removal empties the
Expand Down Expand Up @@ -872,18 +885,27 @@ pub fn remove_catalog_definition(file: &mut YamlFile, catalog_name: &str, dep_na
removed
}

/// Apply a consumer instance's `expected_specifier` to the yaml via the
/// catalog-definition route. No-op when the instance is not a catalog
/// instance or has no expected specifier.
/// Apply a consumer instance's `expected_specifier` to the yaml. Catalog
/// instances route through the catalog-definition path; non-catalog
/// `PnpmWorkspace` instances (pnpm `overrides`, which use the
/// `versionsByName` strategy) write into the top-level block named by the
/// dep type's `path` (e.g. `overrides`). No-op when the instance has no
/// expected specifier.
pub fn copy_expected_specifier_yaml(file: &mut YamlFile, instance: &Instance) {
let Some(catalog_name) = instance.catalog_name() else {
return;
};
let Some(expected) = instance.expected_specifier.borrow().clone() else {
return;
};
let dep_name = instance.descriptor.name.clone();
insert_catalog_definition(file, catalog_name, &dep_name, &expected);
if let Some(catalog_name) = instance.catalog_name() {
insert_catalog_definition(file, catalog_name, &dep_name, &expected);
return;
}
// `overrides` is a flat `name: version` block — the same shape as the
// default `catalog:` block. `path` is its JSON pointer, e.g. `/overrides`.
if matches!(instance.descriptor.dependency_type.strategy, Strategy::VersionsByName) {
let block_key = instance.descriptor.dependency_type.path.trim_start_matches('/');
insert_into_yaml_block(file, block_key, &dep_name, &expected);
}
}

/// Persist a JSON file when dirty. Returns `Ok(true)` on actual write,
Expand Down Expand Up @@ -966,42 +988,76 @@ fn json_values_differ(a: &JsonValue, b: &JsonValue) -> bool {
}
}

/// Get-or-create a flat top-level block named `key` as a mapping, promoting
/// the document root if it was not already a mapping. Shared by the default
/// `catalog:` block and pnpm `overrides:`.
fn ensure_yaml_block<'a>(file: &'a mut YamlFile, key: &str) -> &'a mut Mapping {
if !matches!(file.contents, YamlValue::Mapping(_)) {
file.contents = YamlValue::Mapping(Mapping::new());
}
let YamlValue::Mapping(ref mut root) = file.contents else {
unreachable!("just promoted to Mapping");
};
let block_key = YamlValue::String(key.to_string());
if !matches!(root.get(&block_key), Some(YamlValue::Mapping(_))) {
root.insert(block_key.clone(), YamlValue::Mapping(Mapping::new()));
}
let YamlValue::Mapping(block) = root.get_mut(&block_key).expect("just inserted") else {
unreachable!();
};
block
}

/// Get-or-create the catalog block (top-level `catalog` for default, or a
/// nested entry under `catalogs.{name}` for named catalogs) as a mapping.
fn ensure_catalog_block<'a>(file: &'a mut YamlFile, catalog_name: &str) -> &'a mut Mapping {
if catalog_name == "default" {
return ensure_yaml_block(file, "catalog");
}
// Promote root to a Mapping if it was Null (freshly created file).
if !matches!(file.contents, YamlValue::Mapping(_)) {
file.contents = YamlValue::Mapping(Mapping::new());
}
let YamlValue::Mapping(ref mut root) = file.contents else {
unreachable!("just promoted to Mapping");
};
if catalog_name == "default" {
let key = YamlValue::String("catalog".to_string());
if !matches!(root.get(&key), Some(YamlValue::Mapping(_))) {
root.insert(key.clone(), YamlValue::Mapping(Mapping::new()));
}
let YamlValue::Mapping(block) = root.get_mut(&key).expect("just inserted") else {
unreachable!();
};
block
} else {
let catalogs_key = YamlValue::String("catalogs".to_string());
if !matches!(root.get(&catalogs_key), Some(YamlValue::Mapping(_))) {
root.insert(catalogs_key.clone(), YamlValue::Mapping(Mapping::new()));
}
let YamlValue::Mapping(catalogs) = root.get_mut(&catalogs_key).expect("just inserted") else {
unreachable!();
};
let name_key = YamlValue::String(catalog_name.to_string());
if !matches!(catalogs.get(&name_key), Some(YamlValue::Mapping(_))) {
catalogs.insert(name_key.clone(), YamlValue::Mapping(Mapping::new()));
}
let YamlValue::Mapping(block) = catalogs.get_mut(&name_key).expect("just inserted") else {
unreachable!();
};
block
let catalogs_key = YamlValue::String("catalogs".to_string());
if !matches!(root.get(&catalogs_key), Some(YamlValue::Mapping(_))) {
root.insert(catalogs_key.clone(), YamlValue::Mapping(Mapping::new()));
}
let YamlValue::Mapping(catalogs) = root.get_mut(&catalogs_key).expect("just inserted") else {
unreachable!();
};
let name_key = YamlValue::String(catalog_name.to_string());
if !matches!(catalogs.get(&name_key), Some(YamlValue::Mapping(_))) {
catalogs.insert(name_key.clone(), YamlValue::Mapping(Mapping::new()));
}
let YamlValue::Mapping(block) = catalogs.get_mut(&name_key).expect("just inserted") else {
unreachable!();
};
block
}

/// Insert `dep_name → specifier` into a flat top-level block named `block_key`
/// (e.g. `overrides`), creating the block when absent. Idempotent — returns
/// `true` only when in-memory state changed — and records a `PendingYamlOp` so
/// the write path replays the edit via `yamlpatch` and preserves formatting.
/// The single-block sibling of `insert_catalog_definition`.
fn insert_into_yaml_block(file: &mut YamlFile, block_key: &str, dep_name: &str, specifier: &Rc<Specifier>) -> bool {
let next_value = YamlValue::String(specifier.get_raw().to_string());
// Choose patch shape from PRE-mutation state; `ensure_yaml_block` mutates
// `file.contents` below.
let pending = build_block_insert_patch(file, block_key, dep_name, &next_value);
let block = ensure_yaml_block(file, block_key);
if block.get(dep_name) == Some(&next_value) {
return false;
}
block.insert(YamlValue::String(dep_name.to_string()), next_value);
if let Some(op) = pending {
file.patches.push(op);
}
file.dirty = true;
true
}

/// Convert a `yaml_serde::Value` into a `serde_json::Value` for JSON pointer
Expand Down