From 5ca7db7f60dd6a8066d62cdd4baba84645dc062e Mon Sep 17 00:00:00 2001 From: Jeff Valore Date: Sun, 23 Dec 2018 06:57:51 -0500 Subject: [PATCH 1/3] 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 --- __tests__/commands/_helpers.js | 1 + __tests__/commands/install/integration.js | 13 +++++++++++++ .../extraneous-node-modules/node_modules/extra.js | 0 .../install/extraneous-node-modules/package.json | 5 +++++ .../extraneous-node-modules/some_modules/extra.js | 0 src/cli/commands/autoclean.js | 8 ++------ src/cli/commands/check.js | 12 ++++++------ src/config.js | 2 +- src/util/execute-lifecycle-script.js | 8 ++------ 9 files changed, 30 insertions(+), 19 deletions(-) create mode 100644 __tests__/fixtures/install/extraneous-node-modules/node_modules/extra.js create mode 100644 __tests__/fixtures/install/extraneous-node-modules/package.json create mode 100644 __tests__/fixtures/install/extraneous-node-modules/some_modules/extra.js diff --git a/__tests__/commands/_helpers.js b/__tests__/commands/_helpers.js index 1bd4d9f590..af71470dcd 100644 --- a/__tests__/commands/_helpers.js +++ b/__tests__/commands/_helpers.js @@ -96,6 +96,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, ); diff --git a/__tests__/commands/install/integration.js b/__tests__/commands/install/integration.js index 116f59c4c4..771156d2cb 100644 --- a/__tests__/commands/install/integration.js +++ b/__tests__/commands/install/integration.js @@ -1139,3 +1139,16 @@ test('install will not warn for missing optional peer dependencies', () => const warningMessage = messageParts.every(part => stdout.includes(part)); 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 => { + 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); + }); +}); diff --git a/__tests__/fixtures/install/extraneous-node-modules/node_modules/extra.js b/__tests__/fixtures/install/extraneous-node-modules/node_modules/extra.js new file mode 100644 index 0000000000..e69de29bb2 diff --git a/__tests__/fixtures/install/extraneous-node-modules/package.json b/__tests__/fixtures/install/extraneous-node-modules/package.json new file mode 100644 index 0000000000..eea039fddf --- /dev/null +++ b/__tests__/fixtures/install/extraneous-node-modules/package.json @@ -0,0 +1,5 @@ +{ + "dependencies": { + "feed": "0.3.0" + } +} diff --git a/__tests__/fixtures/install/extraneous-node-modules/some_modules/extra.js b/__tests__/fixtures/install/extraneous-node-modules/some_modules/extra.js new file mode 100644 index 0000000000..e69de29bb2 diff --git a/src/cli/commands/autoclean.js b/src/cli/commands/autoclean.js index db2e7f6405..805125e9a5 100644 --- a/src/cli/commands/autoclean.js +++ b/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; diff --git a/src/cli/commands/check.js b/src/cli/commands/check.js index 52f5e9e99c..1c2cc2c925 100644 --- a/src/cli/commands/check.js +++ b/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 = 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}`, diff --git a/src/config.js b/src/config.js index 97af68ac20..0a0fa80d91 100644 --- a/src/config.js +++ b/src/config.js @@ -324,7 +324,7 @@ export default class Config { } if (this.modulesFolder) { - this.registryFolders.push(this.modulesFolder); + this.registryFolders = [this.modulesFolder]; } this.networkConcurrency = diff --git a/src/util/execute-lifecycle-script.js b/src/util/execute-lifecycle-script.js index aae9a2d09d..64f600200a 100644 --- a/src/util/execute-lifecycle-script.js +++ b/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; From 8d906d599ae430e93cae66fcdd363f281eac3f49 Mon Sep 17 00:00:00 2001 From: Jeff Valore Date: Sun, 23 Dec 2018 07:12:57 -0500 Subject: [PATCH 2/3] update changelog --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0981085e9e..247f37b6f1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,10 @@ Please add one entry in this file for each change in Yarn's behavior. Use the sa ## Master +- Fix `--modules-folder` handling in several places. `yarn check` now respects `--modules-folder`. `node_modules` no longer checked for extraneous files). + + [#6850](https://github.com/yarnpkg/yarn/pull/6850) - [**Jeff Valore**](https://twitter.com/codingwithspike) + ## 1.13.0 - Implements a new `package.json` field: `peerDependenciesMeta` From 6cc7c3e98597a06d23f204a8e4bee1a64ab99057 Mon Sep 17 00:00:00 2001 From: Jeff Valore Date: Sat, 19 Jan 2019 10:14:06 -0500 Subject: [PATCH 3/3] fix --moduls-folder handling in bin and run commands --- src/cli/commands/bin.js | 3 +-- src/cli/commands/run.js | 5 ++--- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/src/cli/commands/bin.js b/src/cli/commands/bin.js index c8f419f35c..1738c0b3ef 100644 --- a/src/cli/commands/bin.js +++ b/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): Promise { - 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 { diff --git a/src/cli/commands/run.js b/src/cli/commands/run.js index 66fbaaff09..b35e8443ae 100644 --- a/src/cli/commands/run.js +++ b/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 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