fix: use @npmcli/package-json (#356)

Author: lukekarrys

lib/dir.js

@@ -87,7 +87,7 @@ class DirFetcher extends Fetcher {
       return Promise.resolve(this.package)
     }
 
-    return this[_readPackageJson](this.resolved + '/package.json')
+    return this[_readPackageJson](this.resolved)
       .then(mani => this.package = {
         ...mani,
         _integrity: this.integrity && String(this.integrity),

lib/fetcher.js

@@ -5,7 +5,6 @@
 
 const npa = require('npm-package-arg')
 const ssri = require('ssri')
-const { promisify } = require('util')
 const { basename, dirname } = require('path')
 const tar = require('tar')
 const { log } = require('proc-log')
@@ -16,12 +15,14 @@ const cacache = require('cacache')
 const isPackageBin = require('./util/is-package-bin.js')
 const removeTrailingSlashes = require('./util/trailing-slashes.js')
 const getContents = require('@npmcli/installed-package-contents')
-const readPackageJsonFast = require('read-package-json-fast')
-const readPackageJson = promisify(require('read-package-json'))
+const PackageJson = require('@npmcli/package-json')
 const { Minipass } = require('minipass')
-
 const cacheDir = require('./util/cache-dir.js')
 
+// Pacote is only concerned with the package.json contents
+const packageJsonPrepare = (p) => PackageJson.prepare(p).then(pkg => pkg.content)
+const packageJsonNormalize = (p) => PackageJson.normalize(p).then(pkg => pkg.content)
+
 // Private methods.
 // Child classes should not have to override these.
 // Users should never call them.
@@ -93,9 +94,9 @@ class FetcherBase {
     this.fullMetadata = this.before ? true : !!opts.fullMetadata
     this.fullReadJson = !!opts.fullReadJson
     if (this.fullReadJson) {
-      this[_readPackageJson] = readPackageJson
+      this[_readPackageJson] = packageJsonPrepare
     } else {
-      this[_readPackageJson] = readPackageJsonFast
+      this[_readPackageJson] = packageJsonNormalize
     }
 
     // rrh is a registry hostname or 'never' or 'always'

lib/file.js

@@ -1,10 +1,11 @@
-const Fetcher = require('./fetcher.js')
 const fsm = require('fs-minipass')
 const cacache = require('cacache')
-const _tarballFromResolved = Symbol.for('pacote.Fetcher._tarballFromResolved')
-const _exeBins = Symbol('_exeBins')
 const { resolve } = require('path')
-const fs = require('fs')
+const { stat, chmod } = require('fs/promises')
+const Fetcher = require('./fetcher.js')
+
+const _exeBins = Symbol('_exeBins')
+const _tarballFromResolved = Symbol.for('pacote.Fetcher._tarballFromResolved')
 const _readPackageJson = Symbol.for('package.Fetcher._readPackageJson')
 
 class FileFetcher extends Fetcher {
@@ -26,7 +27,7 @@ class FileFetcher extends Fetcher {
     // have to unpack the tarball for this.
     return cacache.tmp.withTmp(this.cache, this.opts, dir =>
       this.extract(dir)
-        .then(() => this[_readPackageJson](dir + '/package.json'))
+        .then(() => this[_readPackageJson](dir))
         .then(mani => this.package = {
           ...mani,
           _integrity: this.integrity && String(this.integrity),
@@ -40,31 +41,31 @@ class FileFetcher extends Fetcher {
       return Promise.resolve()
     }
 
-    return Promise.all(Object.keys(pkg.bin).map(k => new Promise(res => {
+    return Promise.all(Object.keys(pkg.bin).map(async k => {
       const script = resolve(dest, pkg.bin[k])
       // Best effort.  Ignore errors here, the only result is that
       // a bin script is not executable.  But if it's missing or
       // something, we just leave it for a later stage to trip over
       // when we can provide a more useful contextual error.
-      fs.stat(script, (er, st) => {
-        if (er) {
-          return res()
-        }
+      try {
+        const st = await stat(script)
         const mode = st.mode | 0o111
         if (mode === st.mode) {
-          return res()
+          return
         }
-        fs.chmod(script, mode, res)
-      })
-    })))
+        await chmod(script, mode)
+      } catch {
+        // Ignore errors here
+      }
+    }))
   }
 
   extract (dest) {
     // if we've already loaded the manifest, then the super got it.
     // but if not, read the unpacked manifest and chmod properly.
     return super.extract(dest)
       .then(result => this.package ? result
-      : this[_readPackageJson](dest + '/package.json').then(pkg =>
+      : this[_readPackageJson](dest).then(pkg =>
         this[_exeBins](pkg, dest)).then(() => result))
   }
 

lib/git.js

@@ -156,11 +156,11 @@ class GitFetcher extends Fetcher {
   [_resolvedFromClone] () {
     // do a full or shallow clone, then look at the HEAD
     // kind of wasteful, but no other option, really
-    return this[_clone](dir => this.resolved)
+    return this[_clone](() => this.resolved)
   }
 
   [_prepareDir] (dir) {
-    return this[_readPackageJson](dir + '/package.json').then(mani => {
+    return this[_readPackageJson](dir).then(mani => {
       // no need if we aren't going to do any preparation.
       const scripts = mani.scripts
       if (!mani.workspaces && (!scripts || !(
@@ -312,7 +312,7 @@ class GitFetcher extends Fetcher {
     return this.spec.hosted && this.resolved
       ? FileFetcher.prototype.manifest.apply(this)
       : this[_clone](dir =>
-        this[_readPackageJson](dir + '/package.json')
+        this[_readPackageJson](dir)
           .then(mani => this.package = {
             ...mani,
             _resolved: this.resolved,

lib/registry.js

@@ -3,7 +3,7 @@ const RemoteFetcher = require('./remote.js')
 const _tarballFromResolved = Symbol.for('pacote.Fetcher._tarballFromResolved')
 const pacoteVersion = require('../package.json').version
 const removeTrailingSlashes = require('./util/trailing-slashes.js')
-const rpj = require('read-package-json-fast')
+const PackageJson = require('@npmcli/package-json')
 const pickManifest = require('npm-pick-manifest')
 const ssri = require('ssri')
 const crypto = require('crypto')
@@ -127,12 +127,12 @@ class RegistryFetcher extends Fetcher {
     }
 
     const packument = await this.packument()
-    let mani = await pickManifest(packument, this.spec.fetchSpec, {
+    const mani = await new PackageJson().fromContent(pickManifest(packument, this.spec.fetchSpec, {
       ...this.opts,
       defaultTag: this.defaultTag,
       before: this.before,
-    })
-    mani = rpj.normalize(mani)
+    })).normalize().then(p => p.content)
+
     /* XXX add ETARGET and E403 revalidation of cached packuments here */
 
     // add _time from packument if fetched with fullMetadata

package.json

@@ -46,6 +46,7 @@
   "dependencies": {
     "@npmcli/git": "^5.0.0",
     "@npmcli/installed-package-contents": "^2.0.1",
+    "@npmcli/package-json": "^5.1.0",
     "@npmcli/promise-spawn": "^7.0.0",
     "@npmcli/run-script": "^8.0.0",
     "cacache": "^18.0.0",
@@ -57,8 +58,6 @@
     "npm-registry-fetch": "^16.0.0",
     "proc-log": "^4.0.0",
     "promise-retry": "^2.0.1",
-    "read-package-json": "^7.0.0",
-    "read-package-json-fast": "^3.0.0",
     "sigstore": "^2.2.0",
     "ssri": "^10.0.0",
     "tar": "^6.1.11"

test/bin.js

@@ -23,7 +23,7 @@ pacote.manifest = (spec, conf) => Promise.resolve({
 pacote.packument = (spec, conf) => Promise.resolve({ method: 'packument', spec, conf })
 pacote.tarball.file = (spec, file, conf) => Promise.resolve({ method: 'tarball', spec, file, conf })
 const { Minipass } = require('minipass')
-pacote.tarball.stream = (spec, handler, conf) => handler(new Minipass().end('tarball data'))
+pacote.tarball.stream = (spec, handler) => handler(new Minipass().end('tarball data'))
 pacote.extract = (spec, dest, conf) => Promise.resolve({ method: 'extract', spec, dest, conf })
 
 t.test('running bin runs main file', t => {

test/fetcher.js

@@ -94,15 +94,14 @@ t.test('tarball data', async t => {
 
 t.test('tarballFile', t => {
   const target = resolve(me, 'tarball-file')
-  t.test('basic copy', t =>
-    new FileFetcher(abbrevspec, { cache })
-      .tarballFile(target + '/basic/1.tgz'))
 
-  t.test('again, folder already created', t =>
-    new FileFetcher(abbrevspec, { cache })
-      .tarballFile(target + '/basic/2.tgz'))
+  t.test('basic', async t => {
+    await new FileFetcher(abbrevspec, { cache })
+      .tarballFile(target + '/basic/1.tgz')
+
+    await new FileFetcher(abbrevspec, { cache })
+      .tarballFile(target + '/basic/2.tgz')
 
-  t.test('check it', t => {
     const one = fs.readFileSync(target + '/basic/1.tgz')
     const two = fs.readFileSync(target + '/basic/2.tgz')
     const expect = fs.readFileSync(abbrev)

test/git.js

@@ -87,7 +87,7 @@ const tar = require('tar')
 
 let REPO_HEAD = ''
 t.test('setup', { bail: true }, t => {
-  t.test('create repo', t => {
+  t.test('create repo', () => {
     const git = (...cmd) => spawnGit(cmd, { cwd: repo })
     const write = (f, c) => fs.writeFileSync(`${repo}/${f}`, c)
     const npm = (...cmd) => spawnNpm('npm', [
@@ -162,7 +162,7 @@ t.test('setup', { bail: true }, t => {
       .then(({ stdout }) => REPO_HEAD = stdout.trim())
   })
 
-  t.test('create cycle of git prepared deps', async t => {
+  t.test('create cycle of git prepared deps', async () => {
     for (const repoDir of [cycleA, cycleB]) {
       const git = (...cmd) => spawnGit(cmd, { cwd: repoDir })
       const write = (f, c) => fs.writeFileSync(`${repoDir}/${f}`, c)
@@ -227,7 +227,7 @@ t.test('setup', { bail: true }, t => {
     daemon.on('close', () => rmSync(me, { recursive: true, force: true }))
   })
 
-  t.test('create a repo with a submodule', t => {
+  t.test('create a repo with a submodule', () => {
     const submoduleRepo = resolve(me, 'submodule-repo')
     const git = (...cmd) => spawnGit(cmd, { cwd: submoduleRepo })
     const write = (f, c) => fs.writeFileSync(`${submoduleRepo}/${f}`, c)
@@ -285,7 +285,7 @@ t.test('setup', { bail: true }, t => {
       .then(() => git('commit', '-m', 'package'))
   })
 
-  t.test('create a repo with workspaces', t => {
+  t.test('create a repo with workspaces', () => {
     const workspacesRepo = resolve(me, 'workspaces-repo')
     const wsfolder = resolve(me, 'workspaces-repo/a')
     const git = (...cmd) => spawnGit(cmd, { cwd: workspacesRepo })
@@ -315,7 +315,7 @@ t.test('setup', { bail: true }, t => {
       .then(() => git('commit', '-m', 'a/package.json'))
   })
 
-  t.test('create a repo with only a prepack script', t => {
+  t.test('create a repo with only a prepack script', () => {
     const prepackRepo = resolve(me, 'prepack-repo')
     const git = (...cmd) => spawnGit(cmd, { cwd: prepackRepo })
     const write = (f, c) => fs.writeFileSync(`${prepackRepo}/${f}`, c)
@@ -608,7 +608,7 @@ t.test('fetch a weird ref', t => {
   t.end()
 })
 
-t.test('fetch a private repo where the tgz is a 404', t => {
+t.test('fetch a private repo where the tgz is a 404', () => {
   const gf = new GitFetcher(`localhost:repo/x#${REPO_HEAD}`, opts)
   gf.spec.hosted.tarball = () => `${hostedUrl}/not-found.tgz`
   // should fetch it by falling back to ssh when it gets an http error