fix(add-repo): rewire deps in alphabetical order, cover bundleDependencies#584
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors scripts/add-repo.sh step 7 to perform cross-workspace dependency rewrites in a single jq pass per package.json, ensuring rewritten dependency keys are sorted alphabetically and extending rewrites to bundleDependencies / bundledDependencies.
Changes:
- Precomputes rewrite rules once and applies them with a single
jqfilter per workspacepackage.json. - Sorts dependency-shaped sections by key to reduce diff noise and match typical npm insertion order.
- Extends rewrites to
bundleDependencies/bundledDependenciesarrays (rewrite + sort + dedupe).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
Test Results 8 files ±0 1 114 suites ±0 12m 33s ⏱️ -16s Results for commit 4f3eb24. ± Comparison against base commit e7d24a3. This pull request removes 2 and adds 2 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
cwillisf
force-pushed
the
fix/add-repo-sort-and-bundle
branch
from
f5fd2c1 to
140c2e2
Compare
Restructure the dep rewiring step from a nested `(package × dep × kind ×
name)` loop, each iteration appending the new entry via `+= {key: value}`,
to a single jq invocation per package.json that walks all four dep sections
and rewrites matching keys to their @Scoped equivalents pinned to the local
workspace version. Each touched section is sorted alphabetically by key —
matching what `npm install --save` produces — so the inserted entry lands
in its expected position rather than at the bottom of the list.
The rewrite rules are precomputed once per import as a JSON array of
`{matchKeys, target, version}` records. The new package's variant includes
both the bare name and the @Scoped form so that pre-prefixed-but-stale
pins (e.g. `@scratch/task-herder: 12.7.1` shipped by the source repo) get
repinned. Other packages keep the existing policy of matching bare names
only.
A `reduce` over the four section keys with a `has`-style guard ensures
absent sections stay absent (no `peerDependencies: null` leakage).
`bundleDependencies` (and the legacy spelling `bundledDependencies`) is an array of bare package names that npm bundles when running `npm pack`. If a workspace declared one of these names by its bare form, the bare form survived the dep rewire even after the matching `dependencies` entry got renamed to `@scratch/<name>`. At publish time npm would then look up the (no-longer-present) bare name and silently skip bundling it. Extend the rewrite filter with a type-aware branch: object sections are mapped per entry and sorted by key; array sections are mapped per name and uniqued (sort plus dedupe — useful in the corner case where both the bare and @Scoped forms appear in the same array). Add both bundle spellings to the reduce list. Non-array, non-object values (e.g. `bundleDependencies: true`, npm's shorthand for "bundle everything") fall through the `else .` branch unchanged.
Replace the `jq ... | sponge file` idiom with a `jq_in_place FILE ...` helper that writes the jq output to a sibling tempfile and `mv`s it into place only on `jq` exit 0. A `jq` failure now leaves `file` untouched and returns the non-zero status, so `set -e` aborts at the two call sites that don't check explicitly (the new-package fix-up and the workspaces-list insert) and the rewiring loop's `if !` catches it as before. With `jq | sponge`, the pipeline's exit status was sponge's, so a `jq` error would write empty or partial JSON over the input and the script would happily march on. The helper seeds the tempfile with `cp -p "$file" "$tmp"` so it inherits the original's mode (and content); the subsequent `>` truncates without touching the mode bits, and the final mv carries them back. Without this step, mktemp's default 0600 would silently downgrade 0644-mode package.json files. The replacement also drops the `moreutils` (sponge) prerequisite — once nothing in the script reads-and-writes the same file in one pipeline, sponge's purpose is gone.
In the per-package jq filter, if rewriting an object-shaped dep section produces two entries with the same key (e.g. the source package.json declared both "scratch-foo" and "@scratch/scratch-foo" in the same section), `from_entries` would keep the later of the two under jq's stable sort. That meant a stale `@scratch/...` pin could silently override the freshly-rewritten workspace version. Detect the duplicate after the map step and `error()` out, naming the section and the colliding key. `jq_in_place`'s exit-status check then leaves the file untouched and the rewiring loop falls through to `package_replacement_error`, so the developer sees both jq's error and the script's standard "could not replace monorepo refs in PKG" message and can clean up the input package.json. The scenario isn't expected to occur in practice; loud failure on an unexpected input is preferred over silently making a guess. Array sections (bundleDependencies, bundledDependencies) still use `unique` since they carry no version information — collapsing duplicates there is correct rather than fishy.
`jq -f --arg ... <(filter)` parses correctly — jq's argparser recognizes `--arg` as a flag rather than treating it as the filename for `-f` — but the layout makes a reader (LLM or human) reasonably wonder. Move the `--arg`s ahead of `-f` so the filter-file process substitution sits immediately after `-f`. Pure cosmetic change.
cwillisf
force-pushed
the
fix/add-repo-sort-and-bundle
branch
from
140c2e2 to
4f3eb24
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed Changes
Restructure step 7 (cross-workspace dep rewiring) in
scripts/add-repo.sh. Two commits:fix(add-repo): rewire monorepo deps in alphabetical order— replaces the per-(package × dep × kind × name)jq nest, which appended each new entry with+= {key: value}, with one jq invocation per package.json. The new filter walks all four dependency-shaped object sections, rewrites matching keys to their @Scoped equivalents pinned to the local workspace version, and sorts each touched section alphabetically by key. Result: inserted entries land wherenpm install --savewould put them, not at the bottom. Areduceover the section names with a.[$k]-truthy guard ensures missing sections stay missing (nopeerDependencies: nullleakage).feat(add-repo): rewire monorepo names in bundleDependencies too— extends the filter with a type-aware branch: object sections are mapped per entry and sorted by key; array sections are mapped per name anduniqued (sort + dedupe). AddsbundleDependenciesand the legacybundledDependenciesto the section list. Non-array, non-object values (e.g.bundleDependencies: true) fall through unchanged.The rewrite rules are precomputed once per import as
[{matchKeys, target, version}, ...]. The new package's variant includes both the bare and @-scoped forms inmatchKeysso its pre-prefixed-but-stale pins (e.g.@scratch/task-herder: 12.7.1shipped by the source repo) get repinned. Other packages keep the existing bare-name-only policy.Reason for Changes
Reported during a trial
./scripts/add-repo.sh scratch-storageimport:@scratch/scratch-storage: 13.7.4-svglanded at the bottom ofscratch-vm/package.json'sdependenciesinstead of between@scratch/scratch-renderand@scratch/scratch-svg-renderer. Same inscratch-gui. Cosmetic, but it produces diff noise on every import and would compound as more workspaces are added.bundleDependencieswas a latent gap: an entry rewritten fromscratch-storageto@scratch/scratch-storageindependencieswould have left a bare"scratch-storage"orphan inbundleDependencies, and at publish time npm would silently skip bundling the no-longer-existing name. None of the current workspaces usebundleDependencies, but covering it now was almost free with the restructured filter.Test Coverage
bundleDependenciesarray gets rewrites + sort + dedupe; legacybundledDependenciesspelling; non-arraybundleDependencies: trueleft alone; duplicate collapse viaunique../scripts/add-repo.sh scratch-storageend-to-end on a throwaway branch.@scratch/scratch-storagelands between@scratch/scratch-renderand@scratch/scratch-svg-rendererin bothscratch-vm/package.jsonandscratch-gui/package.json.@scratch/task-herderin the newly-importedscratch-storage/package.jsonlands between@babel/runtimeandarraybuffer-loader(and was repinned from the upstream's12.7.1to the local13.7.4-svg). Verified viahasthat absent sections (optionalDependencies,bundleDependencies,bundledDependencies) stayed absent.shellcheck scripts/add-repo.shis clean.