refactor: dry and other pr comments

PR-URL: #391
Credit: @JamieMagee
Close: #391
Reviewed-by: @isaacs

Author: JamieMagee

lib/pack.js

@@ -79,23 +79,25 @@ const Pack = warner(class Pack extends Minipass {
 
     this.portable = !!opt.portable
     this.zip = null
-    if (opt.gzip) {
-      if (typeof opt.gzip !== 'object') {
-        opt.gzip = {}
+    if (opt.gzip || opt.brotli) {
+      if (opt.gzip && opt.brotli) {
+        throw new TypeError('gzip and brotli are mutually exclusive')
       }
-      if (this.portable) {
-        opt.gzip.portable = true
+      if (opt.gzip) {
+        if (typeof opt.gzip !== 'object') {
+          opt.gzip = {}
+        }
+        if (this.portable) {
+          opt.gzip.portable = true
+        }
+        this.zip = new zlib.Gzip(opt.gzip)
       }
-      this.zip = new zlib.Gzip(opt.gzip)
-      this.zip.on('data', chunk => super.write(chunk))
-      this.zip.on('end', _ => super.end())
-      this.zip.on('drain', _ => this[ONDRAIN]())
-      this.on('resume', _ => this.zip.resume())
-    } else if (opt.brotli) {
-      if (typeof opt.brotli !== 'object') {
-        opt.brotli = {}
+      if (opt.brotli) {
+        if (typeof opt.brotli !== 'object') {
+          opt.brotli = {}
+        }
+        this.zip = new zlib.BrotliCompress(opt.brotli)
       }
-      this.zip = new zlib.BrotliCompress(opt.brotli)
       this.zip.on('data', chunk => super.write(chunk))
       this.zip.on('end', _ => super.end())
       this.zip.on('drain', _ => this[ONDRAIN]())

lib/parse.js

@@ -99,7 +99,8 @@ module.exports = warner(class Parser extends EE {
     this.filter = typeof opt.filter === 'function' ? opt.filter : noop
     // Unlike gzip, brotli doesn't have any magic bytes to identify it
     // Users need to explicitly tell us they're extracting a brotli file
-    this.brotli = opt.brotli
+    // Or we infer from the file extension
+    this.brotli = opt.brotli || (opt.file && (opt.file.endsWith('.tar.br') || opt.file.endsWith('.tbr')))
 
     // have to set this so that streams are ok piping into it
     this.writable = true
@@ -359,30 +360,15 @@ module.exports = warner(class Parser extends EE {
         this[BUFFER] = chunk
         return true
       }
-      if (this[UNZIP] === null && this.brotli) {
-        const ended = this[ENDED]
-        this[ENDED] = false
-        this[UNZIP] = new zlib.BrotliDecompress()
-        this[UNZIP].on('data', chunk => this[CONSUMECHUNK](chunk))
-        this[UNZIP].on('error', er => this.abort(er))
-        this[UNZIP].on('end', _ => {
-          this[ENDED] = true
-          this[CONSUMECHUNK]()
-        })
-        this[WRITING] = true
-        const ret = this[UNZIP][ended ? 'end' : 'write'](chunk)
-        this[WRITING] = false
-        return ret
-      }
       for (let i = 0; this[UNZIP] === null && i < gzipHeader.length; i++) {
         if (chunk[i] !== gzipHeader[i]) {
           this[UNZIP] = false
         }
       }
-      if (this[UNZIP] === null) {
+      if (this[UNZIP] === null || (this[UNZIP] === false && this.brotli)) {
         const ended = this[ENDED]
         this[ENDED] = false
-        this[UNZIP] = new zlib.Unzip()
+        this[UNZIP] = this.brotli ? new zlib.BrotliDecompress() : new zlib.Unzip()
         this[UNZIP].on('data', chunk => this[CONSUMECHUNK](chunk))
         this[UNZIP].on('error', er => this.abort(er))
         this[UNZIP].on('end', _ => {

test/extract.js

@@ -323,12 +323,25 @@ t.test('brotli', async t => {
     await rimraf(dir)
   })
 
-  t.test('fails if brotli', async t => {
-    const expect = new Error("TAR_BAD_ARCHIVE: Unrecognized archive format")
-    t.throws(_ => x({ sync: true, file: file }), expect)
+  t.test('fails if unknown file extension', async t => {
+    const filename = path.resolve(__dirname, 'brotli/example.unknown')
+    const f = fs.openSync(filename, 'a')
+    fs.closeSync(f)
+
+    const expect = new Error('TAR_BAD_ARCHIVE: Unrecognized archive format')
+
+    t.throws(_ => x({ sync: true, file: filename }), expect)
+  })
+
+  t.test('succeeds based on file extension', t => {
+    x({ sync: true, file: file, C: dir })
+
+    t.same(fs.readdirSync(dir + '/x').sort(),
+      ['1', '10', '2', '3', '4', '5', '6', '7', '8', '9'])
+    t.end()
   })
 
-  t.test('succeeds', t => {
+  t.test('succeeds when passed explicit option', t => {
     x({ sync: true, file: file, C: dir, brotli: true })
 
     t.same(fs.readdirSync(dir + '/x').sort(),

test/pack.js

@@ -382,6 +382,12 @@ t.test('if brotli is truthy, make it an object', t => {
   t.end()
 })
 
+t.test('throws if both gzip and brotli are truthy', t => {
+  const opt = { gzip: true, brotli: true }
+  t.throws(_ => new Pack(opt), new TypeError('gzip and brotli are mutually exclusive'))
+  t.end()
+})
+
 t.test('gzip, also a very deep path', t => {
   const out = []
 

test/parse.js

@@ -125,6 +125,28 @@ t.test('fixture tests', t => {
         bs.end(zlib.gzipSync(tardata))
       })
 
+      t.test('compress with brotli based on filename .tar.br', t => {
+        const p = new Parse({
+          maxMetaEntrySize: maxMeta,
+          filter: filter ? (path, entry) => entry.size % 2 !== 0 : null,
+          strict: strict,
+          file: 'example.tar.br',
+        })
+        trackEvents(t, expect, p)
+        p.end(zlib.brotliCompressSync(tardata))
+      })
+
+      t.test('compress with brotli based on filename .tbr', t => {
+        const p = new Parse({
+          maxMetaEntrySize: maxMeta,
+          filter: filter ? (path, entry) => entry.size % 2 !== 0 : null,
+          strict: strict,
+          file: 'example.tbr',
+        })
+        trackEvents(t, expect, p)
+        p.end(zlib.brotliCompressSync(tardata))
+      })
+
       t.test('compress with brotli all at once', t => {
         const p = new Parse({
           maxMetaEntrySize: maxMeta,