Correctly install workspace child deps when workspace child not symli…

…nked to root (yarnpkg#7289)

* Test for conflict between pkg versions in root and workspace child

This test creates the scenario where the child is not symlinked into the root node_modules folder, because
the root installs a different version of the workspace child from the registry.

* fix(hoister): Correctly change path for ws child dependencies when child not hoisted

* Update CHANGELOG.md

* Better comment

CHANGELOG.md

@@ -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)

__tests__/commands/install/workspaces-install.js

@@ -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

__tests__/fixtures/install/workspaces-install-conflict-without-symlink/arrify/package.json

@@ -0,0 +1,7 @@
+{
+  "name": "arrify",
+  "version": "2.0.0",
+  "dependencies": {
+    "isarray": "2.0.0"
+  }
+}

__tests__/fixtures/install/workspaces-install-conflict-without-symlink/package.json

@@ -0,0 +1,11 @@
+{
+  "name": "my-project",
+  "private": true,
+  "dependencies": {
+    "arrify": "1.0.0",
+    "isarray": "1.0.0"
+  },
+  "workspaces": [
+    "arrify"
+  ]
+}

src/package-hoister.js

@@ -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) {
+        continue;
+      }
+
       for (let i = 0; i < keyParts.length; i++) {
         const key = keyParts.slice(0, i + 1).join('#');
         const hoisted = this.tree.get(key);
@@ -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;

src/package-linker.js

@@ -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 {