Add test for #6058 nm mistreating link: deps as inner workspace #6089
Conversation
| { | ||
| private: true, | ||
| dependencies: { | ||
| [`one-fixed-dep`]: `2.0.0`, |
There was a problem hiding this comment.
I bet the line above is not needed. The test should be minimal
| [`app`]: `link:./app`, | ||
| }, | ||
| workspaces: [ | ||
| `packages/*`, |
| nodeLinker: `node-modules`, | ||
| }, | ||
| async ({path, run, source}) => { | ||
| await writeJson(npath.toPortablePath(`${path}/packages/workspace/package.json`), { |
There was a problem hiding this comment.
Again, this statement should be irrelevant to the test and can be removed
|
@joshhunt Many thanks! Please shrink down the test to a minimum, I made notices about the lines that can be safely removed in my opinion. Please correct me if I am wrong. |
|
Thanks for the review @larixer! I've pushed up what I believe to be the most minimal reproduction of the issue that fails on master. Indeed the If I remove all the suggested lines, the issue doesn't reproduce on master: test(`should not treat link: dependencies as an inner workspaces`,
makeTemporaryEnv(
{
private: true,
dependencies: {
// [`one-fixed-dep`]: `2.0.0`,
[`app`]: `link:./app`,
},
workspaces: [
`app/plugins/*`,
],
},
{
nodeLinker: `node-modules`,
},
async ({path, run, source}) => {
console.log(`path`, path);
await writeJson(npath.toPortablePath(`${path}/app/plugins/foo-plugin/package.json`), {
name: `foo-plugin`,
version: `1.0.0`,
dependencies: {
[`one-fixed-dep`]: `1.0.0`,
},
});
await run(`install`);
expect(await xfs.existsPromise(npath.toPortablePath(`${path}/app/node_modules`))).toBe(false);
},
),
);~/d/g/yarn-berry (master)> tree /tmp/xfs-6cf41900/test
/tmp/xfs-6cf41900/test
├── app
│ └── plugins
│ └── foo-plugin
│ └── package.json
├── node_modules
│ ├── app -> ../app
│ ├── foo-plugin -> ../app/plugins/foo-plugin
│ ├── no-deps
│ │ ├── index.js
│ │ ├── no-deps-1.0.0.js
│ │ └── package.json
│ └── one-fixed-dep
│ ├── index.js
│ └── package.json
├── package.json
└── yarn.lock
8 directories, 8 files(I added the extra file to the I'm a bit out of my depth with all this package manager stuff, but I believe conflicting versions of a transitive depedency ( Here's the problematic tree (from master) ~/d/g/yarn-berry (master)> tree /tmp/xfs-570c7b4d/test
/tmp/xfs-570c7b4d/test
├── app
│ ├── node_modules
│ │ └── no-deps
│ │ ├── index.js
│ │ ├── no-deps-1.0.0.js
│ │ └── package.json
│ └── plugins
│ └── foo-plugin
│ ├── node_modules
│ │ └── one-fixed-dep
│ │ ├── index.js
│ │ └── package.json
│ └── package.json
├── node_modules
│ ├── app -> ../app
│ ├── foo-plugin -> ../app/plugins/foo-plugin
│ ├── no-deps
│ │ ├── index.js
│ │ ├── no-deps-2.0.0.js
│ │ └── package.json
│ └── one-fixed-dep
│ ├── index.js
│ └── package.json
├── package.json
└── yarn.lock
12 directories, 13 filesNote that it's not the direct dependency Just for throughness, here's what the file tree should look like with your fix: ~/d/g/yarn-berry (joshhunt/nm-mistreatment-link-as-workspace-fix-test)> tree /tmp/xfs-484f3a71/test
/tmp/xfs-484f3a71/test
├── app
│ └── plugins
│ └── foo-plugin
│ ├── node_modules
│ │ ├── no-deps
│ │ │ ├── index.js
│ │ │ └── package.json
│ │ └── one-fixed-dep
│ │ ├── index.js
│ │ └── package.json
│ └── package.json
├── node_modules
│ ├── app -> ../app
│ ├── foo-plugin -> ../app/plugins/foo-plugin
│ ├── no-deps
│ │ ├── index.js
│ │ └── package.json
│ └── one-fixed-dep
│ ├── index.js
│ └── package.json
├── package.json
└── yarn.lock
11 directories, 11 files |
merceyz
merged commit e8632d1
into
yarnpkg:larixer/nm-mistreatment-link-as-workspace-fix
What's the problem this PR addresses?
Adds an integration test for #6058 to assert that link: dependencies are not mistreated as inner workspaces.
It took a bit of trial and error to get the right dependency tree that causes the issue, but eventually figured it out. It's roughly the same as https://github.com/joshhunt/yarn-repro-uuid-install-location, but with the existing fixtures.
...
How did you fix it?
It's just a test :)
I wrote the test against main to make sure it fails, and then check out the PR with the test and it passed :)
Test output against main and branch
...
Checklist