Skip to content

fix: keep // comment keys with the field below them in sorted package.json sections #74

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

Merged
dsherret merged 1 commit into from
Oct 1, 2026
Merged

fix: keep // comment keys with the field below them in sorted package.json sections #74

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
104 changes: 59 additions & 45 deletions src/package_json.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,12 @@ pub fn is_package_json_file(path: &Path) -> bool {
///
/// The top level is written in the conventional field order, which is the one used by
/// [`sort-package-json`](https://github.com/keithamus/sort-package-json), and the maps whose keys
/// are package names or similar are written alphabetically, one level deep. A top level key
/// starting with `//`, which stands in for a comment, stays above the field it was written over. A
/// dependency section written in runs with a comment heading each is alphabetized a run at a time,
/// with each heading left over its run. Everything else is left alone: the order of `exports`
/// conditions and of `files`, `workspaces` or `scripts` entries is the author's to decide, and so is
/// the order of anything a package manager reads as a list of rules rather than as a map.
/// are package names or similar are written alphabetically, one level deep. A key starting with
/// `//`, which stands in for a comment, stays above the field it was written over. A dependency
/// section written in runs with a comment heading each is alphabetized a run at a time, with each
/// heading left over its run. Everything else is left alone: the order of `exports` conditions and
/// of `files`, `workspaces` or `scripts` entries is the author's to decide, and so is the order of
/// anything a package manager reads as a list of rules rather than as a map.
///
/// The work happens on the CST so that what was written with a property travels with it. Text that
/// doesn't parse is handed back untouched, since the formatter is about to report that itself.
Expand All @@ -41,7 +41,7 @@ pub fn apply_conventions<'a>(text: &'a str, config: &Configuration) -> Cow<'a, s
// A comment written above a top level property travels with it. A conventional order rearranges
// the whole file, so a comment left behind would end up over a property it says nothing about,
// and one written above `dependencies` is almost always about those.
sort_top_level_fields(&root_object);
sort_fields(&root_object, None, top_level_field_key);
for prop in root_object.properties() {
let Some(section) = alphabetical_section(&decoded_name(&prop)) else {
continue;
Expand All @@ -50,7 +50,7 @@ pub fn apply_conventions<'a>(text: &'a str, config: &Configuration) -> Cow<'a, s
continue;
};
match section {
Section::Plain => object.sort_properties().by_key(NpmName::of),
Section::Plain => sort_fields(&object, None, NpmName::of),
Section::Dependencies => sort_dependencies(&object, config),
Section::Overrides if !names_a_package_twice(&object) => sort_dependencies(&object, config),
Section::Overrides => {}
Expand All @@ -66,37 +66,6 @@ pub fn apply_conventions<'a>(text: &'a str, config: &Configuration) -> Cow<'a, s
}
}

/// Sorts the top level fields into the conventional order.
///
/// `package.json` has no comments, so a key starting with `//` is commonly written above a field to
/// say something about it. Such a key is not a field of its own: it travels with the field written
/// below it, the same way a comment does, and one with no field below it stays at the end.
fn sort_top_level_fields(root_object: &CstObject) {
// walking upwards so that each comment key finds the field below it
let mut field = None;
let mut fields = HashMap::new();
for prop in root_object.properties().into_iter().rev() {
let index = prop.child_index();
if !is_comment_key(&decoded_name(&prop)) {
field = Some(prop);
}
fields.insert(index, field.clone());
}

root_object.sort_properties().by_key(|prop| {
// the index keeps a field's comment keys above it, in the order they were written
let index = prop.child_index();
let field = fields[&index].as_ref().map(top_level_field_key);
(field.is_none(), field, index)
});
}

/// Whether a key is the `package.json` stand-in for a comment (`"//"`, `"// node"`, ...), which npm
/// reserves for that.
fn is_comment_key(name: &str) -> bool {
name.starts_with("//")
}

/// Sorts a dependency section a run at a time.
///
/// A long list of dependencies is often written as runs with a comment heading each, and a
Expand All @@ -115,7 +84,7 @@ fn is_comment_key(name: &str) -> bool {
/// as a whole.
fn sort_dependencies(section: &CstObject, config: &Configuration) {
if config.object_prefer_single_line || !opens_on_its_own_line(section) {
section.sort_properties().by_key(NpmName::of);
sort_fields(section, None, NpmName::of);
return;
}

Expand All @@ -130,10 +99,52 @@ fn sort_dependencies(section: &CstObject, config: &Configuration) {
(prop.child_index(), run)
})
.collect::<HashMap<_, _>>();
section
.sort_properties()
.pin_comment_headers()
.by_key(|prop| (runs[&prop.child_index()], NpmName::of(prop)));
sort_fields(section, Some(runs), NpmName::of);
}

/// Sorts an object's fields by a key, with each comment key kept above the field it was written
/// over.
///
/// `package.json` has no comments, so a key starting with `//` is commonly written above a field to
/// say something about it. Such a key is not a field of its own: it travels with the field written
/// below it, the same way a comment does, and one with no field below it stays at the end.
///
/// Given the run each property was written in, by child index, the fields are sorted a run at a
/// time with each run's heading left over it. A comment key is then only about a field in its own
/// run, and one with no such field below it stays at the end of its run rather than follow a field
/// under the next heading.
fn sort_fields<K: Ord>(object: &CstObject, runs: Option<HashMap<usize, usize>>, key: impl Fn(&CstObjectProp) -> K) {
let run = |prop: &CstObjectProp| runs.as_ref().map_or(0, |runs| runs[&prop.child_index()]);

// walking upwards so that each comment key finds the field below it
let mut field: Option<CstObjectProp> = None;
let mut fields = HashMap::new();
for prop in object.properties().into_iter().rev() {
let index = prop.child_index();
if !is_comment_key(&decoded_name(&prop)) {
field = Some(prop);
} else if field.as_ref().is_some_and(|field| run(field) != run(&prop)) {
field = None;
}
fields.insert(index, field.clone());
}

let mut sort = object.sort_properties();
if runs.is_some() {
sort = sort.pin_comment_headers();
}
sort.by_key(|prop| {
// the index keeps a field's comment keys above it, in the order they were written
let index = prop.child_index();
let field = fields[&index].as_ref().map(&key);
(run(prop), field.is_none(), field, index)
});
}

/// Whether a key is the `package.json` stand-in for a comment (`"//"`, `"// node"`, ...), which npm
/// reserves for that.
fn is_comment_key(name: &str) -> bool {
name.starts_with("//")
}

/// Whether the section's first property is written on a line after its open brace, which is what
Expand Down Expand Up @@ -167,7 +178,10 @@ fn names_a_package_twice(overrides: &CstObject) -> bool {
overrides
.properties()
.iter()
.any(|prop| !seen.insert(package_name(&decoded_name(prop)).to_string()))
.map(decoded_name)
// a comment key names no package, however many times it is written
.filter(|name| !is_comment_key(name))
.any(|name| !seen.insert(package_name(&name).to_string()))
}

/// The package a key names without any version written after it, so that `foo@^2` is `foo` and
Expand Down
181 changes: 181 additions & 0 deletions tests/specs/package_json/PackageJson_Conventions.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -417,3 +417,184 @@
"// zzz": "about zzz",
"zzz": true
}

== should move a comment key with the field written below it inside a sorted section ==
{
"name": "my-package",
"bin": {
"zzz": "./zzz.js",
"// aaa": "kept for the old name",
"aaa": "./aaa.js"
},
"dependencies": {
"// zod": "pinned until the v4 migration",
"zod": "3.0.0",
"//": "about chalk",
"//": "and more about chalk",
"chalk": "^5.0.0",
"// nothing": "with no dependency below it"
},
"engines": {
"npm": ">=10",
"// node": "Support import.meta.dirname and fetch",
"node": ">=22.16.0"
}
}

[expect]
{
"name": "my-package",
"bin": {
"// aaa": "kept for the old name",
"aaa": "./aaa.js",
"zzz": "./zzz.js"
},
"dependencies": {
"//": "about chalk",
"//": "and more about chalk",
"chalk": "^5.0.0",
"// zod": "pinned until the v4 migration",
"zod": "3.0.0",
"// nothing": "with no dependency below it"
},
"engines": {
"// node": "Support import.meta.dirname and fetch",
"node": ">=22.16.0",
"npm": ">=10"
}
}

== should keep a comment key in the run of the dependency written below it ==
{
"name": "my-package",
"dependencies": {
"zod": "^3.0.0",
"// chalk": "for the cli",
"chalk": "^5.0.0",

// the ones we patched
"// semver": "see patches/semver.patch",
"semver": "^7.0.0",
"glob": "^10.0.0"
}
}

[expect]
{
"name": "my-package",
"dependencies": {
"// chalk": "for the cli",
"chalk": "^5.0.0",
"zod": "^3.0.0",

// the ones we patched
"glob": "^10.0.0",
"// semver": "see patches/semver.patch",
"semver": "^7.0.0"
}
}

== should sort overrides that only repeat a comment key ==
{
"name": "my-package",
"overrides": {
"//": "security fix",
"koa": "^3.0.0",
"//": "dedupe",
"axios": "^1.15.0"
}
}

[expect]
{
"name": "my-package",
"overrides": {
"//": "dedupe",
"axios": "^1.15.0",
"//": "security fix",
"koa": "^3.0.0"
}
}

== should leave a comment key at the end of its run when the next dependency is under another heading ==
{
"name": "my-package",
"dependencies": {
"zod": "^3.0.0",
"ajv": "^8.0.0",
"// trailing": "about this run",

// the ones we patched
"semver": "^7.0.0",
"glob": "^10.0.0",

// the rest
"// note": "nothing here yet",

// tooling
"tsx": "^4.0.0",
"chalk": "^5.0.0",
"//": "last"
}
}

[expect]
{
"name": "my-package",
"dependencies": {
"ajv": "^8.0.0",
"zod": "^3.0.0",
"// trailing": "about this run",

// the ones we patched
"glob": "^10.0.0",
"semver": "^7.0.0",

// the rest
"// note": "nothing here yet",

// tooling
"chalk": "^5.0.0",
"tsx": "^4.0.0",
"//": "last"
}
}

== should move a comment key with its dependency in a section opened on the brace line ==
{
"name": "my-package",
"dependencies": { "zod": "^3.0.0",
"// chalk": "for the cli",
"chalk": "^5.0.0"
}
}

[expect]
{
"name": "my-package",
"dependencies": { "// chalk": "for the cli", "chalk": "^5.0.0", "zod": "^3.0.0" }
}

== should leave overrides that name a package twice alone, comment keys or not ==
{
"name": "my-package",
"overrides": {
"//": "the order of these two matters",
"foo@2": "2.9.9",
"//": "fallback",
"foo@1 || 2": "1.0.0",
"bar": "1.0.0"
}
}

[expect]
{
"name": "my-package",
"overrides": {
"//": "the order of these two matters",
"foo@2": "2.9.9",
"//": "fallback",
"foo@1 || 2": "1.0.0",
"bar": "1.0.0"
}
}
Loading