From 27b26d4f8f337386ac7cce6bc4e09effb066e1a4 Mon Sep 17 00:00:00 2001
From: Rob Palmer <rpalmer57@bloomberg.net>
Date: Thu, 15 Feb 2018 14:56:34 +0000
Subject: [PATCH] fix(composer): invoke callback only once

In Node.js Streams, if an error is omitted downstream, that error can be
propagated back through connected streams. In gulp-uglify, this error
was being thrown by the next callback, being caught, and callback called
a second time.

This CL ensures the callback is called only once, by calling it outside
the try..catch block wrapping the minifier.

[terinjokes@gmail.com: amended change, rewrote tests and commit message]
Signed-off-by: Terin Stock <terinjokes@gmail.com>
---
 composer.js      | 10 ++++---
 test/composer.js | 73 ++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 79 insertions(+), 4 deletions(-)
 create mode 100644 test/composer.js

diff --git a/composer.js b/composer.js
index 52f86b4..26d66d2 100644
--- a/composer.js
+++ b/composer.js
@@ -6,12 +6,14 @@ module.exports = function(uglify, logger) {
   return function(opts) {
     var minifier = minify(uglify, logger)(opts);
     return through.obj(function(file, encoding, callback) {
+      var newFile = null;
+      var err = null;
       try {
-        var newFile = minifier(file);
-        callback(null, newFile);
-      } catch (err) {
-        callback(err);
+        newFile = minifier(file);
+      } catch (e) {
+        err = e;
       }
+      callback(err, newFile);
     });
   };
 };
diff --git a/test/composer.js b/test/composer.js
new file mode 100644
index 0000000..6e8098e
--- /dev/null
+++ b/test/composer.js
@@ -0,0 +1,73 @@
+'use strict';
+var test = require('tape-catch');
+var assert = require('assert');
+var Buffer = require('safe-buffer').Buffer;
+var Vinyl = require('vinyl');
+var td = require('testdouble');
+var through = require('through2');
+var composer = require('../composer');
+var GulpUglifyError = require('../lib/gulp-uglify-error');
+
+test('composer should forward errors', function(t) {
+  var badJsFile = new Vinyl({
+    cwd: '/',
+    base: '/test/',
+    path: '/test/file.js',
+    contents: Buffer.from('invalid js')
+  });
+
+  var uglify = td.object(['minify']);
+  var logger = td.object(['warn']);
+  var composed = composer(uglify, logger)({});
+
+  assert.throws(function() {
+    composed.write(badJsFile);
+  }, GulpUglifyError);
+
+  td.reset();
+  t.end();
+});
+
+test("compose doesn't invoke callback twice", function(t) {
+  var expectedErr = new Error();
+  var jsFile = new Vinyl({
+    cwd: '/',
+    base: '/test/',
+    path: '/test/file.js',
+    contents: Buffer.from('var x = 123')
+  });
+
+  var thrower = through.obj(function() {
+    throw expectedErr;
+  });
+
+  var uglify = td.object(['minify']);
+  var logger = td.object(['warn']);
+  td.when(
+    uglify.minify(
+      {'file.js': 'var x = 123'},
+      {
+        output: {}
+      }
+    )
+  ).thenReturn({
+    code: '',
+    map: {}
+  });
+
+  var composed = composer(uglify, logger)({});
+  composed.pipe(thrower);
+
+  assert.throws(
+    function() {
+      composed.write(jsFile);
+    },
+    function(err) {
+      assert.strictEqual(err, expectedErr);
+      return true;
+    }
+  );
+
+  td.reset();
+  t.end();
+});