diff --git a/lib/base-command.js b/lib/base-command.js index 870c69acc492d..c5bd3fd94f960 100644 --- a/lib/base-command.js +++ b/lib/base-command.js @@ -7,8 +7,6 @@ class BaseCommand { constructor (npm) { this.wrapWidth = 80 this.npm = npm - this.workspaces = null - this.workspacePaths = null } get name () { @@ -75,7 +73,13 @@ class BaseCommand { } async setWorkspaces (filters) { - const ws = await getWorkspaces(filters, { path: this.npm.localPrefix }) + if (this.isArboristCmd) + this.includeWorkspaceRoot = false + + const ws = await getWorkspaces(filters, { + path: this.npm.localPrefix, + includeWorkspaceRoot: this.includeWorkspaceRoot, + }) this.workspaces = ws this.workspaceNames = [...ws.keys()] this.workspacePaths = [...ws.values()] diff --git a/lib/config.js b/lib/config.js index a56dd92ffbde6..be8b65cef2055 100644 --- a/lib/config.js +++ b/lib/config.js @@ -156,7 +156,7 @@ class Config extends BaseCommand { const out = [] for (const key of keys) { if (!publicVar(key)) - throw `The ${key} option is protected, and cannot be retrieved in this way` + throw new Error(`The ${key} option is protected, and cannot be retrieved in this way`) const pref = keys.length > 1 ? `${key}=` : '' out.push(pref + this.npm.config.get(key)) diff --git a/lib/npm.js b/lib/npm.js index 966d11210c275..ace6ecf03a941 100644 --- a/lib/npm.js +++ b/lib/npm.js @@ -138,6 +138,15 @@ const npm = module.exports = new class extends EventEmitter { const workspacesEnabled = this.config.get('workspaces') const workspacesFilters = this.config.get('workspace') const filterByWorkspaces = workspacesEnabled || workspacesFilters.length > 0 + // normally this would go in the constructor, but our tests don't + // actually use a real npm object so this.npm.config isn't always + // populated. this is the compromise until we can make that a reality + // and then move this into the constructor. + impl.workspaces = this.config.get('npm') + impl.workspacePaths = null + // normally this would be evaluated in base-command#setWorkspaces, see + // above for explanation + impl.includeWorkspaceRoot = this.config.get('include-workspace-root') if (this.config.get('usage')) { this.output(impl.usage) diff --git a/lib/repo.js b/lib/repo.js index e0172d01f63d1..a6fe008f6515b 100644 --- a/lib/repo.js +++ b/lib/repo.js @@ -48,7 +48,13 @@ class Repo extends BaseCommand { } async get (pkg) { - const opts = { ...this.npm.flatOptions, fullMetadata: true } + // XXX It is very odd that `where` is how pacote knows to look anywhere + // other than the cwd. + const opts = { + ...this.npm.flatOptions, + where: this.npm.localPrefix, + fullMetadata: true, + } const mani = await pacote.manifest(pkg, opts) const r = mani.repository diff --git a/lib/utils/config/definitions.js b/lib/utils/config/definitions.js index 36b8a84a61c47..0c1857234dcf4 100644 --- a/lib/utils/config/definitions.js +++ b/lib/utils/config/definitions.js @@ -908,6 +908,15 @@ define('include-staged', { flatten, }) +define('include-workspace-root', { + default: false, + type: Boolean, + description: ` + Include the workspace root when workspaces are enabled for a command. + `, + flatten, +}) + define('init-author-email', { default: '', type: String, @@ -2119,8 +2128,8 @@ define('workspace', { * Workspace names * Path to a workspace directory - * Path to a parent workspace directory (will result to selecting all of the - nested workspaces) + * Path to a parent workspace directory (will result in selecting all + workspaces within that folder) When set for the \`npm init\` command, this may be set to the folder of a workspace which does not yet exist, to create the folder and set it @@ -2129,13 +2138,16 @@ define('workspace', { }) define('workspaces', { - default: false, - type: Boolean, + default: null, + type: [null, Boolean], short: 'ws', envExport: false, description: ` Enable running a command in the context of **all** the configured workspaces. + + Explicitly setting this to false will cause commands like \`install\` to + ignore workspaces altogether. `, }) diff --git a/lib/workspaces/arborist-cmd.js b/lib/workspaces/arborist-cmd.js index cb6b66b8cb257..d6dad3cfb8d47 100644 --- a/lib/workspaces/arborist-cmd.js +++ b/lib/workspaces/arborist-cmd.js @@ -4,6 +4,11 @@ const BaseCommand = require('../base-command.js') class ArboristCmd extends BaseCommand { + constructor () { + super(...arguments) + this.isArboristCmd = true + } + /* istanbul ignore next - see test/lib/load-all-commands.js */ static get params () { return [ @@ -13,7 +18,7 @@ class ArboristCmd extends BaseCommand { } execWorkspaces (args, filters, cb) { - this.setWorkspaces(filters) + this.setWorkspaces(filters, true) .then(() => { this.exec(args, cb) }) diff --git a/lib/workspaces/get-workspaces.js b/lib/workspaces/get-workspaces.js index 91b0074556ae7..3eb8e4865b706 100644 --- a/lib/workspaces/get-workspaces.js +++ b/lib/workspaces/get-workspaces.js @@ -5,11 +5,16 @@ const rpj = require('read-package-json-fast') // Returns an Map of paths to workspaces indexed by workspace name // { foo => '/path/to/foo' } -const getWorkspaces = async (filters, { path }) => { +const getWorkspaces = async (filters, { path, includeWorkspaceRoot }) => { // TODO we need a better error to be bubbled up here if this rpj call fails const pkg = await rpj(resolve(path, 'package.json')) const workspaces = await mapWorkspaces({ cwd: path, pkg }) - const res = filters.length ? new Map() : workspaces + let res = new Map() + if (includeWorkspaceRoot) + res.set(pkg.name, path) + + if (!filters.length) + res = new Map([...res, ...workspaces]) for (const filterArg of filters) { for (const [workspaceName, workspacePath] of workspaces.entries()) { diff --git a/tap-snapshots/test/lib/utils/config/definitions.js.test.cjs b/tap-snapshots/test/lib/utils/config/definitions.js.test.cjs index 01b137b8af54a..18ceb1b3bb187 100644 --- a/tap-snapshots/test/lib/utils/config/definitions.js.test.cjs +++ b/tap-snapshots/test/lib/utils/config/definitions.js.test.cjs @@ -63,6 +63,7 @@ Array [ "ignore-scripts", "include", "include-staged", + "include-workspace-root", "init-author-email", "init-author-name", "init-author-url", @@ -825,6 +826,15 @@ Allow installing "staged" published packages, as defined by [npm RFC PR This is experimental, and not implemented by the npm public registry. ` +exports[`test/lib/utils/config/definitions.js TAP > config description for include-workspace-root 1`] = ` +#### \`include-workspace-root\` + +* Default: false +* Type: Boolean + +Include the workspace root when workspaces are enabled for a command. +` + exports[`test/lib/utils/config/definitions.js TAP > config description for init-author-email 1`] = ` #### \`init-author-email\` @@ -1833,8 +1843,8 @@ Valid values for the \`workspace\` config are either: * Workspace names * Path to a workspace directory -* Path to a parent workspace directory (will result to selecting all of the - nested workspaces) +* Path to a parent workspace directory (will result in selecting all + workspaces within that folder) When set for the \`npm init\` command, this may be set to the folder of a workspace which does not yet exist, to create the folder and set it up as a @@ -1846,12 +1856,15 @@ This value is not exported to the environment for child processes. exports[`test/lib/utils/config/definitions.js TAP > config description for workspaces 1`] = ` #### \`workspaces\` -* Default: false -* Type: Boolean +* Default: null +* Type: null or Boolean Enable running a command in the context of **all** the configured workspaces. +Explicitly setting this to false will cause commands like \`install\` to +ignore workspaces altogether. + This value is not exported to the environment for child processes. ` diff --git a/tap-snapshots/test/lib/utils/config/describe-all.js.test.cjs b/tap-snapshots/test/lib/utils/config/describe-all.js.test.cjs index 8487b45174cc3..c67450ec4032c 100644 --- a/tap-snapshots/test/lib/utils/config/describe-all.js.test.cjs +++ b/tap-snapshots/test/lib/utils/config/describe-all.js.test.cjs @@ -521,6 +521,13 @@ Allow installing "staged" published packages, as defined by [npm RFC PR This is experimental, and not implemented by the npm public registry. +#### \`include-workspace-root\` + +* Default: false +* Type: Boolean + +Include the workspace root when workspaces are enabled for a command. + #### \`init-author-email\` * Default: "" @@ -1246,8 +1253,8 @@ Valid values for the \`workspace\` config are either: * Workspace names * Path to a workspace directory -* Path to a parent workspace directory (will result to selecting all of the - nested workspaces) +* Path to a parent workspace directory (will result in selecting all + workspaces within that folder) When set for the \`npm init\` command, this may be set to the folder of a workspace which does not yet exist, to create the folder and set it up as a @@ -1257,12 +1264,15 @@ This value is not exported to the environment for child processes. #### \`workspaces\` -* Default: false -* Type: Boolean +* Default: null +* Type: null or Boolean Enable running a command in the context of **all** the configured workspaces. +Explicitly setting this to false will cause commands like \`install\` to +ignore workspaces altogether. + This value is not exported to the environment for child processes. #### \`yes\` diff --git a/test/lib/repo.js b/test/lib/repo.js index e1ac90b1e7577..41bff27447f1d 100644 --- a/test/lib/repo.js +++ b/test/lib/repo.js @@ -1,5 +1,5 @@ const t = require('tap') -const { fake: mockNpm } = require('../fixtures/mock-npm.js') +const { real: mockNpm } = require('../fixtures/mock-npm.js') const { join, sep } = require('path') const pkgDirs = t.testdir({ @@ -154,6 +154,7 @@ const pkgDirs = t.testdir({ name: 'workspaces-test', version: '1.2.3-test', workspaces: ['workspace-a', 'workspace-b', 'workspace-c'], + repository: 'https://github.com/npm/workspaces-test', }), 'workspace-a': { 'package.json': JSON.stringify({ @@ -185,19 +186,17 @@ const openUrl = async (npm, url, errMsg) => { opened[url]++ } -const Repo = t.mock('../../lib/repo.js', { +const { command, npm } = mockNpm(t, { '../../lib/utils/open-url.js': openUrl, }) -const flatOptions = {} -const npm = mockNpm({ flatOptions }) -const repo = new Repo(npm) +t.before(async () => { + await npm.load() +}) t.afterEach(() => opened = {}) t.test('open repo urls', t => { - // XXX It is very odd that `where` is how pacote knows to look anywhere other - // than the cwd. I would think npm.localPrefix would factor in somehow - flatOptions.where = pkgDirs + npm.localPrefix = pkgDirs const expect = { hostedgit: 'https://github.com/foo/hostedgit', hostedgitat: 'https://github.com/foo/hostedgitat', @@ -227,22 +226,19 @@ t.test('open repo urls', t => { const keys = Object.keys(expect) t.plan(keys.length) keys.forEach(pkg => { - t.test(pkg, t => { - repo.exec([['.', pkg].join(sep)], (err) => { - if (err) - throw err - const url = expect[pkg] - t.match({ - [url]: 1, - }, opened, `opened ${url}`, {opened}) - t.end() - }) + t.test(pkg, async t => { + await command('repo', [['.', pkg].join(sep)]) + const url = expect[pkg] + t.match({ + [url]: 1, + }, opened, `opened ${url}`, {opened}) + t.end() }) }) }) t.test('fail if cannot figure out repo url', t => { - flatOptions.where = pkgDirs + npm.localPrefix = pkgDirs const cases = [ 'norepo', 'repoobbj-nourl', @@ -253,57 +249,65 @@ t.test('fail if cannot figure out repo url', t => { t.plan(cases.length) cases.forEach(pkg => { - t.test(pkg, t => { - repo.exec([['.', pkg].join(sep)], (err) => { - t.match(err, { pkgid: pkg }) - t.end() - }) + t.test(pkg, async t => { + t.rejects( + command('repo', [['.', pkg].join(sep)]), + { pkgid: pkg } + ) }) }) }) -t.test('open default package if none specified', t => { - flatOptions.where = pkgDirs - repo.exec([], (er) => { - if (er) - throw er - t.equal(opened['https://example.com/thispkg'], 1, 'opened expected url', {opened}) - t.end() - }) +t.test('open default package if none specified', async t => { + npm.localPrefix = pkgDirs + await command('repo', []) + t.equal(opened['https://example.com/thispkg'], 1, 'opened expected url', {opened}) }) t.test('workspaces', t => { - flatOptions.where = undefined npm.localPrefix = join(pkgDirs, 'workspaces') - t.test('all workspaces', (t) => { - repo.execWorkspaces([], [], (err) => { - t.notOk(err) - t.match({ - 'https://repo.workspace-a/': 1, // Gets translated to https! - 'https://github.com/npm/workspace-b': 1, - }, opened, 'opened two valid repo urls') - t.end() - }) + t.afterEach(() => { + npm.config.set('workspaces', null) + npm.config.set('workspace', []) + npm.config.set('include-workspace-root', false) }) - t.test('one workspace', (t) => { - repo.execWorkspaces([], ['workspace-a'], (err) => { - t.notOk(err) - t.match({ - 'https://repo.workspace-a/': 1, - }, opened, 'opened one requested repo urls') - t.end() - }) + t.test('include workspace root', async (t) => { + npm.config.set('workspaces', true) + npm.config.set('include-workspace-root', true) + await command('repo', []) + t.match({ + 'https://github.com/npm/workspaces-test': 1, + 'https://repo.workspace-a/': 1, // Gets translated to https! + 'https://github.com/npm/workspace-b': 1, + }, opened, 'opened two valid repo urls') }) - t.test('invalid workspace', (t) => { - repo.execWorkspaces([], ['workspace-x'], (err) => { - t.match(err, /No workspaces found/) - t.match(err, /workspace-x/) - t.match({}, opened, 'opened no repo urls') - t.end() - }) + t.test('all workspaces', async (t) => { + npm.config.set('workspaces', true) + await command('repo', []) + t.match({ + 'https://repo.workspace-a/': 1, // Gets translated to https! + 'https://github.com/npm/workspace-b': 1, + }, opened, 'opened two valid repo urls') + }) + + t.test('one workspace', async (t) => { + npm.config.set('workspace', ['workspace-a']) + await command('repo', []) + t.match({ + 'https://repo.workspace-a/': 1, + }, opened, 'opened one requested repo urls') + }) + + t.test('invalid workspace', async (t) => { + npm.config.set('workspace', ['workspace-x']) + await t.rejects( + command('repo', []), + /workspace-x/ + ) + t.match({}, opened, 'opened no repo urls') }) t.end() }) diff --git a/test/lib/workspaces/get-workspaces.js b/test/lib/workspaces/get-workspaces.js index 4ea055e02f8f2..0f51d95fcb763 100644 --- a/test/lib/workspaces/get-workspaces.js +++ b/test/lib/workspaces/get-workspaces.js @@ -86,6 +86,17 @@ t.test('get-workspaces', async t => { 'should filter by package name' ) + workspaces = await getWorkspaces(['a', 'b'], { path, includeWorkspaceRoot: true }) + t.same( + clean(workspaces, path), + new Map(Object.entries({ + x: '{PATH}', + a: '{PATH}/packages/a', + b: '{PATH}/packages/b', + })), + 'include rootspace root' + ) + workspaces = await getWorkspaces(['./packages/c'], { path }) t.same( clean(workspaces, path),