Nested executables fix

__tests__/commands/_helpers.js

@@ -114,7 +114,7 @@ export async function run<T, R>(
   try {
     const config = new Config(reporter);
     await config.init({
-      binLinks: !!flags.binLinks,
+      binLinks: typeof flags.binLinks === 'boolean' ? flags.binLinks : true,
       cwd,
       globalFolder: path.join(cwd, '.yarn-global'),
       cacheFolder: flags.cacheFolder || path.join(cwd, '.yarn-cache'),

__tests__/commands/global.js

@@ -1,9 +1,11 @@
 /* @flow */
 
 import type {CLIFunctionReturn} from '../../src/types.js';
+import type Config from '../../src/config.js';
 import {ConsoleReporter} from '../../src/reporters/index.js';
 import {run as buildRun} from './_helpers.js';
 import {run as global} from '../../src/cli/commands/global.js';
+import stringify from '../../src/lockfile/stringify.js';
 import * as fs from '../../src/util/fs.js';
 import assert from 'assert';
 
@@ -12,17 +14,25 @@ jasmine.DEFAULT_TIMEOUT_INTERVAL = 90000;
 const os = require('os');
 const path = require('path');
 
+// call getGlobalPath with the path to GLOBAL_BIN as the 1st argument to get the same path
+// for all platforms – path-to-temp-folder/.yarn-tmpbin/bin
+const GLOBAL_BIN = process.platform === 'win32' ? path.join('.yarn-tmpbin', 'bin') : '.yarn-tmpbin';
+
 const fixturesLoc = path.join(__dirname, '..', 'fixtures', 'global');
 const runGlobal = buildRun.bind(
   null,
   ConsoleReporter,
   fixturesLoc,
   (args, flags, config, reporter): CLIFunctionReturn => {
+    if (typeof flags.prefix === 'undefined' && !flags._noPrefix) {
+      flags.prefix = path.join(config.cwd, GLOBAL_BIN);
+    }
+    delete flags._noPrefix;
     return global(config, reporter, flags, args);
   },
 );
 
-function getGlobalPath(prefix, name): string {
+function getGlobalPath(prefix, name = ''): string {
   if (process.platform === 'win32') {
     return path.join(prefix, name);
   } else {
@@ -34,12 +44,56 @@ function getTempGlobalFolder(): string {
   return path.join(os.tmpdir(), `yarn-global-${Math.random()}`);
 }
 
-test.concurrent('add without flag', (): Promise<void> => {
-  return runGlobal(['add', 'react-native-cli'], {}, 'add-without-flag', async (config) => {
-    assert.ok(await fs.exists(path.join(config.globalFolder, 'node_modules', 'react-native-cli')));
-    assert.ok(await fs.exists(path.join(config.globalFolder, 'node_modules', '.bin', 'react-native')));
-  });
-});
+/**
+ * Test that all the links in the `yarn global bin` folder are from globally
+ * installed packages and that all the bins from the packages installed
+ */
+async function testFilesInGlobalBin(
+  config: Config,
+  expected: Array<string>,
+  userCmd?: Array<string> = [],
+): Promise<void> {
+  if (!config.binLinks) {
+    expected = [];
+  }
+  if (process.platform === 'win32') {
+    expected = expected.reduce((a, b) => {
+      return a.concat(b, `${b}.cmd`);
+    }, []);
+  }
+  const expectedList = [...expected, ...userCmd].sort().join(',');
+
+  // read the content of the folder
+  // global function change config.cwd to config.globalFolder,
+  // go one level up to be on the .yarn folder
+  const binFolder = getGlobalPath(path.join(config.cwd, '..', GLOBAL_BIN));
+
+  // when the folder not exist set files to empty array
+  let files = [];
+  try {
+    files = await fs.readdir(binFolder);
+  } catch (ex) {}
+
+  assert.equal(files.sort().join(','), expectedList);
+}
+
+// Make sure the file was updated by comparing the first 10 characters
+async function compareContent(config: Config): Promise<void> {
+  if (!config.binLinks) {
+    return;
+  }
+  const binFolder = getGlobalPath(path.join(config.cwd, '..', GLOBAL_BIN));
+  const expectedFolder = path.join(config.cwd, '..', 'expected-bin-files');
+  const files = await fs.readdir(expectedFolder);
+  for (const file of files) {
+    const [win, cmd] = [process.platform === 'win32', file.endsWith('.cmd')];
+    if (win && cmd || !win && !cmd) {
+      const actual = await fs.readFile(path.join(binFolder, file));
+      const expected = await fs.readFile(path.join(expectedFolder, file));
+      assert.equal(actual.substr(0, 10), expected.substr(0, 10));
+    }
+  }
+}
 
 test.concurrent('add with prefix flag', (): Promise<void> => {
   const tmpGlobalFolder = getTempGlobalFolder();
@@ -48,14 +102,81 @@ test.concurrent('add with prefix flag', (): Promise<void> => {
   });
 });
 
+test.concurrent('add with .yarnrc file', (): Promise<void> => {
+  const tmpGlobalFolder = getTempGlobalFolder();
+  return runGlobal(['add', 'react-native-cli'], {_noPrefix: true}, 'add-with-yarnrc-file', async (config) => {
+    assert.ok(await fs.exists(getGlobalPath(tmpGlobalFolder, 'react-native')));
+  }, async (cwd) => {
+    // create .yarnrc file and place it in .yarn-global
+    const loc = path.join(cwd, '.yarn-global', '.yarnrc');
+    await fs.mkdirp(path.join(cwd, '.yarn-global'));
+    await fs.writeFilePreservingEol(loc, `${stringify({prefix: tmpGlobalFolder})}\n`);
+  });
+});
+
 // don't run this test in `concurrent`, it will affect other tests
-test('add with PREFIX enviroment variable', (): Promise<void> => {
+test('add with PREFIX environment variable', (): Promise<void> => {
   const tmpGlobalFolder = getTempGlobalFolder();
   const envPrefix = process.env.PREFIX;
   process.env.PREFIX = tmpGlobalFolder;
-  return runGlobal(['add', 'react-native-cli'], {}, 'add-with-prefix-env', async (config) => {
-    assert.ok(await fs.exists(getGlobalPath(tmpGlobalFolder, 'react-native')));
+  return runGlobal(['add', 'react-native-cli'], {_noPrefix: true}, 'add-with-prefix-env', async (config) => {
     // restore env
     process.env.PREFIX = envPrefix;
+    assert.ok(await fs.exists(getGlobalPath(tmpGlobalFolder, 'react-native')));
   });
 });
+
+function globalAddBins(binLinks): Function {
+  return (): Promise<void> => {
+    return runGlobal(['add', 'react-native-cli'], {binLinks}, 'global-add-with-bin', async (config) => {
+      assert.ok(await fs.exists(path.join(config.cwd, 'node_modules', 'react-native-cli')));
+      const binExist = await fs.exists(path.join(config.cwd, 'node_modules', '.bin', 'react-native'));
+      assert.ok(config.binLinks ? binExist : !binExist);
+      await testFilesInGlobalBin(config, ['react-native']);
+      await compareContent(config);
+    });
+  };
+}
+
+function globalRemoveBins(binLinks): Function {
+  return (): Promise<void> => {
+    // A@1 -> B@1
+    // C@1
+    // react-native - missing bin
+
+    // remove A
+
+    // result....
+    // C@1 - without bin
+    // react-native - with bin
+    const name = 'global-remove';
+    return runGlobal(['remove', 'dep-a'], {binLinks}, name, async (config, reporter) => {
+      // the link for react-native was missing frol the files in the fixtures folder,
+      // we expect it to be installed
+      await testFilesInGlobalBin(config, ['react-native'], ['user-command']);
+      assert.ok(!await fs.exists(path.join(config.cwd, 'node_modules/dep-a')));
+      assert.ok(!await fs.exists(path.join(config.cwd, 'node_modules/dep-b')));
+      assert.ok(await fs.exists(path.join(config.cwd, 'node_modules/dep-c')));
+    });
+  };
+}
+
+function globalUpgradeBins(binLinks): Function {
+  return (): Promise<void> => {
+    const name = 'global-update-with-bin';
+    return runGlobal(['upgrade', 'react-native-cli'], {binLinks}, name, async (config) => {
+      await testFilesInGlobalBin(config, ['react-native']);
+      await compareContent(config);
+    });
+  };
+}
+
+// flags.binLinks = true
+test.concurrent('global add: package with bin, flags: {binlinks: true}', globalAddBins(true));
+test.concurrent('global remove: dependencies with bins, flags: {binlinks: true}', globalRemoveBins(true));
+test.concurrent('global upgrade: package with bin, flags: {binlinks: true}', globalUpgradeBins(true));
+
+// flags.binLinks = false
+test.concurrent('global add: package with bin, flags: {binlinks: false}', globalAddBins(false));
+test.concurrent('global remove: dependencies with bins, flags: {binlinks: false}', globalRemoveBins(false));
+test.concurrent('global upgrade: package with bin, flags: {binlinks: false}', globalUpgradeBins(false));

__tests__/commands/install/integration.js

@@ -6,7 +6,8 @@ import * as reporters from '../../../src/reporters/index.js';
 import {Install} from '../../../src/cli/commands/install.js';
 import Lockfile from '../../../src/lockfile/wrapper.js';
 import * as fs from '../../../src/util/fs.js';
-import {getPackageVersion, explodeLockfile, runInstall, createLockfile} from '../_helpers.js';
+import {ConsoleReporter} from '../../../src/reporters/index.js';
+import {getPackageVersion, explodeLockfile, createLockfile, runInstall, run as buildRun} from '../_helpers.js';
 import {promisify} from '../../../src/util/promise';
 
 jasmine.DEFAULT_TIMEOUT_INTERVAL = 90000;
@@ -18,6 +19,8 @@ const fsNode = require('fs');
 const path = require('path');
 const os = require('os');
 
+const fixturesLoc = path.join(__dirname, '..', '..', 'fixtures', 'install');
+
 beforeEach(request.__resetAuthedRequests);
 // $FlowFixMe
 afterEach(request.__resetAuthedRequests);
@@ -452,15 +455,39 @@ test.concurrent(
   },
 );
 
-// disabled to resolve https://github.com/yarnpkg/yarn/pull/1210
-test.skip('install should hoist nested bin scripts', (): Promise<void> => {
-  return runInstall({binLinks: true}, 'install-nested-bin', async (config) => {
+test.concurrent('install should hoist nested bin scripts', (): Promise<void> => {
+  // we mock linkBin to enable us to test it
+  let mockLinkBin;
+
+  const run = buildRun.bind(
+    null,
+    ConsoleReporter,
+    fixturesLoc,
+    async (args, flags, config, reporter, lockfile): Promise<Install> => {
+      const install = new Install(flags, config, reporter, lockfile);
+      const linkBin = install.linker.linkBin;
+      install.linker.linkBin = mockLinkBin = jest.fn((...args) => {
+        return linkBin(...args);
+      });
+      await install.init();
+      await check(config, reporter, {}, []);
+      return install;
+    },
+    [],
+  );
+
+  return run({binLinks: true}, 'install-nested-bin', async (config) => {
     const binScripts = await fs.walk(path.join(config.cwd, 'node_modules', '.bin'));
     // need to double the amount as windows makes 2 entries for each dependency
     // so for below, there would be an entry for eslint and eslint.cmd on win32
     const amount = process.platform === 'win32' ? 20 : 10;
     assert.equal(binScripts.length, amount);
     assert(binScripts.findIndex((f) => f.basename === 'eslint') > -1);
+
+    // linkBin should be called once for each bin
+    const locations = mockLinkBin.mock.calls.map((call) => call[0]);
+    const uniqueResult = locations.filter((loc, index, arr) => arr.indexOf(loc) === index);
+    assert.equal(locations.length, uniqueResult.length);
   });
 });
 

__tests__/commands/remove.js

@@ -1,5 +1,6 @@
 /* @flow */
 
+import type {Install} from '../../src/cli/commands/install.js';
 import {ConsoleReporter} from '../../src/reporters/index.js';
 import {run as buildRun, explodeLockfile} from './_helpers.js';
 import {run as remove} from '../../src/cli/commands/remove.js';
@@ -12,9 +13,12 @@ jasmine.DEFAULT_TIMEOUT_INTERVAL = 90000;
 const path = require('path');
 
 const fixturesLoc = path.join(__dirname, '..', 'fixtures', 'remove');
-const runRemove = buildRun.bind(null, ConsoleReporter, fixturesLoc, (args, flags, config, reporter): Promise<void> => {
-  return remove(config, reporter, flags, args);
-});
+const runRemove = buildRun.bind(
+  null,
+  ConsoleReporter,
+  fixturesLoc,
+  (args, flags, config, reporter): Promise<Install> => remove(config, reporter, flags, args),
+);
 
 test.concurrent('throws error with no arguments', (): Promise<void> => {
   const reporter = new reporters.ConsoleReporter({});

__tests__/commands/upgrade.js

@@ -1,5 +1,6 @@
 /* @flow */
 
+import type {Add} from '../../src/cli/commands/add.js';
 import {ConsoleReporter} from '../../src/reporters/index.js';
 import {explodeLockfile, run as buildRun} from './_helpers.js';
 import {run as upgrade} from '../../src/cli/commands/upgrade.js';
@@ -12,9 +13,12 @@ jasmine.DEFAULT_TIMEOUT_INTERVAL = 90000;
 const path = require('path');
 
 const fixturesLoc = path.join(__dirname, '..', 'fixtures', 'upgrade');
-const runUpgrade = buildRun.bind(null, ConsoleReporter, fixturesLoc, (args, flags, config, reporter): Promise<void> => {
-  return upgrade(config, reporter, flags, args);
-});
+const runUpgrade = buildRun.bind(
+  null,
+  ConsoleReporter,
+  fixturesLoc,
+  (args, flags, config, reporter): Promise<Add> => upgrade(config, reporter, flags, args),
+);
 
 test.concurrent('throws if lockfile is out of date', (): Promise<void> => {
   const reporter = new reporters.ConsoleReporter({});

__tests__/fixtures/global/add-with-prefix-env/.yarn-global/.yarnrc

@@ -0,0 +1 @@
+prefix ""

__tests__/fixtures/global/global-add-with-bin/expected-bin-files/react-native

@@ -0,0 +1 @@
+#!/usr/bin/env node

__tests__/fixtures/global/global-add-with-bin/expected-bin-files/react-native.cmd

@@ -0,0 +1 @@
+@IF EXIST "%~dp0\node.exe"

__tests__/fixtures/global/global-remove/.yarn-global/.npmrc

@@ -0,0 +1 @@
+yarn-offline-mirror=./mirror-for-offline