Cache manifest as fetcher.package

This allows greater efficiency, and will make it possible to set the mode of package bins so that they're always executable.
npm · Oct 29, 2019 · 347c563 · 347c563
1 parent 8d38b28
commit 347c563
Show file tree

Hide file tree

Showing 10 changed files with 80 additions and 20 deletions.
diff --git a/lib/dir.js b/lib/dir.js
@@ -21,9 +21,8 @@ class DirFetcher extends Fetcher {
     return ['directory']
   }
 
-  [_prepareDir] (dir) {
-    return readPackageJson(dir + '/package.json')
-      .then(mani => {
+  [_prepareDir] () {
+    return this.manifest().then(mani => {
         if (!mani.scripts || !mani.scripts.prepare)
           return
 
@@ -33,7 +32,7 @@ class DirFetcher extends Fetcher {
         return npm(
           this.npmBin,
           [].concat(this.npmRunCmd).concat('prepare').concat(this.npmCliConfig),
-          dir,
+          this.resolved,
           'directory preparation failed'
         )
       })
@@ -46,7 +45,7 @@ class DirFetcher extends Fetcher {
 
     // run the prepare script, get the list of files, and tar it up
     // pipe to the stream, and proxy errors the chain.
-    this[_prepareDir](this.resolved)
+    this[_prepareDir]()
       .then(() => packlist({ path: this.resolved }))
       .then(files => tar.c({
         cwd: this.resolved,
@@ -63,12 +62,15 @@ class DirFetcher extends Fetcher {
   }
 
   manifest () {
+    if (this.package)
+      return Promise.resolve(this.package)
+
     return readPackageJson(this.resolved + '/package.json')
-      .then(mani => ({
+      .then(mani => this.package = {
         ...mani,
         _integrity: String(this.integrity),
         _resolved: this.resolved,
-      }))
+      })
   }
 
   packument () {

diff --git a/lib/fetcher.js b/lib/fetcher.js
@@ -49,6 +49,7 @@ class FetcherBase {
     this.cache = opts.cache || cacheDir()
     this.resolved = opts.resolved || null
     this.integrity = opts.integrity || null
+    this.package = null
     this.type = this.constructor.name
     this.fmode = opts.fmode || 0o666
     this.dmode = opts.dmode || 0o777

diff --git a/lib/file.js b/lib/file.js
@@ -17,15 +17,18 @@ class FileFetcher extends Fetcher {
   }
 
   manifest () {
+    if (this.package)
+      return Promise.resolve(this.package)
+
     // have to unpack the tarball for this.
     return cacache.tmp.withTmp(this.cache, this.opts, dir =>
       this.extract(dir)
       .then(() => readPackageJson(dir + '/package.json'))
-      .then(mani => ({
+      .then(mani => this.package = {
         ...mani,
         _integrity: String(this.integrity),
         _resolved: this.resolved,
-      })))
+      }))
   }
 
   [_tarballFromResolved] () {

diff --git a/lib/git.js b/lib/git.js
@@ -203,15 +203,18 @@ class GitFetcher extends Fetcher {
   }
 
   manifest () {
+    if (this.package)
+      return Promise.resolve(this.package)
+
     return this.spec.hosted && this.resolved
       ? FileFetcher.prototype.manifest.apply(this)
       : this[_clone](dir =>
           readPackageJson(dir + '/package.json')
-            .then(mani => ({
+            .then(mani => this.package = {
               ...mani,
               _integrity: String(this.integrity),
               _resolved: this.resolved,
-            })))
+            }))
   }
 
   packument () {

diff --git a/lib/registry.js b/lib/registry.js
@@ -92,6 +92,9 @@ class RegistryFetcher extends Fetcher {
   }
 
   manifest () {
+    if (this.package)
+      return Promise.resolve(this.package)
+
     return this.packument()
       .then(packument => pickManifest(packument, this.spec.fetchSpec, {
         ...this.opts,
@@ -110,20 +113,18 @@ class RegistryFetcher extends Fetcher {
           if (this.integrity)
             mani._integrity = String(this.integrity)
         }
-        return mani
+        return this.package = mani
       })
   }
 
   [_tarballFromResolved] () {
     // we use a RemoteFetcher to get the actual tarball stream
-    const stream = new Minipass()
-    new RemoteFetcher(this.resolved, {
+    return new RemoteFetcher(this.resolved, {
       ...this.opts,
       integrity: this.integrity,
       resolved: this.resolved,
       pkgid: `registry:${this.spec.name}@${this.resolved}`,
-    })[_tarballFromResolved]().pipe(stream)
-    return stream
+    })[_tarballFromResolved]()
   }
 
   get types () {

diff --git a/tap-snapshots/test-dir.js-TAP.test.js b/tap-snapshots/test-dir.js-TAP.test.js
@@ -126,6 +126,45 @@ Object {
 }
 `
 
+exports[`test/dir.js TAP basic > saved package.json 1`] = `
+Object {
+  "_id": "abbrev@1.1.1",
+  "_integrity": "null",
+  "_resolved": "\${CWD}/test/fixtures/abbrev",
+  "author": Object {
+    "email": "i@izs.me",
+    "name": "Isaac Z. Schlueter",
+  },
+  "bugs": Object {
+    "url": "https://github.com/isaacs/abbrev-js/issues",
+  },
+  "description": "Like ruby's abbrev module, but in js",
+  "devDependencies": Object {
+    "tap": "^10.1",
+  },
+  "files": Array [
+    "abbrev.js",
+  ],
+  "homepage": "https://github.com/isaacs/abbrev-js#readme",
+  "license": "ISC",
+  "main": "abbrev.js",
+  "name": "abbrev",
+  "readme": "# abbrev-js\\n\\nJust like [ruby's Abbrev](http://apidock.com/ruby/Abbrev).\\n\\nUsage:\\n\\n    var abbrev = require(\\"abbrev\\");\\n    abbrev(\\"foo\\", \\"fool\\", \\"folding\\", \\"flop\\");\\n    \\n    // returns:\\n    { fl: 'flop'\\n    , flo: 'flop'\\n    , flop: 'flop'\\n    , fol: 'folding'\\n    , fold: 'folding'\\n    , foldi: 'folding'\\n    , foldin: 'folding'\\n    , folding: 'folding'\\n    , foo: 'foo'\\n    , fool: 'fool'\\n    }\\n\\nThis is handy for command-line scripts, or other cases where you want to be able to accept shorthands.\\n",
+  "readmeFilename": "README.md",
+  "repository": Object {
+    "type": "git",
+    "url": "git+ssh://git@github.com/isaacs/abbrev-js.git",
+  },
+  "scripts": Object {
+    "postpublish": "git push origin --all; git push origin --tags",
+    "postversion": "npm publish",
+    "preversion": "npm test",
+    "test": "tap test.js --100",
+  },
+  "version": "1.1.1",
+}
+`
+
 exports[`test/dir.js TAP with prepare script > extract 1`] = `
 Object {
   "integrity": "sha512-HTzPAt8wmXNchUdisnGDSCuUgrFee5v8F6GsLc5mQd29VXiNzv4PGz71jjLSIF1wWQSB+UjLTmSJSGznF/s/Lw==",

diff --git a/test/dir.js b/test/dir.js
@@ -23,6 +23,8 @@ t.test('basic', t => {
   const pj = me + '/abbrev/package.json'
   return t.resolveMatchSnapshot(f.extract(me + '/abbrev'), 'extract')
     .then(() => t.matchSnapshot(require(pj), 'package.json extracted'))
+    .then(() => t.matchSnapshot(f.package, 'saved package.json'))
+    .then(() => f.manifest().then(mani => t.equal(mani, f.package)))
 })
 
 const prepare = resolve(__dirname, 'fixtures/prepare-script')

diff --git a/test/file.js b/test/file.js
@@ -14,11 +14,14 @@ t.teardown(() => rimraf.sync(me))
 const abbrev = resolve(__dirname, 'fixtures/abbrev-1.1.1.tgz')
 const abbrevspec = `file:${relative(process.cwd(), abbrev)}`
 
-t.test('basic', t => {
+t.test('basic', async t => {
   const f = new FileFetcher(abbrevspec, {})
   t.same(f.types, ['file'])
-  t.resolveMatchSnapshot(f.packument(), 'packument')
-  t.resolveMatchSnapshot(f.manifest(), 'manifest')
+  const fm = await f.manifest()
+  t.matchSnapshot(fm, 'manifest')
+  t.equal(fm, f.package)
+  t.equal(await f.manifest(), fm, 'cached manifest')
+  t.matchSnapshot(await f.packument(), 'packument')
   const pj = me + '/extract/package.json'
   return t.resolveMatchSnapshot(f.extract(me + '/extract'), 'extract')
     .then(() => t.matchSnapshot(require(pj), 'package.json extracted'))

diff --git a/test/git.js b/test/git.js
@@ -255,6 +255,7 @@ t.test('basic stuff', async t => {
   t.throws(() => fs.statSync(me + '/g/prepare.js'))
 
   const gm = await g.manifest()
+  t.equal(gm, g.package)
   t.match(gm, {
     name: 'repo',
     version: '1.0.0',
@@ -271,6 +272,7 @@ t.test('basic stuff', async t => {
     _integrity: `${g.integrity}`,
     _resolved: `${remote}#${g.resolvedSha}`,
   })
+  t.equal(await g.manifest(), gm, 'cached manifest')
 
   // one that doesn't have an npm install step, but does have submods
   const subs = new GitFetcher(submodsRemote, {cache})

diff --git a/test/registry.js b/test/registry.js
@@ -64,7 +64,11 @@ t.test('underscore, no tag or version', t => {
   const f = new RegistryFetcher('underscore', {registry, cache})
 
   return f.resolve().then(r => t.equal(r, `${registry}underscore/-/underscore-1.5.1.tgz`))
-  .then(() => f.manifest()).then(m => t.match(m, { version: '1.5.1' }))
+  .then(() => f.manifest()).then(m => {
+    t.equal(m, f.package)
+    t.match(m, { version: '1.5.1' })
+    return f.manifest().then(m2 => t.equal(m, m2, 'manifest cached'))
+  })
   .then(() => f.extract(me + '/underscore'))
   .then(result => t.deepEqual(result, {
     resolved: `${registry}underscore/-/underscore-1.5.1.tgz`,