Preserve resolved versions when generating NPM lockfile#176
Merged
Conversation
Read the workspace root package-lock.json via Arborist.loadVirtual and copy entries for the target workspace's transitive closure verbatim, preserving the original version, resolved, and integrity fields. The previous buildIdealTree path resolved dependencies against the isolate manifest and produced lockfiles whose versions could drift from the monorepo's root lockfile. Falls back to the prior buildIdealTree behavior when no root package-lock.json exists (forceNpm from pnpm/bun/yarn, modern Yarn).
Contributor
There was a problem hiding this comment.
Pull request overview
Updates the NPM lockfile isolation flow to preserve the monorepo’s already-resolved versions and integrity metadata by subsetting the root package-lock.json, improving deployment reproducibility.
Changes:
- Pass additional workspace/manifest context into the NPM lockfile generator from
processLockfile. - Rework NPM lockfile generation to prefer rewriting from the root
package-lock.jsonvia ArboristloadVirtual+ dependency graph traversal, with a fallback tobuildIdealTreewhen no root lockfile exists. - Add unit tests for the pure JSON lockfile rewrite helper (
buildIsolatedLockfileJson).
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/lib/lockfile/process-lockfile.ts | Passes target workspace name/manifest and registry context into generateNpmLockfile. |
| src/lib/lockfile/process-lockfile.test.ts | Updates expectations for the new generateNpmLockfile parameter set. |
| src/lib/lockfile/helpers/generate-npm-lockfile.ts | Implements root-lockfile-based rewrite path, adds pure JSON rewrite helper and fallback path. |
| src/lib/lockfile/helpers/generate-npm-lockfile.test.ts | Adds unit tests validating rewrite behavior (root remap, preserved entries, overrides, etc.). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
When the target workspace has its own nested node_modules in the source lockfile (a version override different from the hoisted root version), those entries have source paths like packages/<target>/node_modules/<dep>. Since the target becomes the isolate root, these need to be remapped to node_modules/<dep>; otherwise they'd stay at a path that doesn't exist in the isolate and the target would silently install the wrong version. This is exactly the mono-ts firebase-admin scenario from #111.
Checks in a real npm workspaces monorepo as a fixture (root manifest, package-lock.json from a real npm install, and workspace package.json files) with the target pinning semver@^6 and a sibling pinning semver@^7 — so npm hoists 7.7.4 to the root and nests 6.3.1 under the target package. The test copies the fixture to a tmp directory and runs the real generateNpmLockfile (with Arborist loadVirtual) against it, asserting the isolated lockfile surfaces the nested 6.3.1 at the isolate root's node_modules and excludes the hoisted 7.7.4 that only the sibling needs. Reverting either the loadVirtual seed or the nested-path remap regresses this test.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 11 changed files in this pull request and generated 3 comments.
Files not reviewed (1)
- src/lib/lockfile/helpers/fixtures/nested-version-override/workspace/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
- Rename targetLink to targetImporterNode and compute targetLinkLoc directly from the package name. workspaceNodes returns the real importer Node (not a Link); Node.target getter returns this, which let the old code pass tests by coincidence. - Throw when the target importer entry is missing from srcData.packages, and when any reachable node has no corresponding source entry. These are invariants that should fail fast rather than produce a malformed lockfile silently. - Spread top-level fields from the source lockfile so unknown fields survive isolation, and drop the legacy v2 dependencies tree which would be stale after subsetting. - Add an integration test fixture with three internal workspaces (api depends on shared, shared depends on utils) that exercises the adapted-manifest overlay path for internal dep lockfile entries.
- When two source entries remap to the same output path with different content, throw a descriptive error rather than silently dropping one. This happens when the target pins a nested version override that collides with a hoisted version still needed by another reachable dependency. Identical colliding entries are still allowed (no-op). - When Arborist's virtual tree has no packages map (e.g. unusual lockfile shapes), fall back to buildIdealTree instead of throwing. - Add unit tests for both the conflict and the no-conflict collision cases.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 16 changed files in this pull request and generated 2 comments.
Files not reviewed (2)
- src/lib/lockfile/helpers/fixtures/internal-deps/workspace/package-lock.json: Language not supported
- src/lib/lockfile/helpers/fixtures/nested-version-override/workspace/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 0937972. Configure here.
- Only remap nested paths under the target when they live under `<target>/node_modules/` — preserves nested workspace importer paths like `packages/api/lib/core` so the internal-dep overlay loop finds them and so install paths stay correct. - Throw when the target importer didn't end up in the reachable set before overlaying the root entry. - Stop defaulting `requires: true` when the source lockfile omitted it; propagate via the srcData spread only when it was there.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 16 changed files in this pull request and generated 1 comment.
Files not reviewed (2)
- src/lib/lockfile/helpers/fixtures/internal-deps/workspace/package-lock.json: Language not supported
- src/lib/lockfile/helpers/fixtures/nested-version-override/workspace/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Closes #111.
The NPM lockfile generator previously called
arborist.buildIdealTree()on the isolate directory, which ignores the monorepo's root
package-lock.jsonand resolves dependencies against the isolate'sadapted manifest. The isolated lockfile could therefore contain
different versions and integrity hashes than the monorepo's lockfile,
breaking reproducibility for deployments.
The new implementation uses Arborist only for graph traversal.
loadVirtualat the workspace root hydrates every Node with theoriginal
resolvedandintegrityfrom the root lockfile (no registrycalls), and
workspaceDependencySetidentifies the transitive closureneeded by the target. The isolated
package-lock.jsonis then builtby copying matching entries verbatim from the source lockfile, so
versions and integrity are preserved exactly.
Path remapping handles the target becoming the isolate root:
packages/<target>becomes""packages/<target>/node_modules/<dep>becomesnode_modules/<dep>— this is the case from NPM lockfile versions do not match #111 where the target pins a different
version than is hoisted at the workspace root
node_modules/<target>self-link is droppednode_modules/…paths and internal deps' paths are keptverbatim
The root
""entry and each internal dependency entry are overlaidwith their adapted manifest deps so workspace references become
file:references.
When no root
package-lock.jsonexists (forceNpmfrom pnpm/bun/yarn,or the modern-Yarn fallback), the generator falls back to the prior
buildIdealTreebehavior unchanged.A checked-in fixture reproduces the #111 scenario (real npm workspaces
monorepo where the target pins semver@^6 and a sibling pins semver@^7,
so npm hoists 7.7.4 at root and nests 6.3.1 under the target). The
integration test runs
generateNpmLockfileagainst it and asserts theisolated lockfile surfaces the nested 6.3.1 at the isolate root and
excludes the sibling's hoisted 7.7.4.
Scope: lockfile (npm)
Visibility: user-facing