Skip to content
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

Correctly install workspace child deps when workspace child not symlinked to root #7289

Merged
merged 4 commits into from Nov 22, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
Expand Up @@ -4,6 +4,10 @@ Please add one entry in this file for each change in Yarn's behavior. Use the sa

## Master

- Correctly installs workspace child dependencies when workspace child not symlinked to root.

[#7289](https://github.com/yarnpkg/yarn/pull/7289) - [**Daniel Tschinder**](https://github.com/danez)

- Makes running scripts with Plug'n Play possible on node 13

[#7650](https://github.com/yarnpkg/yarn/pull/7650) - [**Sander Verweij**](https://github.com/sverweij)
Expand Down
19 changes: 19 additions & 0 deletions __tests__/commands/install/workspaces-install.js
Expand Up @@ -103,6 +103,25 @@ test.concurrent('install should install unhoistable dependencies in workspace no
});
});

test.concurrent(
'install should install unhoistable dependencies in workspace node_modules even when no symlink exists',
(): Promise<void> => {
return runInstall({}, 'workspaces-install-conflict-without-symlink', async (config): Promise<void> => {
// node_modules/isarray@1.0.0
let packageFile = await fs.readFile(path.join(config.cwd, 'node_modules', 'isarray', 'package.json'));
expect(JSON.parse(packageFile).version).toEqual('1.0.0');

// node_modules/arrify@1.0.0
packageFile = await fs.readFile(path.join(config.cwd, 'node_modules', 'arrify', 'package.json'));
expect(JSON.parse(packageFile).version).toEqual('1.0.0');

// node_modules/arrify/isarray@2.0.0
packageFile = await fs.readFile(path.join(config.cwd, 'arrify', 'node_modules', 'isarray', 'package.json'));
expect(JSON.parse(packageFile).version).toEqual('2.0.0');
});
},
);

test.concurrent('install should link workspaces that refer each other', (): Promise<void> => {
return runInstall({}, 'workspaces-install-link', async (config): Promise<void> => {
// packages/workspace-1/node_modules/left-pad - missing because it is hoisted to the root
Expand Down
@@ -0,0 +1,7 @@
{
"name": "arrify",
"version": "2.0.0",
"dependencies": {
"isarray": "2.0.0"
}
}
@@ -0,0 +1,11 @@
{
"name": "my-project",
"private": true,
"dependencies": {
"arrify": "1.0.0",
"isarray": "1.0.0"
},
"workspaces": [
"arrify"
]
}
Binary file not shown.
Binary file not shown.
33 changes: 26 additions & 7 deletions src/package-hoister.js
Expand Up @@ -835,6 +835,15 @@ export default class PackageHoister {
// up of modules from different registries so we need to handle this specially
const parts: Array<string> = [];
const keyParts = key.split('#');
const isWorkspaceEntry = this.workspaceLayout && keyParts[0] === this.workspaceLayout.virtualManifestName;

// Don't add the virtual manifest (keyParts.length === 1)
// or ws childs which were not hoisted to the root (keyParts.length === 2).
// If a ws child was hoisted its key would not contain the virtual manifest name
if (isWorkspaceEntry && keyParts.length <= 2) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is the second part of the condition ([...] which already have a different version in the root)?

Copy link
Contributor Author

@danez danez May 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is dependent on the length:

So basically it is this:

if (
  this.workspaceLayout && // Do we have a workspace?
  keyParts[0] === this.workspaceLayout.virtualManifestName && // Is the first keypart the virtual manifest name? If so we are dealing with modules from within workspaces or the virtual manifest itself
  (
    keyParts.length === 1 || // the key is "workspace-aggregator-1234" Which is the virtual workspace itself
    keyParts.length === 2  //  the key is "workspace-aggregator-1234#child" Which is one of the workspace childs which could not be hoisted.
  )
){}

If the workspace child was hoisted to the root the key would be child directly or respectively child#deps which can be installed correctly already, because child is symlinked in the root node_modules folder.

If the length is > 3 then we are dealing with dependencies of a ws child which was not hoisted to the root node_modules folder, which is exactly what we want to handle.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also updated the comment to describe it better.

continue;
}

for (let i = 0; i < keyParts.length; i++) {
const key = keyParts.slice(0, i + 1).join('#');
const hoisted = this.tree.get(key);
Expand All @@ -843,16 +852,26 @@ export default class PackageHoister {
parts.push(keyParts[i]);
}

const shallowLocs = [];
if (this.config.modulesFolder) {
// remove the first part which will be the folder name and replace it with a
// hardcoded modules folder
parts.splice(0, 1, this.config.modulesFolder);
// Check if the destination is pointing to a sub folder of the virtualManifestName
// e.g. _project_/node_modules/workspace-aggregator-123456/node_modules/workspaceChild/node_modules/dependency
// This probably happened because the hoister was not able to hoist the workspace child to the root
// So we have to change the folder to the workspace package location
if (this.workspaceLayout && isWorkspaceEntry) {
const wspPkg = this.workspaceLayout.workspaces[keyParts[1]];
invariant(wspPkg, `expected workspace package to exist for "${keyParts[1]}"`);
parts.splice(0, 4, wspPkg.loc);
} else {
// first part will be the registry-specific module folder
parts.splice(0, 0, this.config.lockfileFolder);
if (this.config.modulesFolder) {
// remove the first part which will be the folder name and replace it with a
// hardcoded modules folder
parts.splice(0, 1, this.config.modulesFolder);
} else {
// first part will be the registry-specific module folder
parts.splice(0, 0, this.config.lockfileFolder);
}
}

const shallowLocs = [];
info.shallowPaths.forEach(shallowPath => {
const shallowCopyParts = parts.slice();
shallowCopyParts[0] = this.config.cwd;
Expand Down
4 changes: 0 additions & 4 deletions src/package-linker.js
Expand Up @@ -251,10 +251,6 @@ export default class PackageLinker {
} else if (workspaceLayout && remote.type === 'workspace' && !isShallow) {
src = remote.reference;
type = 'symlink';
if (dest.indexOf(workspaceLayout.virtualManifestName) !== -1) {
// we don't need to install virtual manifest
continue;
}
// to get real path for non hoisted dependencies
symlinkPaths.set(dest, src);
} else {
Expand Down