fix(refactor): symbol cleanup (#365)

fix: move bin to its own directory

lib/bin.js → bin/index.js

@@ -1,5 +1,4 @@
 #!/usr/bin/env node
-/* eslint-disable no-console */
 
 const run = conf => {
   const pacote = require('../')

lib/dir.js

@@ -4,13 +4,10 @@ const { Minipass } = require('minipass')
 const tarCreateOptions = require('./util/tar-create-options.js')
 const packlist = require('npm-packlist')
 const tar = require('tar')
-const _prepareDir = Symbol('_prepareDir')
 const { resolve } = require('path')
-const _readPackageJson = Symbol.for('package.Fetcher._readPackageJson')
-
 const runScript = require('@npmcli/run-script')
+const _ = require('./util/protected.js')
 
-const _tarballFromResolved = Symbol.for('pacote.Fetcher._tarballFromResolved')
 class DirFetcher extends Fetcher {
   constructor (spec, opts) {
     super(spec, opts)
@@ -30,7 +27,7 @@ class DirFetcher extends Fetcher {
     return ['directory']
   }
 
-  [_prepareDir] () {
+  [_.prepareDir] () {
     return this.manifest().then(mani => {
       if (!mani.scripts || !mani.scripts.prepare) {
         return
@@ -55,7 +52,7 @@ class DirFetcher extends Fetcher {
     })
   }
 
-  [_tarballFromResolved] () {
+  [_.tarballFromResolved] () {
     if (!this.tree && !this.Arborist) {
       throw new Error('DirFetcher requires either a tree or an Arborist constructor to pack')
     }
@@ -68,7 +65,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[_.prepareDir]()
       .then(async () => {
         if (!this.tree) {
           const arb = new this.Arborist({ path: this.resolved })
@@ -87,7 +84,7 @@ class DirFetcher extends Fetcher {
       return Promise.resolve(this.package)
     }
 
-    return this[_readPackageJson](this.resolved)
+    return this[_.readPackageJson](this.resolved)
       .then(mani => this.package = {
         ...mani,
         _integrity: this.integrity && String(this.integrity),

lib/fetcher.js

@@ -18,27 +18,12 @@ const getContents = require('@npmcli/installed-package-contents')
 const PackageJson = require('@npmcli/package-json')
 const { Minipass } = require('minipass')
 const cacheDir = require('./util/cache-dir.js')
+const _ = require('./util/protected.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.
-const _extract = Symbol('_extract')
-const _mkdir = Symbol('_mkdir')
-const _empty = Symbol('_empty')
-const _toFile = Symbol('_toFile')
-const _tarxOptions = Symbol('_tarxOptions')
-const _entryMode = Symbol('_entryMode')
-const _istream = Symbol('_istream')
-const _assertType = Symbol('_assertType')
-const _tarballFromCache = Symbol('_tarballFromCache')
-const _tarballFromResolved = Symbol.for('pacote.Fetcher._tarballFromResolved')
-const _cacheFetches = Symbol.for('pacote.Fetcher._cacheFetches')
-const _readPackageJson = Symbol.for('package.Fetcher._readPackageJson')
-
 class FetcherBase {
   constructor (spec, opts) {
     if (!opts || typeof opts !== 'object') {
@@ -57,7 +42,7 @@ class FetcherBase {
     this.from = this.spec.registry
       ? `${this.spec.name}@${this.spec.rawSpec}` : this.spec.saveSpec
 
-    this[_assertType]()
+    this.#assertType()
     // clone the opts object so that others aren't upset when we mutate it
     // by adding/modifying the integrity value.
     this.opts = { ...opts }
@@ -93,11 +78,9 @@ class FetcherBase {
     this.before = opts.before
     this.fullMetadata = this.before ? true : !!opts.fullMetadata
     this.fullReadJson = !!opts.fullReadJson
-    if (this.fullReadJson) {
-      this[_readPackageJson] = packageJsonPrepare
-    } else {
-      this[_readPackageJson] = packageJsonNormalize
-    }
+    this[_.readPackageJson] = this.fullReadJson
+      ? packageJsonPrepare
+      : packageJsonNormalize
 
     // rrh is a registry hostname or 'never' or 'always'
     // defaults to registry.npmjs.org
@@ -188,7 +171,7 @@ class FetcherBase {
   // private, should be overridden.
   // Note that they should *not* calculate or check integrity or cache,
   // but *just*  return the raw tarball data stream.
-  [_tarballFromResolved] () {
+  [_.tarballFromResolved] () {
     throw this.notImplementedError
   }
 
@@ -204,17 +187,17 @@ class FetcherBase {
 
   // private
   // Note: cacache will raise a EINTEGRITY error if the integrity doesn't match
-  [_tarballFromCache] () {
+  #tarballFromCache () {
     return cacache.get.stream.byDigest(this.cache, this.integrity, this.opts)
   }
 
-  get [_cacheFetches] () {
+  get [_.cacheFetches] () {
     return true
   }
 
-  [_istream] (stream) {
+  #istream (stream) {
     // if not caching this, just return it
-    if (!this.opts.cache || !this[_cacheFetches]) {
+    if (!this.opts.cache || !this[_.cacheFetches]) {
       // instead of creating a new integrity stream, we only piggyback on the
       // provided stream's events
       if (stream.hasIntegrityEmitter) {
@@ -267,7 +250,7 @@ class FetcherBase {
     return false
   }
 
-  [_assertType] () {
+  #assertType () {
     if (this.types && !this.types.includes(this.spec.type)) {
       throw new TypeError(`Wrong spec type (${
         this.spec.type
@@ -306,7 +289,7 @@ class FetcherBase {
       !this.preferOnline &&
       this.integrity &&
       this.resolved
-    ) ? streamHandler(this[_tarballFromCache]()).catch(er => {
+    ) ? streamHandler(this.#tarballFromCache()).catch(er => {
         if (this.isDataCorruptionError(er)) {
           log.warn('tarball', `cached data for ${
           this.spec
@@ -329,7 +312,7 @@ class FetcherBase {
         }. Extracting by manifest.`)
       }
       return this.resolve().then(() => retry(tryAgain =>
-        streamHandler(this[_istream](this[_tarballFromResolved]()))
+        streamHandler(this.#istream(this[_.tarballFromResolved]()))
           .catch(streamErr => {
           // Most likely data integrity.  A cache ENOENT error is unlikely
           // here, since we're definitely not reading from the cache, but it
@@ -352,24 +335,24 @@ class FetcherBase {
     return cacache.rm.content(this.cache, this.integrity, this.opts)
   }
 
-  [_empty] (path) {
+  #empty (path) {
     return getContents({ path, depth: 1 }).then(contents => Promise.all(
       contents.map(entry => fs.rm(entry, { recursive: true, force: true }))))
   }
 
-  async [_mkdir] (dest) {
-    await this[_empty](dest)
+  async #mkdir (dest) {
+    await this.#empty(dest)
     return await fs.mkdir(dest, { recursive: true })
   }
 
   // extraction is always the same.  the only difference is where
   // the tarball comes from.
   async extract (dest) {
-    await this[_mkdir](dest)
-    return this.tarballStream((tarball) => this[_extract](dest, tarball))
+    await this.#mkdir(dest)
+    return this.tarballStream((tarball) => this.#extract(dest, tarball))
   }
 
-  [_toFile] (dest) {
+  #toFile (dest) {
     return this.tarballStream(str => new Promise((res, rej) => {
       const writer = new fsm.WriteStream(dest)
       str.on('error', er => writer.emit('error', er))
@@ -383,15 +366,15 @@ class FetcherBase {
     }))
   }
 
-  // don't use this[_mkdir] because we don't want to rimraf anything
+  // don't use this.#mkdir because we don't want to rimraf anything
   async tarballFile (dest) {
     const dir = dirname(dest)
     await fs.mkdir(dir, { recursive: true })
-    return this[_toFile](dest)
+    return this.#toFile(dest)
   }
 
-  [_extract] (dest, tarball) {
-    const extractor = tar.x(this[_tarxOptions]({ cwd: dest }))
+  #extract (dest, tarball) {
+    const extractor = tar.x(this.#tarxOptions({ cwd: dest }))
     const p = new Promise((resolve, reject) => {
       extractor.on('end', () => {
         resolve({
@@ -416,7 +399,7 @@ class FetcherBase {
 
   // always ensure that entries are at least as permissive as our configured
   // dmode/fmode, but never more permissive than the umask allows.
-  [_entryMode] (path, mode, type) {
+  #entryMode (path, mode, type) {
     const m = /Directory|GNUDumpDir/.test(type) ? this.dmode
       : /File$/.test(type) ? this.fmode
       : /* istanbul ignore next - should never happen in a pkg */ 0
@@ -427,7 +410,7 @@ class FetcherBase {
     return ((mode | m) & ~this.umask) | exe | 0o600
   }
 
-  [_tarxOptions] ({ cwd }) {
+  #tarxOptions ({ cwd }) {
     const sawIgnores = new Set()
     return {
       cwd,
@@ -437,7 +420,7 @@ class FetcherBase {
         if (/Link$/.test(entry.type)) {
           return false
         }
-        entry.mode = this[_entryMode](entry.path, entry.mode, entry.type)
+        entry.mode = this.#entryMode(entry.path, entry.mode, entry.type)
         // this replicates the npm pack behavior where .gitignore files
         // are treated like .npmignore files, but only if a .npmignore
         // file is not present.

lib/file.js

@@ -3,10 +3,7 @@ const cacache = require('cacache')
 const { resolve } = require('path')
 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')
+const _ = require('./util/protected.js')
 
 class FileFetcher extends Fetcher {
   constructor (spec, opts) {
@@ -27,7 +24,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))
+        .then(() => this[_.readPackageJson](dir))
         .then(mani => this.package = {
           ...mani,
           _integrity: this.integrity && String(this.integrity),
@@ -36,7 +33,7 @@ class FileFetcher extends Fetcher {
         }))
   }
 
-  [_exeBins] (pkg, dest) {
+  #exeBins (pkg, dest) {
     if (!pkg.bin) {
       return Promise.resolve()
     }
@@ -65,11 +62,11 @@ class FileFetcher extends Fetcher {
     // but if not, read the unpacked manifest and chmod properly.
     return super.extract(dest)
       .then(result => this.package ? result
-      : this[_readPackageJson](dest).then(pkg =>
-        this[_exeBins](pkg, dest)).then(() => result))
+      : this[_.readPackageJson](dest).then(pkg =>
+        this.#exeBins(pkg, dest)).then(() => result))
   }
 
-  [_tarballFromResolved] () {
+  [_.tarballFromResolved] () {
     // create a read stream and return it
     return new fsm.ReadStream(this.resolved)
   }