fix(modules-folder): Fix --modules-folder handling in several places. (…

…#6850)

* fix(modules-folder): Fix --modules-folder handling in several places.

Fixes a few issues with the `--modules-folder` parameter.

The extraneous file check would always
also clean files from `node_modules`. This no longer happens, only the `--modules-folder` location
is checked.

The `yarn check` command used to always only check `node_modules`. Now it will check the
`--modules-folder` location instead.

When running a command or executing a lifecycle script, yarn
would build a `PATH` that included `node_modules/.bin` first, then add the `--modules-folder`'s
`/.bin` after, which would have led to the wrong binaries being executed.

fixes #5419 and fixes #6847

* update changelog

* fix --moduls-folder handling in bin and run commands

CHANGELOG.md

@@ -4,11 +4,15 @@ Please add one entry in this file for each change in Yarn's behavior. Use the sa
 
 ## Master
 
+- Fixes `--modules-folder` handling in several places (ex: `yarn check` now respects `--modules-folder`)
+
+  [#6850](https://github.com/yarnpkg/yarn/pull/6850) - [**Jeff Valore**](https://twitter.com/codingwithspike)
+
 - Removes `rootModuleFolders` (internal variable which wasn't used anywhere)
 
   [#6846](https://github.com/yarnpkg/yarn/pull/6846) - [**Jeff Valore**](https://twitter.com/codingwithspike)
 
-- Allows `global-folder` to be set in `.yarnrc` files
+- Adds support for setting `global-folder` from `.yarnrc` files
 
   [#7056](https://github.com/yarnpkg/yarn/pull/7056) - [**Hsiao-nan Cheung**](https://github.com/niheaven)
 

__tests__/commands/_helpers.js

@@ -97,6 +97,7 @@ export function makeConfigFromDirectory(cwd: string, reporter: Reporter, flags:
       focus: !!flags.focus,
       enableDefaultRc: !flags.noDefaultRc,
       extraneousYarnrcFiles: flags.useYarnrc,
+      modulesFolder: flags.modulesFolder ? path.join(cwd, flags.modulesFolder) : undefined,
     },
     reporter,
   );

__tests__/commands/install/integration.js

@@ -1140,6 +1140,19 @@ test('install will not warn for missing optional peer dependencies', () =>
     expect(warningMessage).toBe(false);
   }));
 
+test('does not check node_modules for extraneous files when --modules-folder used', async () => {
+  // Scenario: https://github.com/yarnpkg/yarn/issues/5419
+  // When `--modules-foler` is passed, yarn should check that directory for extraneous files.
+  // Also, the default node_modules dir, if it exists, should not be cleaned out (marked as extraneous).
+  await runInstall({modulesFolder: './some_modules'}, 'extraneous-node-modules', async (config): Promise<void> => {
+    expect(await fs.exists(`${config.cwd}/some_modules/feed`)).toEqual(true);
+    // Extraneous files in node_modules should not have been cleaned.
+    expect(await fs.exists(`${config.cwd}/node_modules/extra.js`)).toEqual(true);
+    // Extraneous files in some_modules should have been cleaned.
+    expect(await fs.exists(`${config.cwd}/some_modules/extra.js`)).toEqual(false);
+  });
+});
+
 test('install skips the scripts if the yarnrc specifies skip-scripts true', () =>
   runInstall({}, 'ignore-scripts-by-yarnrc', (config, reporter, install, getStdout) => {
     const stdout = getStdout();

__tests__/fixtures/install/extraneous-node-modules/package.json

@@ -0,0 +1,5 @@
+{
+  "dependencies": {
+    "feed": "0.3.0"
+  }
+}

src/cli/commands/autoclean.js

@@ -78,12 +78,8 @@ export async function clean(
 
   // build list of possible module folders
   const locs = new Set();
-  if (config.modulesFolder) {
-    locs.add(config.modulesFolder);
-  }
-  for (const name of registryNames) {
-    const registry = config.registries[name];
-    locs.add(path.join(config.lockfileFolder, registry.folder));
+  for (const registryFolder of config.registryFolders) {
+    locs.add(path.resolve(config.lockfileFolder, registryFolder));
   }
 
   const workspaceRootFolder = config.workspaceRootFolder;

src/cli/commands/bin.js

@@ -2,7 +2,6 @@
 
 import type {Reporter} from '../../reporters/index.js';
 import type Config from '../../config.js';
-import RegistryYarn from '../../resolvers/registries/yarn-resolver.js';
 import {getBinEntries} from './run.js';
 
 const path = require('path');
@@ -16,7 +15,7 @@ export function setFlags(commander: Object) {
 }
 
 export async function run(config: Config, reporter: Reporter, flags: Object, args: Array<string>): Promise<void> {
-  const binFolder = path.join(config.cwd, config.registries[RegistryYarn.registry].folder, '.bin');
+  const binFolder = path.join(config.cwd, config.registryFolders[0], '.bin');
   if (args.length === 0) {
     reporter.log(binFolder, {force: true});
   } else {

src/cli/commands/check.js

@@ -39,7 +39,7 @@ export async function verifyTreeCheck(
   }
   // check all dependencies recursively without relying on internal resolver
   const registryName = 'yarn';
-  const registry = config.registries[registryName];
+  const registryFolder = config.registryFolders[0];
   const cwd = config.workspaceRootFolder ? config.lockfileFolder : config.cwd;
   const rootManifest = await config.readManifest(cwd, registryName);
 
@@ -86,7 +86,7 @@ export async function verifyTreeCheck(
   const locationsVisited: Set<string> = new Set();
   while (dependenciesToCheckVersion.length) {
     const dep = dependenciesToCheckVersion.shift();
-    const manifestLoc = path.join(dep.parentCwd, registry.folder, dep.name);
+    const manifestLoc = path.resolve(dep.parentCwd, registryFolder, dep.name);
     if (locationsVisited.has(manifestLoc + `@${dep.version}`)) {
       continue;
     }
@@ -114,19 +114,19 @@ export async function verifyTreeCheck(
     const dependencies = pkg.dependencies;
     if (dependencies) {
       for (const subdep in dependencies) {
-        const subDepPath = path.join(manifestLoc, registry.folder, subdep);
+        const subDepPath = path.resolve(manifestLoc, registryFolder, subdep);
         let found = false;
         const relative = path.relative(cwd, subDepPath);
-        const locations = path.normalize(relative).split(registry.folder + path.sep).filter(dir => !!dir);
+        const locations = path.normalize(relative).split(registryFolder + path.sep).filter(dir => !!dir);
         locations.pop();
         while (locations.length >= 0) {
           let possiblePath;
           if (locations.length > 0) {
-            possiblePath = path.join(cwd, registry.folder, locations.join(path.sep + registry.folder + path.sep));
+            possiblePath = path.join(cwd, registryFolder, locations.join(path.sep + registryFolder + path.sep));
           } else {
             possiblePath = cwd;
           }
-          if (await fs.exists(path.join(possiblePath, registry.folder, subdep))) {
+          if (await fs.exists(path.resolve(possiblePath, registryFolder, subdep))) {
             dependenciesToCheckVersion.push({
               name: subdep,
               originalKey: `${dep.originalKey}#${subdep}`,

src/cli/commands/run.js

@@ -5,7 +5,6 @@ import type Config from '../../config.js';
 import {execCommand, makeEnv} from '../../util/execute-lifecycle-script.js';
 import {dynamicRequire} from '../../util/dynamic-require.js';
 import {MessageError} from '../../errors.js';
-import {registries} from '../../resolvers/index.js';
 import * as fs from '../../util/fs.js';
 import * as constants from '../../constants.js';
 
@@ -29,8 +28,8 @@ export async function getBinEntries(config: Config): Promise<Map<string, string>
   const binEntries = new Map();
 
   // Setup the node_modules/.bin folders for analysis
-  for (const registry of Object.keys(registries)) {
-    binFolders.add(path.join(config.cwd, config.registries[registry].folder, '.bin'));
+  for (const registryFolder of config.registryFolders) {
+    binFolders.add(path.resolve(config.lockfileFolder, registryFolder, '.bin'));
   }
 
   // Same thing, but for the pnp dependencies, located inside the cache

src/config.js

@@ -317,7 +317,7 @@ export default class Config {
     }
 
     if (this.modulesFolder) {
-      this.registryFolders.push(this.modulesFolder);
+      this.registryFolders = [this.modulesFolder];
     }
 
     this.networkConcurrency =

src/util/execute-lifecycle-script.js

@@ -7,7 +7,6 @@ import * as child from './child.js';
 import * as fs from './fs.js';
 import {dynamicRequire} from './dynamic-require.js';
 import {makePortableProxyScript} from './portable-script.js';
-import {registries} from '../resolvers/index.js';
 import {fixCmdWinSlashes} from './fix-cmd-win-slashes.js';
 import {getBinFolder as getGlobalBinFolder, run as globalRun} from '../cli/commands/global.js';
 
@@ -191,16 +190,13 @@ export async function makeEnv(
   }
 
   // Add node_modules .bin folders to the PATH
-  for (const registry of Object.keys(registries)) {
-    const binFolder = path.join(config.registries[registry].folder, '.bin');
+  for (const registryFolder of config.registryFolders) {
+    const binFolder = path.join(registryFolder, '.bin');
     if (config.workspacesEnabled && config.workspaceRootFolder) {
       pathParts.unshift(path.join(config.workspaceRootFolder, binFolder));
     }
     pathParts.unshift(path.join(config.linkFolder, binFolder));
     pathParts.unshift(path.join(cwd, binFolder));
-    if (config.modulesFolder) {
-      pathParts.unshift(path.join(config.modulesFolder, '.bin'));
-    }
   }
 
   let pnpFile;