Skip to content

Accept duplicate package manifest fields #718

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
colinhacks wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Accept duplicate package manifest fields #718

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
45 changes: 44 additions & 1 deletion vendor/aube/crates/aube-manifest/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -388,7 +388,22 @@ impl PackageJson {
/// [`Error::Parse`] with the source content and a span so `miette`'s
/// `fancy` handler renders a pointer at the offending byte.
pub fn parse(path: &Path, content: String) -> Result<Self, Error> {
parse_json(path, content)
match parse_json(path, content.clone()) {

@pullfrog pullfrog Bot Aug 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This clones the entire manifest string on every parse, including the overwhelmingly common success path, to serve a fallback that essentially never fires. parse_json can only return Error::Parse(Box<ParseError>), and ParseError.src is a miette::NamedSource<String> that already owns the content — BOM-stripped, which would also make the second strip on line 398 unnecessary.

Technical details 
# Avoid the per-manifest copy on the success path

## Affected sites
- `vendor/aube/crates/aube-manifest/src/lib.rs:391` — `content.clone()` runs for every manifest
  `PackageJson::parse` sees, which during an install is once per dependency with lifecycle work plus every
  workspace member.
- `vendor/aube/crates/aube-manifest/src/lib.rs:398` — the BOM strip becomes redundant if the retry reads the
  content back out of the error, since `parse_json` strips the BOM before constructing it (line 1376).

## Required outcome
- No allocation or copy of `content` on the path where `parse_json` succeeds.

## Suggested approach (optional)
`miette::NamedSource::inner()` exists in miette 7.6 and returns `&String`, so the retry can borrow rather than
copy. Sketch — not compiled by me, so treat it as a shape rather than a patch:

```rust
let original = match parse_json(path, content) {
    Ok(manifest) => return Ok(manifest),
    Err(original) => original,
};
// ...existing comment...
if let Error::Parse(pe) = &original
    && let Ok(value) = serde_json::from_str::<serde_json::Value>(pe.src.inner())
    && let Ok(manifest) = serde_json::from_value(value)
{
    return Ok(manifest);
}
Err(original)
```

Ok(manifest) => Ok(manifest),
Err(original) => {
// `JSON.parse`, npm and pnpm accept duplicate object keys and keep the last value.
// Serde's struct deserializer rejects duplicate known fields before it can apply
// that de-facto package.json behavior. Preserve the fast typed parse above for the
// common case; only normalize through a generic JSON object after it fails.
let normalized = content.strip_prefix('\u{FEFF}').unwrap_or(&content);
if let Ok(value) = serde_json::from_str::<serde_json::Value>(normalized)
&& let Ok(manifest) = serde_json::from_value(value)
{
return Ok(manifest);
}
Err(original)
}
}
}

/// True when `peerDependenciesMeta.<name>.optional` is set.
Expand Down Expand Up @@ -1458,6 +1473,34 @@ mod tests {
serde_json::from_str(json).unwrap()
}

#[test]
fn package_json_duplicate_fields_keep_the_last_value() {
let manifest = PackageJson::parse(
Path::new("package.json"),
r#"{
"name": "first",
"scripts": {"install": "old"},
"dependencies": {"left-pad": "1.1.0"},
"name": "last",
"scripts": {"postinstall": "new"},
"dependencies": {"left-pad": "1.3.0"}
}"#
.to_string(),
)
.unwrap();

assert_eq!(manifest.name.as_deref(), Some("last"));
assert_eq!(manifest.scripts.len(), 1);
assert_eq!(
manifest.scripts.get("postinstall").map(String::as_str),
Some("new")
);
assert_eq!(
manifest.dependencies.get("left-pad").map(String::as_str),
Some("1.3.0")
);
}

/// `npm_package_env` mirrors pnpm's exact flattening: name, version,
/// and deep `engines`/`config`/`bin` — and nothing else.
#[test]
Expand Down
Loading