fix(log): pass in logger to external modules

Most of these module use npm-registry-fetch under the hood, which will
log things like the `npm-notice` header if seen.  Currently we aren't
passing in a logger to them, which means that log message is never seen,
among any other logged messages those modules may need to make.

I added tests where I could. Some tests were in a state where the entire
libnpm* module was an empty mocked function, so asserting that it got
passed a `log` attribute was onerous.

lib/commands/access.js

@@ -3,6 +3,7 @@ const path = require('path')
 const libaccess = require('libnpmaccess')
 const readPackageJson = require('read-package-json-fast')
 
+const log = require('../utils/log-shim.js')
 const otplease = require('../utils/otplease.js')
 const getIdentity = require('../utils/get-identity.js')
 const BaseCommand = require('../base-command.js')
@@ -76,7 +77,10 @@ class Access extends BaseCommand {
       throw this.usageError(`${cmd} is not a recognized subcommand.`)
     }
 
-    return this[cmd](args, this.npm.flatOptions)
+    return this[cmd](args, {
+      ...this.npm.flatOptions,
+      log,
+    })
   }
 
   public ([pkg], opts) {

lib/commands/deprecate.js

@@ -1,4 +1,5 @@
 const fetch = require('npm-registry-fetch')
+const log = require('../utils/log-shim.js')
 const otplease = require('../utils/otplease.js')
 const npa = require('npm-package-arg')
 const semver = require('semver')
@@ -50,6 +51,7 @@ class Deprecate extends BaseCommand {
       ...this.npm.flatOptions,
       spec: p,
       query: { write: true },
+      log,
     })
 
     Object.keys(packument.versions)
@@ -64,6 +66,7 @@ class Deprecate extends BaseCommand {
       method: 'PUT',
       body: packument,
       ignoreBody: true,
+      log,
     }))
   }
 }

lib/commands/diff.js

@@ -61,6 +61,7 @@ class Diff extends BaseCommand {
       ...this.npm.flatOptions,
       diffFiles: args,
       where: this.top,
+      log,
     })
     return this.npm.output(res)
   }
@@ -193,6 +194,7 @@ class Diff extends BaseCommand {
         const packument = await pacote.packument(spec, {
           ...this.npm.flatOptions,
           preferOnline: true,
+          log,
         })
         bSpec = pickManifest(
           packument,

lib/commands/dist-tag.js

@@ -29,7 +29,10 @@ class DistTag extends BaseCommand {
   }
 
   async exec ([cmdName, pkg, tag]) {
-    const opts = this.npm.flatOptions
+    const opts = {
+      ...this.npm.flatOptions,
+      log,
+    }
 
     if (['add', 'a', 'set', 's'].includes(cmdName)) {
       return this.add(pkg, tag, opts)

lib/commands/hook.js

@@ -2,6 +2,7 @@ const hookApi = require('libnpmhook')
 const otplease = require('../utils/otplease.js')
 const relativeDate = require('tiny-relative-date')
 const Table = require('cli-table3')
+const log = require('../utils/log-shim.js')
 
 const BaseCommand = require('../base-command.js')
 class Hook extends BaseCommand {
@@ -20,7 +21,10 @@ class Hook extends BaseCommand {
   ]
 
   async exec (args) {
-    return otplease(this.npm.flatOptions, (opts) => {
+    return otplease({
+      ...this.npm.flatOptions,
+      log,
+    }, (opts) => {
       switch (args[0]) {
         case 'add':
           return this.add(args[1], args[2], args[3], opts)

lib/commands/logout.js

@@ -25,6 +25,7 @@ class Logout extends BaseCommand {
         ...this.npm.flatOptions,
         method: 'DELETE',
         ignoreBody: true,
+        log,
       })
     } else if (auth.isBasicAuth) {
       log.verbose('logout', `clearing user credentials for ${reg}`)

lib/commands/owner.js

@@ -57,7 +57,10 @@ class Owner extends BaseCommand {
   }
 
   async exec ([action, ...args]) {
-    const opts = this.npm.flatOptions
+    const opts = {
+      ...this.npm.flatOptions,
+      log,
+    }
     switch (action) {
       case 'ls':
       case 'list':
@@ -195,6 +198,7 @@ class Owner extends BaseCommand {
         method: 'PUT',
         body,
         spec,
+        log,
       })
     })
 

lib/commands/ping.js

@@ -10,7 +10,7 @@ class Ping extends BaseCommand {
   async exec (args) {
     log.notice('PING', this.npm.config.get('registry'))
     const start = Date.now()
-    const details = await pingUtil(this.npm.flatOptions)
+    const details = await pingUtil({ ...this.npm.flatOptions, log })
     const time = Date.now() - start
     log.notice('PONG', `${time}ms`)
     if (this.npm.config.get('json')) {

lib/commands/profile.js

@@ -108,7 +108,7 @@ class Profile extends BaseCommand {
   async get (args) {
     const tfa = 'two-factor auth'
     const info = await pulseTillDone.withPromise(
-      npmProfile.get(this.npm.flatOptions)
+      npmProfile.get({ ...this.npm.flatOptions, log })
     )
 
     if (!info.cidr_whitelist) {
@@ -170,7 +170,7 @@ class Profile extends BaseCommand {
   }
 
   async set (args) {
-    const conf = this.npm.flatOptions
+    const conf = { ...this.npm.flatOptions, log }
     const prop = (args[0] || '').toLowerCase().trim()
 
     let value = args.length > 1 ? args.slice(1).join(' ') : null
@@ -285,7 +285,7 @@ class Profile extends BaseCommand {
     if (auth.basic) {
       log.info('profile', 'Updating authentication to bearer token')
       const result = await npmProfile.createToken(
-        auth.basic.password, false, [], this.npm.flatOptions
+        auth.basic.password, false, [], { ...this.npm.flatOptions, log }
       )
 
       if (!result.token) {
@@ -309,7 +309,7 @@ class Profile extends BaseCommand {
 
     log.info('profile', 'Determine if tfa is pending')
     const userInfo = await pulseTillDone.withPromise(
-      npmProfile.get(this.npm.flatOptions)
+      npmProfile.get({ ...this.npm.flatOptions, log })
     )
 
     const conf = { ...this.npm.flatOptions }

lib/commands/publish.js

@@ -61,7 +61,7 @@ class Publish extends BaseCommand {
       throw new Error('Tag name must not be a valid SemVer range: ' + defaultTag.trim())
     }
 
-    const opts = { ...this.npm.flatOptions }
+    const opts = { ...this.npm.flatOptions, log }
 
     // you can publish name@version, ./foo.tgz, etc.
     // even though the default is the 'file:.' cwd.

lib/commands/star.js

@@ -29,12 +29,13 @@ class Star extends BaseCommand {
     const pkgs = args.map(npa)
     for (const pkg of pkgs) {
       const [username, fullData] = await Promise.all([
-        getIdentity(this.npm, this.npm.flatOptions),
+        getIdentity(this.npm, { ...this.npm.flatOptions, log }),
         fetch.json(pkg.escapedName, {
           ...this.npm.flatOptions,
           spec: pkg,
           query: { write: true },
           preferOnline: true,
+          log,
         }),
       ])
 
@@ -63,6 +64,7 @@ class Star extends BaseCommand {
         spec: pkg,
         method: 'PUT',
         body,
+        log,
       })
 
       this.npm.output(show + ' ' + pkg.name)

lib/commands/team.js

@@ -1,6 +1,7 @@
 const columns = require('cli-columns')
 const libteam = require('libnpmteam')
 
+const log = require('../utils/log-shim.js')
 const otplease = require('../utils/otplease.js')
 
 const BaseCommand = require('../base-command.js')
@@ -42,7 +43,7 @@ class Team extends BaseCommand {
     // XXX: "description" option to libnpmteam is used as a description of the
     // team, but in npm's options, this is a boolean meaning "show the
     // description in npm search output".  Hence its being set to null here.
-    await otplease(this.npm.flatOptions, opts => {
+    await otplease({ ...this.npm.flatOptions, log }, opts => {
       entity = entity.replace(/^@/, '')
       switch (cmd) {
         case 'create': return this.create(entity, opts)

lib/commands/token.js

@@ -168,7 +168,7 @@ class Token extends BaseCommand {
   }
 
   config () {
-    const conf = { ...this.npm.flatOptions }
+    const conf = { ...this.npm.flatOptions, log }
     const creds = this.npm.config.getCredentialsByURI(conf.registry)
     if (creds.token) {
       conf.auth = { token: creds.token }

lib/commands/unpublish.js

@@ -32,7 +32,7 @@ class Unpublish extends BaseCommand {
       return []
     }
 
-    const opts = this.npm.flatOptions
+    const opts = { ...this.npm.flatOptions, log }
     const username = await getIdentity(this.npm, { ...opts }).catch(() => null)
     if (!username) {
       return []

lib/commands/whoami.js

@@ -1,4 +1,5 @@
 const getIdentity = require('../utils/get-identity.js')
+const log = require('../utils/log-shim.js')
 
 const BaseCommand = require('../base-command.js')
 class Whoami extends BaseCommand {
@@ -7,7 +8,7 @@ class Whoami extends BaseCommand {
   static params = ['registry']
 
   async exec (args) {
-    const username = await getIdentity(this.npm, this.npm.flatOptions)
+    const username = await getIdentity(this.npm, { ...this.npm.flatOptions, log })
     this.npm.output(
       this.npm.config.get('json') ? JSON.stringify(username) : username
     )

test/lib/commands/access.js

@@ -75,12 +75,13 @@ t.test('access public on unscoped package', async t => {
 })
 
 t.test('access public on scoped package', async t => {
-  t.plan(2)
+  t.plan(3)
   const name = '@scoped/npm-access-public-pkg'
   const { npm } = await loadMockNpm(t, {
     mocks: {
       libnpmaccess: {
-        public: (pkg, { registry }) => {
+        public: (pkg, { registry, log }) => {
+          t.ok(log, 'should pass a logger')
           t.equal(pkg, name, 'should use pkg name ref')
           t.equal(
             registry,

test/lib/commands/deprecate.js

@@ -2,12 +2,15 @@ const t = require('tap')
 
 let getIdentityImpl = () => 'someperson'
 let npmFetchBody = null
+let npmFetchLog = null
 
 const npmFetch = async (uri, opts) => {
   npmFetchBody = opts.body
+  npmFetchLog = opts.log
 }
 
 npmFetch.json = async (uri, opts) => {
+  npmFetchLog = opts.log
   return {
     versions: {
       '1.0.0': {},
@@ -82,7 +85,12 @@ t.test('invalid semver range', async t => {
 })
 
 t.test('undeprecate', async t => {
+  t.teardown(() => {
+    npmFetchBody = null
+    npmFetchLog = null
+  })
   await deprecate.exec(['foo', ''])
+  t.ok(npmFetchLog, 'was passed a logger')
   t.match(npmFetchBody, {
     versions: {
       '1.0.0': { deprecated: '' },
@@ -95,9 +103,11 @@ t.test('undeprecate', async t => {
 t.test('deprecates given range', async t => {
   t.teardown(() => {
     npmFetchBody = null
+    npmFetchLog = null
   })
 
   await deprecate.exec(['foo@1.0.0', 'this version is deprecated'])
+  t.ok(npmFetchLog, 'was passed a logger')
   t.match(npmFetchBody, {
     versions: {
       '1.0.0': {

test/lib/commands/diff.js

@@ -61,9 +61,10 @@ const diff = new Diff(npm)
 
 t.test('no args', t => {
   t.test('in a project dir', async t => {
-    t.plan(3)
+    t.plan(4)
 
     libnpmdiff = async ([a, b], opts) => {
+      t.ok(opts.log, 'should be passed a logger')
       t.equal(a, 'foo@latest', 'should have default spec comparison')
       t.equal(b, `file:${fooPath}`, 'should compare to cwd')
       t.match(opts, npm.flatOptions, 'should forward flat options')

test/lib/commands/dist-tag.js

@@ -43,14 +43,19 @@ const routeMap = {
 // XXX overriding this does not appear to do anything, adding t.plan to things
 // that use it fails the test
 let npmRegistryFetchMock = (url, opts) => {
+  npmRegistryFetchLog = opts.log
   if (url === '/-/package/foo/dist-tags') {
     throw new Error('no package found')
   }
 
   return routeMap[url]
 }
 
-npmRegistryFetchMock.json = async (url, opts) => routeMap[url]
+let npmRegistryFetchLog
+npmRegistryFetchMock.json = async (url, opts) => {
+  npmRegistryFetchLog = opts.log
+  return routeMap[url]
+}
 
 const logger = (...msgs) => {
   for (const msg of [...msgs]) {
@@ -81,13 +86,18 @@ const npm = mockNpm({
 })
 const distTag = new DistTag(npm)
 
+t.afterEach(() => {
+  npmRegistryFetchLog = null
+})
+
 t.test('ls in current package', async t => {
   npm.prefix = t.testdir({
     'package.json': JSON.stringify({
       name: '@scoped/pkg',
     }),
   })
   await distTag.exec(['ls'])
+  t.ok(npmRegistryFetchLog, 'is passed a logger')
   t.matchSnapshot(
     result,
     'should list available tags for current package'
@@ -289,6 +299,7 @@ t.test('add new tag', async t => {
   })
 
   npmRegistryFetchMock = async (url, opts) => {
+    t.ok(opts.log, 'is passed a logger')
     t.equal(opts.method, 'PUT', 'should trigger request to add new tag')
     t.equal(opts.body, '7.7.7', 'should point to expected version')
   }
@@ -355,6 +366,7 @@ t.test('remove existing tag', async t => {
   }
   npm.prefix = t.testdir({})
   await distTag.exec(['rm', '@scoped/another', 'c'])
+  t.ok(npmRegistryFetchLog, 'is passed a logger')
   t.matchSnapshot(log, 'should log remove info')
   t.matchSnapshot(result, 'should return success msg')
 })