From 726b61caf9b588c3c070f47f3166e49663f65fb9 Mon Sep 17 00:00:00 2001 From: Gar Date: Tue, 27 Jul 2021 15:38:07 -0700 Subject: [PATCH] feat(workspaces): --include-workspace-root Adds a new config item that includes the workspace root when running non-arborist commands (i.e. repo, version, publish). Arborist will need to be udpated to look for this flag to change its behavior to include the workspace root for its functions. This also changes --workspaces to a trinary, so that setting it to false will explicitly exclude workspaces altogether. This is also going to require an arborist change so that it ignores workspaces altogether. --- lib/base-command.js | 10 +- lib/config.js | 2 +- lib/npm.js | 9 ++ lib/repo.js | 8 +- lib/utils/config/definitions.js | 20 ++- lib/workspaces/arborist-cmd.js | 7 +- lib/workspaces/get-workspaces.js | 9 +- .../lib/utils/config/definitions.js.test.cjs | 21 +++- .../lib/utils/config/describe-all.js.test.cjs | 18 ++- test/lib/repo.js | 118 +++++++++--------- test/lib/workspaces/get-workspaces.js | 11 ++ 11 files changed, 156 insertions(+), 77 deletions(-) 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),