-
-
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
Changes from all commits
ab6f33d
1141a60
4cf48ff
0240d62
979778c
e6e0fd5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
releases: | ||
"@yarnpkg/cli": patch | ||
"@yarnpkg/nm": patch | ||
"@yarnpkg/plugin-nm": patch | ||
|
||
declined: | ||
- "@yarnpkg/plugin-compat" | ||
- "@yarnpkg/plugin-constraints" | ||
- "@yarnpkg/plugin-dlx" | ||
- "@yarnpkg/plugin-essentials" | ||
- "@yarnpkg/plugin-init" | ||
- "@yarnpkg/plugin-interactive-tools" | ||
- "@yarnpkg/plugin-npm-cli" | ||
- "@yarnpkg/plugin-pack" | ||
- "@yarnpkg/plugin-patch" | ||
- "@yarnpkg/plugin-pnp" | ||
- "@yarnpkg/plugin-pnpm" | ||
- "@yarnpkg/plugin-stage" | ||
- "@yarnpkg/plugin-typescript" | ||
- "@yarnpkg/plugin-version" | ||
- "@yarnpkg/plugin-workspace-tools" | ||
- "@yarnpkg/builder" | ||
- "@yarnpkg/core" | ||
- "@yarnpkg/doctor" | ||
- "@yarnpkg/pnpify" |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -550,7 +550,6 @@ describe(`Dragon tests`, () => { | |
for (const [nodeLinker, shouldHaveAccessToTheSameInstance] of [ | ||
[`pnp`, true], | ||
[`pnpm`, true], | ||
[`node-modules`, false], | ||
]) { | ||
test(`it should pass the dragon test 11 with "nodeLinker: ${nodeLinker}"`, | ||
makeTemporaryEnv( | ||
|
@@ -580,9 +579,10 @@ describe(`Dragon tests`, () => { | |
await expect(run(`install`)).resolves.toBeTruthy(); | ||
|
||
// Make sure that both the root and dragon-test-11-b have access to the same instance. | ||
// This is only possible with the PnP and pnpm linkers, because the node-modules linker | ||
// 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, | ||
// 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 commentThe 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 commentThe 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. |
||
await expect(source(` | ||
(() => { | ||
const {createRequire} = require(\`module\`); | ||
|
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) 🤔