fix(linker): Fix yarn removing linked deps during link stage (yarnpkg…

…#4757)

**Summary**

 Actual fix: changed fs.readlink to fs.realpath when checking if a symlink is a linked dependency in package-linker.js This fixes yarn removing linked deps when installing or updating.

Fixes yarnpkg#3288, fixes yarnpkg#4770, fixes yarnpkg#4635, fixes yarnpkg#4603.

Potential fix for yarnpkg#3202.

**Test plan**

See yarnpkg#3288 (comment) for repro steps.
See yarnpkg#3288 (comment) for my explanation of the problem.

With a real world test scenario this works, but I'm unable to have it break from a unit test. I added a test in the integration suite but with the bug added back in it still passes because both generated paths are identical. I would like some help with the unit test.

__tests__/commands/_helpers.js

@@ -10,16 +10,17 @@ import * as fs from '../../src/util/fs.js';
 import {Install} from '../../src/cli/commands/install.js';
 import Config from '../../src/config.js';
 import parsePackagePath from '../../src/util/parse-package-path.js';
-
+import type {CLIFunctionReturn} from '../../src/types.js';
+import {run as link} from '../../src/cli/commands/link.js';
 const stream = require('stream');
 const path = require('path');
 
-const fixturesLoc = path.join(__dirname, '..', 'fixtures', 'install');
+const installFixturesLoc = path.join(__dirname, '..', 'fixtures', 'install');
 
 export const runInstall = run.bind(
   null,
   ConsoleReporter,
-  fixturesLoc,
+  installFixturesLoc,
   async (args, flags, config, reporter, lockfile): Promise<Install> => {
     const install = new Install(flags, config, reporter, lockfile);
     await install.init();
@@ -30,6 +31,17 @@ export const runInstall = run.bind(
   [],
 );
 
+const linkFixturesLoc = path.join(__dirname, '..', 'fixtures', 'link');
+
+export const runLink = run.bind(
+  null,
+  ConsoleReporter,
+  linkFixturesLoc,
+  (args, flags, config, reporter): CLIFunctionReturn => {
+    return link(config, reporter, flags, args);
+  },
+);
+
 export async function createLockfile(dir: string): Promise<Lockfile> {
   const lockfileLoc = path.join(dir, constants.LOCKFILE_FILENAME);
   let lockfile;
@@ -94,7 +106,7 @@ export async function run<T, R>(
   ) => Promise<T> | T,
   args: Array<string>,
   flags: Object,
-  name: string | {source: string, cwd: string},
+  name: string | {source?: string, cwd: string},
   checkInstalled: ?(config: Config, reporter: R, install: T, getStdout: () => string) => ?Promise<void>,
   beforeInstall: ?(cwd: string) => ?Promise<void>,
 ): Promise<void> {
@@ -113,11 +125,16 @@ export async function run<T, R>(
   if (fixturesLoc) {
     const source = typeof name === 'string' ? name : name.source;
 
-    const dir = path.join(fixturesLoc, source);
-    cwd = await fs.makeTempDir(path.basename(dir));
-    await fs.copy(dir, cwd, reporter);
-    if (typeof name !== 'string') {
-      cwd = path.join(cwd, name.cwd);
+    // if source wasn't set then assume we were given a complete path
+    if (typeof source === 'undefined') {
+      cwd = typeof name !== 'string' ? name.cwd : await fs.makeTempDir();
+    } else {
+      const dir = path.join(fixturesLoc, source);
+      cwd = await fs.makeTempDir(path.basename(dir));
+      await fs.copy(dir, cwd, reporter);
+      if (typeof name !== 'string') {
+        cwd = path.join(cwd, name.cwd);
+      }
     }
   } else {
     // if fixture loc is not set then CWD is some empty temp dir

__tests__/commands/install/integration.js

@@ -11,7 +11,7 @@ import {parse} from '../../../src/lockfile';
 import {Install, run as install} from '../../../src/cli/commands/install.js';
 import Lockfile from '../../../src/lockfile';
 import * as fs from '../../../src/util/fs.js';
-import {getPackageVersion, explodeLockfile, runInstall, createLockfile, run as buildRun} from '../_helpers.js';
+import {getPackageVersion, explodeLockfile, runInstall, runLink, createLockfile, run as buildRun} from '../_helpers.js';
 
 jasmine.DEFAULT_TIMEOUT_INTERVAL = 150000;
 
@@ -866,29 +866,6 @@ test('install a scoped module from authed private registry with a missing traili
   });
 });
 
-test.concurrent('install will not overwrite files in symlinked scoped directories', async (): Promise<void> => {
-  await runInstall(
-    {},
-    'install-dont-overwrite-linked-scoped',
-    async (config): Promise<void> => {
-      const dependencyPath = path.join(config.cwd, 'node_modules', '@fakescope', 'fake-dependency');
-      expect('Symlinked scoped package test').toEqual(
-        (await fs.readJson(path.join(dependencyPath, 'package.json'))).description,
-      );
-      expect(await fs.exists(path.join(dependencyPath, 'index.js'))).toEqual(false);
-    },
-    async cwd => {
-      const dirToLink = path.join(cwd, 'dir-to-link');
-
-      await fs.mkdirp(path.join(cwd, '.yarn-link', '@fakescope'));
-      await fs.symlink(dirToLink, path.join(cwd, '.yarn-link', '@fakescope', 'fake-dependency'));
-
-      await fs.mkdirp(path.join(cwd, 'node_modules', '@fakescope'));
-      await fs.symlink(dirToLink, path.join(cwd, 'node_modules', '@fakescope', 'fake-dependency'));
-    },
-  );
-});
-
 test.concurrent('install of scoped package with subdependency conflict should pass check', (): Promise<void> => {
   return runInstall({}, 'install-scoped-package-with-subdependency-conflict', async (config, reporter) => {
     let allCorrect = true;
@@ -1134,3 +1111,54 @@ test.concurrent('warns for missing bundledDependencies', (): Promise<void> => {
     'missing-bundled-dep',
   );
 });
+
+test.concurrent('install will not overwrite linked scoped dependencies', async (): Promise<void> => {
+  // install only dependencies
+  await runInstall({production: true}, 'install-dont-overwrite-linked', async (installConfig): Promise<void> => {
+    // link our fake dep to the registry
+    await runLink([], {}, 'package-with-name-scoped', async (linkConfig): Promise<void> => {
+      // link our fake dependency in our node_modules
+      await runLink(
+        ['@fakescope/a-package'],
+        {linkFolder: linkConfig.linkFolder},
+        {cwd: installConfig.cwd},
+        async (): Promise<void> => {
+          // check that it exists (just in case)
+          const existed = await fs.exists(path.join(installConfig.cwd, 'node_modules', '@fakescope', 'a-package'));
+          expect(existed).toEqual(true);
+
+          // run install to install dev deps which would remove the linked dep if the bug was present
+          await runInstall({linkFolder: linkConfig.linkFolder}, {cwd: installConfig.cwd}, async (): Promise<void> => {
+            // if the linked dep is still there is a win :)
+            const existed = await fs.exists(path.join(installConfig.cwd, 'node_modules', '@fakescope', 'a-package'));
+            expect(existed).toEqual(true);
+          });
+        },
+      );
+    });
+  });
+});
+
+test.concurrent('install will not overwrite linked dependencies', async (): Promise<void> => {
+  // install only dependencies
+  await runInstall({production: true}, 'install-dont-overwrite-linked', async (installConfig): Promise<void> => {
+    // link our fake dep to the registry
+    await runLink([], {}, 'package-with-name', async (linkConfig): Promise<void> => {
+      // link our fake dependency in our node_modules
+      await runLink(['a-package'], {linkFolder: linkConfig.linkFolder}, {cwd: installConfig.cwd}, async (): Promise<
+        void,
+      > => {
+        // check that it exists (just in case)
+        const existed = await fs.exists(path.join(installConfig.cwd, 'node_modules', 'a-package'));
+        expect(existed).toEqual(true);
+
+        // run install to install dev deps which would remove the linked dep if the bug was present
+        await runInstall({linkFolder: linkConfig.linkFolder}, {cwd: installConfig.cwd}, async (): Promise<void> => {
+          // if the linked dep is still there is a win :)
+          const existed = await fs.exists(path.join(installConfig.cwd, 'node_modules', 'a-package'));
+          expect(existed).toEqual(true);
+        });
+      });
+    });
+  });
+});

__tests__/commands/link.js

@@ -1,24 +1,12 @@
 /* @flow */
 
-import {run as buildRun} from './_helpers.js';
-import {run as link} from '../../src/cli/commands/link.js';
+import {runLink} from './_helpers.js';
 import {ConsoleReporter} from '../../src/reporters/index.js';
-import type {CLIFunctionReturn} from '../../src/types.js';
 import mkdir from './../_temp.js';
 import * as fs from '../../src/util/fs.js';
 
 const path = require('path');
 
-const fixturesLoc = path.join(__dirname, '..', 'fixtures', 'link');
-const runLink = buildRun.bind(
-  null,
-  ConsoleReporter,
-  fixturesLoc,
-  (args, flags, config, reporter): CLIFunctionReturn => {
-    return link(config, reporter, flags, args);
-  },
-);
-
 test.concurrent('creates folder in linkFolder', async (): Promise<void> => {
   const linkFolder = await mkdir('link-folder');
   await runLink([], {linkFolder}, 'package-with-name', async (config, reporter): Promise<void> => {

__tests__/fixtures/install/install-dont-overwrite-linked/package.json

@@ -0,0 +1,8 @@
+{
+  "dependencies": {
+    "left-pad": "1.1.3"
+  },
+  "devDependencies": {
+    "is-buffer": "^1.1.5"
+  }
+}

__tests__/fixtures/link/package-with-name-scoped/package.json

@@ -0,0 +1,3 @@
+{
+  "name": "@fakescope/a-package"
+}

src/package-linker.js

@@ -304,8 +304,13 @@ export default class PackageLinker {
       const stat = await fs.lstat(entryPath);
 
       if (stat.isSymbolicLink()) {
-        const packageName = entry;
-        linkTargets.set(packageName, await fs.readlink(entryPath));
+        try {
+          const entryTarget = await fs.realpath(entryPath);
+          linkTargets.set(entry, entryTarget);
+        } catch (err) {
+          this.reporter.warn(this.reporter.lang('linkTargetMissing', entry));
+          await fs.unlink(entryPath);
+        }
       } else if (stat.isDirectory() && entry[0] === '@') {
         // if the entry is directory beginning with '@', then we're dealing with a package scope, which
         // means we must iterate inside to retrieve the package names it contains
@@ -317,7 +322,13 @@ export default class PackageLinker {
 
           if (stat2.isSymbolicLink()) {
             const packageName = `${scopeName}/${entry2}`;
-            linkTargets.set(packageName, await fs.readlink(entryPath2));
+            try {
+              const entryTarget = await fs.realpath(entryPath2);
+              linkTargets.set(packageName, entryTarget);
+            } catch (err) {
+              this.reporter.warn(this.reporter.lang('linkTargetMissing', packageName));
+              await fs.unlink(entryPath2);
+            }
           }
         }
       }
@@ -334,7 +345,7 @@ export default class PackageLinker {
       if (
         (await fs.lstat(loc)).isSymbolicLink() &&
         linkTargets.has(packageName) &&
-        linkTargets.get(packageName) === (await fs.readlink(loc))
+        linkTargets.get(packageName) === (await fs.realpath(loc))
       ) {
         possibleExtraneous.delete(loc);
         copyQueue.delete(loc);

src/reporters/lang/en.js

@@ -172,6 +172,7 @@ const messages = {
   linkUsing: 'Using linked module for $0.',
   linkDisusing: 'Removed linked module $0.',
   linkDisusingMessage: 'You will need to run `yarn` to re-install the package that was linked.',
+  linkTargetMissing: 'The target of linked module $0 is missing. Removing link.',
 
   createInvalidBin: 'Invalid bin entry found in package $0.',
   createMissingPackage: