Skip to content

fix(manifest): preserve package.json indent, line endings, and EOF newline - #583

Open
afonsojramos wants to merge 2 commits into
nubjs:mainfrom
afonsojramos:fix/manifest-style-preservation
Open

fix(manifest): preserve package.json indent, line endings, and EOF newline#583
afonsojramos wants to merge 2 commits into
nubjs:mainfrom
afonsojramos:fix/manifest-style-preservation

Conversation

@afonsojramos

Copy link
Copy Markdown
Contributor

Problem

Any package.json rewrite through the aube command layer — update/add/audit dep-section syncs and the namespace settings editors — reformats the whole file: a tab- or 4-space-indented manifest is reindented to two spaces, CRLF becomes LF, and a missing EOF newline gets appended. A one-key change diffs as the entire file:

$ cat package.json   # tab-indented
{
	"name": "x",
	"devDependencies": { … }
}
$ nub update         # bumps one specifier…
$ git diff --stat package.json
 1 file changed, 44 insertions(+), 44 deletions(-)   # …and reindents everything

npm and pnpm both detect and reproduce the file's own style (detect-indent in @npmcli/package-json / @pnpm/read-project-manifest). nub-core's pin writer (edit_manifest in pm/resolve.rs) already does this for the devEngines stamp — the command-layer writers just never got the same treatment.

Root cause

Six writers serialize with serde_json::to_string_pretty (hardwired two-space indent, LF) and append \n unconditionally:

  • aube/src/commands/manifest_io.rs: write_manifest_json, update_manifest_json_object
  • aube-manifest/src/workspace/edits.rs: remove_setting_entry, edit_setting_map, add_to_pnpm_only_built_dependencies, set_pnpm_allow_builds_entries

Fix

Add detect_json_style / serialize_json_with_style to aube-manifest (first indented line's whitespace run as the indent unit, CRLF reproduction, trailing-newline state — the same rules as nub-core's detect_format) and route all six writers through them. Files with no indentation to imitate keep the previous two-space/LF/trailing-newline output, so freshly created manifests are byte-identical to before.

Tests

  • Style helpers: tab/4-space/CRLF/EOF-newline detection, style-faithful serialization, and a byte-for-byte round-trip of a tabbed document.
  • write_manifest_dep_sections preserves tab indentation (the nub update repro) and never two-space-reindents.
  • write_manifest_json preserves CRLF + absent trailing newline.
  • edit_setting_map preserves tab indentation through a settings write.

Verified end-to-end: nub update on a tab-indented manifest bumps the one specifier and leaves every other byte alone (including the devEngines stamp interplay).

@vercel

vercel Bot commented Jul 28, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
nub Ready Ready Preview Jul 29, 2026 11:27pm

Request Review

@pullfrog pullfrog Bot left a comment

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.

ℹ️ No blocking issues — one completeness gap worth a look.

Reviewed changes — style-preserving package.json rewrites: detect the file's own indent unit, line endings, and EOF-newline state, and reproduce them on write instead of serde's hardwired two-space/LF/trailing-newline output.

  • Add JsonStyle + detect_json_style + serialize_json_with_style to aube-manifest — detects the first indented line's whitespace run as the indent unit, CRLF anywhere, and trailing-newline presence; Default reproduces the previous two-space/LF/trailing-newline output byte-for-byte.
  • Route the four settings writers in edits.rs through the new helperremove_setting_entry, edit_setting_map, add_to_pnpm_only_built_dependencies, set_pnpm_allow_builds_entries detect style before parse and drop the manual push('\n').
  • Route write_manifest_json + update_manifest_json_object through it — the add/remove/update/audit dep-section and object writers now preserve style; write_manifest_json falls back to the default style on a read error.
  • Tests — style-helper detection/serialization/round-trip, tab-indent preservation through nub update and a settings write, and CRLF + absent-trailing-newline preservation.

The style logic is a faithful port of nub-core's already-in-production detect_format/serialize_manifest (crates/nub-core/src/pm/resolve.rs), serde_json runs with preserve_order so key order survives, and the CRLF \n\r\n replace is safe because serde escapes in-string newlines. No correctness concerns in the changed code.

ℹ️ patch-commit / patch-remove still reformat the manifest

The PR fixes six writers, but two more in-place package.json mutators were left on the old serde_json::to_string_pretty + out.push('\n') pattern this PR is replacing. They live in aube/src/patches.rs and back aube patch-commit / patch-remove, so a patchedDependencies write into a tab- or CRLF-indented manifest will still whole-file reformat — the exact symptom the PR set out to eliminate, just on a path outside the enumerated scope.

Technical details
# `patch-commit` / `patch-remove` bypass the new style-preserving writers

## Affected sites
- `vendor/aube/crates/aube/src/patches.rs:534``upsert_manifest_patched_dependency` serializes with `serde_json::to_string_pretty(&value)` then `out.push('\n')`, writing `patchedDependencies` into the user's in-place `package.json`.
- `vendor/aube/crates/aube/src/patches.rs:579``remove_bun_patched_dependency` does the same on removal.
- `vendor/aube/crates/aube/src/commands/deploy/rewrite.rs:286` — also `to_string_pretty`, but writes a generated deploy artifact rather than the user's source manifest, so style fidelity matters far less; call it out only for completeness.

## Required outcome
- The two `patches.rs` in-place writers reproduce the manifest's own indent / line-ending / trailing-newline state, matching the six writers this PR already fixed.

## Suggested approach
- Both already read `raw` and parse via `aube_manifest::parse_json`. Add `let style = aube_manifest::detect_json_style(&raw);` before the parse and replace the `to_string_pretty(&value)` + `push('\n')` pair with `aube_manifest::serialize_json_with_style(&value, &style)`, exactly as the `edits.rs` writers now do.

## Open questions for the human
- Is the enumerated six-writer scope deliberate (patch paths deferred to a follow-up), or should `patch-commit`/`patch-remove` be folded into this PR? If deferred, a tracked follow-up keeps the gap from being silently forgotten.

Pullfrog  | Fix it ➔View workflow run | Using Claude Opus𝕏

…wline

Every package.json rewrite in the aube command layer went through
serde_json::to_string_pretty, which hardwires two-space indent and LF:
an update/add/audit or settings edit reindented a tab- or 4-space
manifest wholesale, converted CRLF to LF, and appended a trailing
newline — a whole-file diff for a one-key change. npm and pnpm both
detect and reproduce the file's own style; nub-core's pin writer
(edit_manifest) already does the same, but the vendored command
writers never got that treatment.

Add detect_json_style / serialize_json_with_style to aube-manifest
(first indented line's whitespace run as the indent unit, CRLF
reproduction, trailing-newline state) and route all six package.json
writers through them: write_manifest_json, update_manifest_json_object,
and the four namespace settings editors in workspace/edits.rs. Files
with no indentation to imitate keep the previous two-space output.
@afonsojramos

Copy link
Copy Markdown
Contributor Author

Good catch — folded patch-commit/patch-remove into the PR (same detect-before-parse + style-serialize pattern, plus a tab-preservation test). Left deploy/rewrite.rs on to_string_pretty deliberately since it writes a generated deploy artifact rather than the user's source manifest. Also rebased onto main.

@pullfrog pullfrog Bot left a comment

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.

ℹ️ No new issues in the code itself — one coverage gap in the accompanying test.

Reviewed changes — the delta since the prior review is commit 8a8aeb89, which closes the patches.rs completeness gap that review raised. Commit ca30e540 is the original d48a8389 rebased onto current main; its three files are unchanged apart from one upstream hunk in edits.rs picked up by the rebase.

  • Routed the two patch-commit / patch-remove manifest writers through the style-preserving helperupsert_manifest_patched_dependency and remove_bun_patched_dependency now detect the file's style from raw before parse_json consumes it and serialize with serialize_json_with_style, dropping the unconditional out.push('\n').
  • Added upsert_preserves_tab_indentation — asserts a tab-indented manifest survives upsert_patched_dependency without a two-space reindent.

Both substitutions are correct and consistent with the six writers fixed in the first commit: raw is borrowed for detection and then moved into parse_json, so there is no use-after-move, and the ordering matches edits.rs and manifest_io.rs. I also enumerated every remaining to_string_pretty in vendor/aube/crates — what's left is stdout JSON reporters, freshly-created seed manifests where the default style is correct, and deploy/rewrite.rs:286, which writes a generated deploy artifact rather than the user's source manifest. No user-manifest writer is left on the old pattern.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using Claude Opus𝕏

Comment thread vendor/aube/crates/aube/src/patches.rs
The two in-place patchedDependencies writers were left on the
to_string_pretty pattern the rest of this change replaces; route them
through the same style-preserving serializer. (deploy/rewrite.rs keeps
to_string_pretty deliberately: it emits a generated artifact, not the
user's source manifest.)
@afonsojramos

Copy link
Copy Markdown
Contributor Author

Right again — the fixture had no lockfile, so the write routed through config_write_targetedit_setting_map and the test duplicated the edits.rs coverage without touching the changed writer. Replaced it with bun_patch_writers_preserve_tab_indentation: a bun.lock in the fixture takes the LockfileKind::Bun arm into upsert_manifest_patched_dependency, the assertion on the top-level patchedDependencies field proves the routing reached that writer, and the removal leg covers remove_bun_patched_dependency the same way. Verified red with the two writer substitutions reverted, green with them in place.

@pullfrog pullfrog Bot left a comment

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.

✅ No new issues found.

Reviewed changes — the delta since the prior review is the patches.rs test only. The branch was force-pushed (8a8aeb895c1025a2); the tree diff between them touches nothing but that test, and no production code changed.

  • Reworked the patch-writer test into bun_patch_writers_preserve_tab_indentation — the fixture now writes a bun.lock, which takes the LockfileKind::Bun arm and routes the write through upsert_manifest_patched_dependency instead of falling through to edit_setting_map.
  • Added a routing assertion — the test checks the entry landed in the un-branded top-level patchedDependencies, which is what proves the changed writer was reached rather than the namespace editor.
  • Extended the test through remove_patched_dependencyremove_bun_patched_dependency's style path is now asserted too, where its previous coverage was style-agnostic.

I verified this empirically rather than by reading. On a clean 5c1025a2 tree, cargo test -p aube-manifest --lib passes 127 tests and cargo test -p aube --lib -- manifest_io patches passes 22, including the new one. Reverting the serialize_json_with_style substitution in remove_bun_patched_dependency alone fails the test at patches.rs:860; reverting it in upsert_manifest_patched_dependency alone fails the post-upsert assertion. Both writers are genuinely covered, so the coverage gap the prior review raised is closed. The bun.lock routing also holds at the source: detect_existing_lockfile_kind is existence-only over lockfile_candidates, the "{}" body is never parsed, and the Bun arm passes namespace: None.

Pullfrog  | View workflow run | Using Claude Opus𝕏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant