fix: support npm install-strategy=linked (.store layout) - #596
Open
manzoorwanijk wants to merge 10 commits into
Open
manzoorwanijk wants to merge 10 commits into
manzoorwanijk wants to merge 10 commits into
Conversation
Author
|
@ds300 any chance of getting this reviewed anytime soon? |
|
@ds300 LGTM! |
Author
|
A gentle reminder about this |
Scoped packages live under .store/@scope/, and nested lockfile paths do not always match the real dependent, so pick the store entry by lockfile version instead of the first name match. Drop the name-only ancestor node_modules fallback, which could patch an unrelated copy.
Reading VCS details, the state file and fast-forwarding during a rebase all used the logical node_modules path, which does not exist for packages that only live in .store.
Rebase looked up the state file and reversed patches at the logical node_modules path, so it reported no state for .store-only packages.
The app's .npmrc is copied into the temp repo, so install-strategy=linked turned the package into a symlink and git could not diff it.
Follow workspace links into the root .store, read versions from the store entry name so patches to package.json stay resolvable, and refuse to pick between same-version entries that npm keys by dependency graph.
--create-issue looked up VCS details again at the logical path, which throws for packages that only live in .store.
Aliased dependencies are stored under their real package name, which the lockfile records in the entry's name field.
makePatch reads the lockfile entry for the requested path, so resolving a nested spec that npm hoists elsewhere crashed later. Check that the package exists before any lookup that assumes it does.
Stripping it changed CLI output and broke the dev-only-patches snapshot.
Author
|
Today I asked Claude Code and Codex to review the changes and they found some issues which I have fixed in the above commits. |
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
2 participants
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.
Summary
Adds support for npm
install-strategy=linked, where transitive and nested packages only exist undernode_modules/.store. Creating, updating, appending, rebasing, applying and reversing patches now work for those packages. Regular installs are unaffected, because resolution only runs when the expected path is missing.Fixes #595
How it works
.store/<name>@<version>-<hash>, scoped under.store/@scope/) using the version from the lockfile..storework too.makePatchtemp repo installs withinstall-strategy=hoisted, since a copied.npmrcwould otherwise turn the package into a symlink git can't diff.Verified end to end with npm 11. CI's npm versions don't support the linked strategy, so coverage here is unit tests.