From 0075f2852b09c636200fc54116068eb903e63a63 Mon Sep 17 00:00:00 2001 From: Gar Date: Wed, 4 May 2022 11:42:18 -0700 Subject: [PATCH] fix: consolidate bugs, docs, repo command logic All three of these commands do the same thing: open a manifest and find a url inside to open it. The finding of that manifest was not very consistent across these three commands. Some work with workspaces while others don't. Some work correctly with `--prefix` while others don't. This PR consolidates these commands so that they all are consistent in how they find the manifest being referenced. The specifics of which url they open are still left to each command. The util that only these three commands were using was consolidated into their base class. --- docs/content/commands/npm-bugs.md | 64 ++++++++++++++++++- docs/content/commands/npm-repo.md | 10 +++ lib/commands/bugs.js | 31 ++------- lib/commands/docs.js | 45 ++----------- lib/commands/repo.js | 45 ++----------- lib/package-url-cmd.js | 61 ++++++++++++++++++ lib/utils/hosted-git-info-from-manifest.js | 14 ---- .../test/lib/load-all-commands.js.test.cjs | 6 +- .../test/lib/utils/npm-usage.js.test.cjs | 6 +- test/lib/commands/docs.js | 13 ++-- .../utils/hosted-git-info-from-manifest.js | 21 ------ 11 files changed, 167 insertions(+), 149 deletions(-) create mode 100644 lib/package-url-cmd.js delete mode 100644 lib/utils/hosted-git-info-from-manifest.js delete mode 100644 test/lib/utils/hosted-git-info-from-manifest.js diff --git a/docs/content/commands/npm-bugs.md b/docs/content/commands/npm-bugs.md index aeddeb848e81b..6b45f1f18ac66 100644 --- a/docs/content/commands/npm-bugs.md +++ b/docs/content/commands/npm-bugs.md @@ -11,7 +11,7 @@ description: Report bugs for a package in a web browser ```bash -npm bugs [] +npm bugs [ [ ...]] alias: issues ``` @@ -58,6 +58,68 @@ The base URL of the npm registry. +#### `workspace` + +* Default: +* Type: String (can be set multiple times) + +Enable running a command in the context of the configured workspaces of the +current project while filtering by running only the workspaces defined by +this configuration option. + +Valid values for the `workspace` config are either: + +* Workspace names +* Path to a workspace directory +* 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 +brand new workspace within the project. + +This value is not exported to the environment for child processes. + + + + +#### `workspaces` + +* Default: null +* Type: null or Boolean + +Set to true to run the command in the context of **all** configured +workspaces. + +Explicitly setting this to false will cause commands like `install` to +ignore workspaces altogether. When not set explicitly: + +- Commands that operate on the `node_modules` tree (install, update, etc.) +will link workspaces into the `node_modules` folder. - Commands that do +other things (test, exec, publish, etc.) will operate on the root project, +_unless_ one or more workspaces are specified in the `workspace` config. + +This value is not exported to the environment for child processes. + + + + +#### `include-workspace-root` + +* Default: false +* Type: Boolean + +Include the workspace root when workspaces are enabled for a command. + +When false, specifying individual workspaces via the `workspace` config, or +all workspaces via the `workspaces` flag, will cause npm to operate only on +the specified workspaces, and not on the root project. + +This value is not exported to the environment for child processes. + + + + ### See Also diff --git a/docs/content/commands/npm-repo.md b/docs/content/commands/npm-repo.md index 404c724ff0290..fc540a9382b23 100644 --- a/docs/content/commands/npm-repo.md +++ b/docs/content/commands/npm-repo.md @@ -46,6 +46,16 @@ Set to `true` to use default system URL opener. +#### `registry` + +* Default: "https://registry.npmjs.org/" +* Type: URL + +The base URL of the npm registry. + + + + #### `workspace` * Default: diff --git a/lib/commands/bugs.js b/lib/commands/bugs.js index f6218f033f3d3..17cbd96649b87 100644 --- a/lib/commands/bugs.js +++ b/lib/commands/bugs.js @@ -1,33 +1,10 @@ -const pacote = require('pacote') -const log = require('../utils/log-shim') -const openUrl = require('../utils/open-url.js') -const hostedFromMani = require('../utils/hosted-git-info-from-manifest.js') -const BaseCommand = require('../base-command.js') +const PackageUrlCmd = require('../package-url-cmd.js') -class Bugs extends BaseCommand { +class Bugs extends PackageUrlCmd { static description = 'Report bugs for a package in a web browser' static name = 'bugs' - static usage = ['[]'] - static params = ['browser', 'registry'] - static ignoreImplicitWorkspace = true - async exec (args) { - if (!args || !args.length) { - args = ['.'] - } - - await Promise.all(args.map(pkg => this.getBugs(pkg))) - } - - async getBugs (pkg) { - const opts = { ...this.npm.flatOptions, fullMetadata: true } - const mani = await pacote.manifest(pkg, opts) - const url = this.getBugsUrl(mani) - log.silly('bugs', 'url', url) - await openUrl(this.npm, url, `${mani.name} bug list available at the following URL`) - } - - getBugsUrl (mani) { + getUrl (spec, mani) { if (mani.bugs) { if (typeof mani.bugs === 'string') { return mani.bugs @@ -43,7 +20,7 @@ class Bugs extends BaseCommand { } // try to get it from the repo, if possible - const info = hostedFromMani(mani) + const info = this.hostedFromMani(mani) if (info) { return info.bugs() } diff --git a/lib/commands/docs.js b/lib/commands/docs.js index 631615acc56b3..5d20215b56a07 100644 --- a/lib/commands/docs.js +++ b/lib/commands/docs.js @@ -1,54 +1,19 @@ -const pacote = require('pacote') -const openUrl = require('../utils/open-url.js') -const hostedFromMani = require('../utils/hosted-git-info-from-manifest.js') -const log = require('../utils/log-shim') -const BaseCommand = require('../base-command.js') -class Docs extends BaseCommand { +const PackageUrlCmd = require('../package-url-cmd.js') +class Docs extends PackageUrlCmd { static description = 'Open documentation for a package in a web browser' static name = 'docs' - static params = [ - 'browser', - 'registry', - 'workspace', - 'workspaces', - 'include-workspace-root', - ] - static usage = ['[ [ ...]]'] - static ignoreImplicitWorkspace = false - - async exec (args) { - if (!args || !args.length) { - args = ['.'] - } - - await Promise.all(args.map(pkg => this.getDocs(pkg))) - } - - async execWorkspaces (args, filters) { - await this.setWorkspaces(filters) - return this.exec(this.workspacePaths) - } - - async getDocs (pkg) { - const opts = { ...this.npm.flatOptions, fullMetadata: true } - const mani = await pacote.manifest(pkg, opts) - const url = this.getDocsUrl(mani) - log.silly('docs', 'url', url) - await openUrl(this.npm, url, `${mani.name} docs available at the following URL`) - } - - getDocsUrl (mani) { + getUrl (spec, mani) { if (mani.homepage) { return mani.homepage } - const info = hostedFromMani(mani) + const info = this.hostedFromMani(mani) if (info) { return info.docs() } - return 'https://www.npmjs.com/package/' + mani.name + return `https://www.npmjs.com/package/${mani.name}` } } module.exports = Docs diff --git a/lib/commands/repo.js b/lib/commands/repo.js index b8dccc209ff87..b89b74c0bf1ba 100644 --- a/lib/commands/repo.js +++ b/lib/commands/repo.js @@ -1,40 +1,11 @@ -const pacote = require('pacote') const { URL } = require('url') -const log = require('../utils/log-shim') -const hostedFromMani = require('../utils/hosted-git-info-from-manifest.js') -const openUrl = require('../utils/open-url.js') -const BaseCommand = require('../base-command.js') -class Repo extends BaseCommand { +const PackageUrlCmd = require('../package-url-cmd.js') +class Repo extends PackageUrlCmd { static description = 'Open package repository page in the browser' static name = 'repo' - static params = ['browser', 'workspace', 'workspaces', 'include-workspace-root'] - static usage = ['[ [ ...]]'] - static ignoreImplicitWorkspace = false - - async exec (args) { - if (!args || !args.length) { - args = ['.'] - } - - await Promise.all(args.map(pkg => this.get(pkg))) - } - - async execWorkspaces (args, filters) { - await this.setWorkspaces(filters) - return this.exec(this.workspacePaths) - } - - async get (pkg) { - // 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) + getUrl (spec, mani) { const r = mani.repository const rurl = !r ? null : typeof r === 'string' ? r @@ -43,22 +14,20 @@ class Repo extends BaseCommand { if (!rurl) { throw Object.assign(new Error('no repository'), { - pkgid: pkg, + pkgid: spec, }) } - const info = hostedFromMani(mani) + const info = this.hostedFromMani(mani) const url = info ? info.browse(mani.repository.directory) : unknownHostedUrl(rurl) if (!url) { throw Object.assign(new Error('no repository: could not get url'), { - pkgid: pkg, + pkgid: spec, }) } - - log.silly('docs', 'url', url) - await openUrl(this.npm, url, `${mani.name} repo available at the following URL`) + return url } } module.exports = Repo diff --git a/lib/package-url-cmd.js b/lib/package-url-cmd.js new file mode 100644 index 0000000000000..b897272149d02 --- /dev/null +++ b/lib/package-url-cmd.js @@ -0,0 +1,61 @@ +// Base command for opening urls from a package manifest (bugs, docs, repo) + +const pacote = require('pacote') +const hostedGitInfo = require('hosted-git-info') + +const openUrl = require('./utils/open-url.js') +const log = require('./utils/log-shim') + +const BaseCommand = require('./base-command.js') +class PackageUrlCommand extends BaseCommand { + static ignoreImplicitWorkspace = false + static params = [ + 'browser', + 'registry', + 'workspace', + 'workspaces', + 'include-workspace-root', + ] + + static usage = ['[ [ ...]]'] + + async exec (args) { + if (!args || !args.length) { + args = ['.'] + } + + for (const arg of args) { + // 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(arg, opts) + const url = this.getUrl(arg, mani) + log.silly(this.name, 'url', url) + await openUrl(this.npm, url, `${mani.name} ${this.name} available at the following URL`) + } + } + + async execWorkspaces (args, filters) { + await this.setWorkspaces(filters) + return this.exec(this.workspacePaths) + } + + // given a manifest, try to get the hosted git info from it based on + // repository (if a string) or repository.url (if an object) returns null + // if it's not a valid repo, or not a known hosted repo + hostedFromMani (mani) { + const r = mani.repository + const rurl = !r ? null + : typeof r === 'string' ? r + : typeof r === 'object' && typeof r.url === 'string' ? r.url + : null + + // hgi returns undefined sometimes, but let's always return null here + return (rurl && hostedGitInfo.fromUrl(rurl.replace(/^git\+/, ''))) || null + } +} +module.exports = PackageUrlCommand diff --git a/lib/utils/hosted-git-info-from-manifest.js b/lib/utils/hosted-git-info-from-manifest.js deleted file mode 100644 index 9592b0b3a937b..0000000000000 --- a/lib/utils/hosted-git-info-from-manifest.js +++ /dev/null @@ -1,14 +0,0 @@ -// given a manifest, try to get the hosted git info from it based on -// repository (if a string) or repository.url (if an object) -// returns null if it's not a valid repo, or not a known hosted repo -const hostedGitInfo = require('hosted-git-info') -module.exports = mani => { - const r = mani.repository - const rurl = !r ? null - : typeof r === 'string' ? r - : typeof r === 'object' && typeof r.url === 'string' ? r.url - : null - - // hgi returns undefined sometimes, but let's always return null here - return (rurl && hostedGitInfo.fromUrl(rurl.replace(/^git\+/, ''))) || null -} diff --git a/tap-snapshots/test/lib/load-all-commands.js.test.cjs b/tap-snapshots/test/lib/load-all-commands.js.test.cjs index eccb06580d9f9..c7c188dd91a74 100644 --- a/tap-snapshots/test/lib/load-all-commands.js.test.cjs +++ b/tap-snapshots/test/lib/load-all-commands.js.test.cjs @@ -81,10 +81,12 @@ exports[`test/lib/load-all-commands.js TAP load each command bugs > must match s Report bugs for a package in a web browser Usage: -npm bugs [] +npm bugs [ [ ...]] Options: [--no-browser|--browser ] [--registry ] +[-w|--workspace [-w|--workspace ...]] +[-ws|--workspaces] [--include-workspace-root] alias: issues @@ -727,7 +729,7 @@ Usage: npm repo [ [ ...]] Options: -[--no-browser|--browser ] +[--no-browser|--browser ] [--registry ] [-w|--workspace [-w|--workspace ...]] [-ws|--workspaces] [--include-workspace-root] diff --git a/tap-snapshots/test/lib/utils/npm-usage.js.test.cjs b/tap-snapshots/test/lib/utils/npm-usage.js.test.cjs index d7ec4953d6ab3..8d83c53a191e0 100644 --- a/tap-snapshots/test/lib/utils/npm-usage.js.test.cjs +++ b/tap-snapshots/test/lib/utils/npm-usage.js.test.cjs @@ -223,10 +223,12 @@ All commands: bugs Report bugs for a package in a web browser Usage: - npm bugs [] + npm bugs [ [ ...]] Options: [--no-browser|--browser ] [--registry ] + [-w|--workspace [-w|--workspace ...]] + [-ws|--workspaces] [--include-workspace-root] alias: issues @@ -777,7 +779,7 @@ All commands: npm repo [ [ ...]] Options: - [--no-browser|--browser ] + [--no-browser|--browser ] [--registry ] [-w|--workspace [-w|--workspace ...]] [-ws|--workspaces] [--include-workspace-root] diff --git a/test/lib/commands/docs.js b/test/lib/commands/docs.js index a3b31bd70656d..b2a65786bf4d8 100644 --- a/test/lib/commands/docs.js +++ b/test/lib/commands/docs.js @@ -35,6 +35,13 @@ const pkgDirs = t.testdir({ repository: { url: 'https://github.com/foo/repoobj' }, }), }, + repourlobj: { + 'package.json': JSON.stringify({ + name: 'repourlobj', + version: '1.2.3', + repository: { url: { works: false } }, + }), + }, workspaces: { 'package.json': JSON.stringify({ name: 'workspaces-test', @@ -81,14 +88,13 @@ const docs = new Docs(npm) t.afterEach(() => opened = {}) t.test('open docs 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 = { nodocs: 'https://www.npmjs.com/package/nodocs', docsurl: 'https://bugzilla.localhost/docsurl', repourl: 'https://github.com/foo/repourl#readme', repoobj: 'https://github.com/foo/repoobj#readme', + repourlobj: 'https://www.npmjs.com/package/repourlobj', '.': 'https://example.com', } const keys = Object.keys(expect) @@ -110,7 +116,6 @@ t.test('open default package if none specified', async t => { }) t.test('workspaces', (t) => { - flatOptions.where = undefined npm.localPrefix = join(pkgDirs, 'workspaces') t.test('all workspaces', async t => { await docs.execWorkspaces([], []) diff --git a/test/lib/utils/hosted-git-info-from-manifest.js b/test/lib/utils/hosted-git-info-from-manifest.js deleted file mode 100644 index 516d3d5867acb..0000000000000 --- a/test/lib/utils/hosted-git-info-from-manifest.js +++ /dev/null @@ -1,21 +0,0 @@ -const t = require('tap') -const hostedFromMani = require('../../../lib/utils/hosted-git-info-from-manifest.js') -const hostedGitInfo = require('hosted-git-info') - -t.equal(hostedFromMani({}), null) -t.equal(hostedFromMani({ repository: { no: 'url' } }), null) -t.equal(hostedFromMani({ repository: 123 }), null) -t.equal(hostedFromMani({ repository: 'not hosted anywhere' }), null) -t.equal(hostedFromMani({ repository: { url: 'not hosted anywhere' } }), null) - -t.match(hostedFromMani({ - repository: 'git+https://github.com/isaacs/abbrev-js', -}), hostedGitInfo.fromUrl('git+https://github.com/isaacs/abbrev-js')) - -t.match(hostedFromMani({ - repository: { url: 'git+https://github.com/isaacs/abbrev-js' }, -}), hostedGitInfo.fromUrl('https://github.com/isaacs/abbrev-js')) - -t.match(hostedFromMani({ - repository: { url: 'git+ssh://git@github.com/isaacs/abbrev-js' }, -}), hostedGitInfo.fromUrl('ssh://git@github.com/isaacs/abbrev-js'))