fix: remove lib/utils/read-package-name.js

This code wasn't doing anything special, just dereferencing `name` from a packument. There is no need for this to exist. Most of the tests were able to handle having this go away, except for `npm owner` which had to have its tests rewritten to be real, which of course surfaced bugs along the way of behavior that was incorrectly being tested. `npm owner` needs some love to clean up its UX, it throws or returns inconsistently. I did fix it so that if there is no package.json in cwd it errored as expected instead of throwing `ENOENT` which is what it did before.
npm · Apr 19, 2022 · 8da28b4 · 8da28b4
1 parent b06e89f
commit 8da28b4
Show file tree

Hide file tree

Showing 8 changed files with 426 additions and 722 deletions.
diff --git a/lib/commands/diff.js b/lib/commands/diff.js
@@ -6,7 +6,7 @@ const Arborist = require('@npmcli/arborist')
 const pacote = require('pacote')
 const pickManifest = require('npm-pick-manifest')
 const log = require('../utils/log-shim')
-const readPackageName = require('../utils/read-package-name.js')
+const readPackage = require('read-package-json-fast')
 const BaseCommand = require('../base-command.js')
 
 class Diff extends BaseCommand {
@@ -81,7 +81,8 @@ class Diff extends BaseCommand {
   async packageName (path) {
     let name
     try {
-      name = await readPackageName(this.prefix)
+      const pkg = await readPackage(resolve(this.prefix, 'package.json'))
+      name = pkg.name
     } catch (e) {
       log.verbose('diff', 'could not read project dir package.json')
     }
@@ -114,7 +115,8 @@ class Diff extends BaseCommand {
     let noPackageJson
     let pkgName
     try {
-      pkgName = await readPackageName(this.prefix)
+      const pkg = await readPackage(resolve(this.prefix, 'package.json'))
+      pkgName = pkg.name
     } catch (e) {
       log.verbose('diff', 'could not read project dir package.json')
       noPackageJson = true
@@ -225,7 +227,8 @@ class Diff extends BaseCommand {
     if (semverA && semverB) {
       let pkgName
       try {
-        pkgName = await readPackageName(this.prefix)
+        const pkg = await readPackage(resolve(this.prefix, 'package.json'))
+        pkgName = pkg.name
       } catch (e) {
         log.verbose('diff', 'could not read project dir package.json')
       }

diff --git a/lib/commands/dist-tag.js b/lib/commands/dist-tag.js
@@ -1,9 +1,10 @@
 const npa = require('npm-package-arg')
+const path = require('path')
 const regFetch = require('npm-registry-fetch')
 const semver = require('semver')
 const log = require('../utils/log-shim')
 const otplease = require('../utils/otplease.js')
-const readPackageName = require('../utils/read-package-name.js')
+const readPackage = require('read-package-json-fast')
 const BaseCommand = require('../base-command.js')
 
 class DistTag extends BaseCommand {
@@ -150,12 +151,12 @@ class DistTag extends BaseCommand {
       if (this.npm.config.get('global')) {
         throw this.usageError()
       }
-      const pkg = await readPackageName(this.npm.prefix)
-      if (!pkg) {
+      const { name } = await readPackage(path.resolve(this.npm.prefix, 'package.json'))
+      if (!name) {
         throw this.usageError()
       }
 
-      return this.list(pkg, opts)
+      return this.list(name, opts)
     }
     spec = npa(spec)
 

diff --git a/lib/commands/owner.js b/lib/commands/owner.js
@@ -3,8 +3,18 @@ const npmFetch = require('npm-registry-fetch')
 const pacote = require('pacote')
 const log = require('../utils/log-shim')
 const otplease = require('../utils/otplease.js')
-const readLocalPkgName = require('../utils/read-package-name.js')
+const readPackageJsonFast = require('read-package-json-fast')
 const BaseCommand = require('../base-command.js')
+const { resolve } = require('path')
+
+const readJson = async (pkg) => {
+  try {
+    const json = await readPackageJsonFast(pkg)
+    return json
+  } catch {
+    return {}
+  }
+}
 
 class Owner extends BaseCommand {
   static description = 'Manage package owners'
@@ -41,12 +51,12 @@ class Owner extends BaseCommand {
       if (this.npm.config.get('global')) {
         return []
       }
-      const pkgName = await readLocalPkgName(this.npm.prefix)
-      if (!pkgName) {
+      const { name } = await readJson(resolve(this.npm.prefix, 'package.json'))
+      if (!name) {
         return []
       }
 
-      const spec = npa(pkgName)
+      const spec = npa(name)
       const data = await pacote.packument(spec, {
         ...this.npm.flatOptions,
         fullMetadata: true,
@@ -96,12 +106,12 @@ class Owner extends BaseCommand {
       if (this.npm.config.get('global')) {
         throw this.usageError()
       }
-      const pkgName = await readLocalPkgName(this.npm.prefix)
-      if (!pkgName) {
+      const { name } = await readJson(resolve(this.npm.prefix, 'package.json'))
+      if (!name) {
         throw this.usageError()
       }
 
-      return pkgName
+      return name
     }
     return pkg
   }
@@ -125,15 +135,6 @@ class Owner extends BaseCommand {
       throw err
     }
 
-    if (!u || !u.name || u.error) {
-      throw Object.assign(
-        new Error(
-          "Couldn't get user data for " + user + ': ' + JSON.stringify(u)
-        ),
-        { code: 'EOWNERUSER' }
-      )
-    }
-
     // normalize user data
     u = { name: u.name, email: u.email }
 
@@ -177,32 +178,31 @@ class Owner extends BaseCommand {
     }
 
     const dataPath = `/${spec.escapedName}/-rev/${encodeURIComponent(data._rev)}`
-    const res = await otplease(this.npm.flatOptions, opts => {
-      return npmFetch.json(dataPath, {
-        ...opts,
-        method: 'PUT',
-        body: {
-          _id: data._id,
-          _rev: data._rev,
-          maintainers,
-        },
-        spec,
+    try {
+      const res = await otplease(this.npm.flatOptions, opts => {
+        return npmFetch.json(dataPath, {
+          ...opts,
+          method: 'PUT',
+          body: {
+            _id: data._id,
+            _rev: data._rev,
+            maintainers,
+          },
+          spec,
+        })
       })
-    })
-
-    if (!res.error) {
       if (addOrRm === 'add') {
         this.npm.output(`+ ${user} (${spec.name})`)
       } else {
         this.npm.output(`- ${user} (${spec.name})`)
       }
-    } else {
+      return res
+    } catch (err) {
       throw Object.assign(
-        new Error('Failed to update package: ' + JSON.stringify(res)),
+        new Error('Failed to update package: ' + JSON.stringify(err.message)),
         { code: 'EOWNERMUTATE' }
       )
     }
-    return res
   }
 }
 

diff --git a/tap-snapshots/test/lib/commands/owner.js.test.cjs b/tap-snapshots/test/lib/commands/owner.js.test.cjs
diff --git a/test/fixtures/mock-registry.js b/test/fixtures/mock-registry.js
@@ -5,6 +5,7 @@
  * for tests against any registry data.
  */
 const pacote = require('pacote')
+const npa = require('npm-package-arg')
 class MockRegistry {
   #tap
   #nock
@@ -84,6 +85,16 @@ class MockRegistry {
     ).reply(200)
   }
 
+  couchuser ({ username, body, responseCode = 200 }) {
+    if (body) {
+      this.nock = this.nock.get(`/-/user/org.couchdb.user:${encodeURIComponent(username)}`)
+        .reply(responseCode, body)
+    } else {
+      this.nock = this.nock.get(`/-/user/org.couchdb.user:${encodeURIComponent(username)}`)
+        .reply(responseCode, { _id: `org.couchdb.user:${username}`, email: '', name: username })
+    }
+  }
+
   couchlogin ({ username, password, email, otp, token = 'npm_default-test-token' }) {
     this.nock = this.nock
       .post('/-/v1/login').reply(401, { error: 'You must be logged in to publish packages.' })
@@ -156,7 +167,8 @@ class MockRegistry {
 
   async package ({ manifest, times = 1, query, tarballs }) {
     let nock = this.nock
-    nock = nock.get(`/${manifest.name}`).times(times)
+    const spec = npa(manifest.name)
+    nock = nock.get(`/${spec.escapedName}`).times(times)
     if (query) {
       nock = nock.query(query)
     }
@@ -203,6 +215,7 @@ class MockRegistry {
         dist: {
           tarball: `${this.#registry}/${name}/-/${name}-${packument.version}.tgz`,
         },
+        maintainers: [],
         ...packument,
       }
       manifest.time[packument.version] = new Date()

diff --git a/test/lib/commands/diff.js b/test/lib/commands/diff.js
@@ -417,7 +417,6 @@ t.test('single arg', t => {
 
     const Diff = t.mock('../../../lib/commands/diff.js', {
       ...mocks,
-      '../../../lib/utils/read-package-name.js': async () => 'my-project',
       pacote: {
         packument: spec => {
           t.equal(spec.name, 'lorem', 'should have expected spec name')
@@ -455,7 +454,6 @@ t.test('single arg', t => {
 
     const Diff = t.mock('../../../lib/commands/diff.js', {
       ...mocks,
-      '../../../lib/utils/read-package-name.js': async () => 'my-project',
       '@npmcli/arborist': class {
         constructor () {
           throw new Error('ERR')