-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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.
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
fix(plugin-nm): Avoid duplicate copies inside aliased dependencies. #4571
Conversation
// can't fulfill the peer dependency promise. For the NM linker we test that it at least | ||
// fulfills the require promise (installing dragon-test-11-a both under the aliased and original name). | ||
// This is only possible with the PnP and pnpm linkers. | ||
// The node-modules linker can't fullfil these requirements for aliased packages, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps it'd make sense to have a page in the documentation explaining the tradeoffs between the different install strategies (and why some things cannot be implemented) 🤔
// This is only possible with the PnP and pnpm linkers. | ||
// The node-modules linker can't fullfil these requirements for aliased packages, | ||
// without resorting to symlinks and a layout resembling pnpm for aliased dependencies, | ||
// which will be too different from the classic node_modules layout for all the other dependencies. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The symlink layout you described on Discord would only affect aliased dependencies, right? Given that they are fairly rare, that nothing would change for regular dependencies, and that it doesn't have the cyclic symlink issue, I feel like it might be worth a try - but your call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. It should work. However, it needs to be implemented carefully. Plus it might be worth resorting to symlinks only when the dependency is actually peer dependend upon or when self references support is enabled. So, if we making this carefully, it's not trivial, need 2-3 days to spend for implementation and tests... Should be done in a separate PR I think.
02ef138
to
4cf48ff
Compare
…4571) * Avoids creation of circular symlinks and duplicates inside aliases * Add a test to check that duplicate copies of aliased packages are not created * Stricter ident comparison for hoisted packages * chore: update changelog Co-authored-by: merceyz <[email protected]>
What's the problem this PR addresses?
The PR:
#3631
attempted to fix peer dependendents finding aliased packages. However, it really didn't fix the issue for node modules linker. Peer dependents still were finding a different copy of a package, plus duplicated package is problematic for disk footprint and performance considerations.
How did you fix it?
I have essentially rolled back the effects of the abovementioned PR and its side effect of creating duplicate copies of aliased dependencies. I have also added a check to prevent circular symbolic links creation by the nm linker, which is a related problem, reported multiple times:
#3256
#3409
#4232
Checklist