feat(preprocessors): if a file matches multiple preprocessor patterns, intelligently merge the list of preprocessors, deduping and trying to preserve the order

sjelin · sjelin · commit 59642a625472 · 2016-02-22T08:33:37.000-08:00
This could be a breaking change, some users might rely on the old order, or on some preprocessors being run multiple times
diff --git a/docs/config/04-preprocessors.md b/docs/config/04-preprocessors.md
@@ -93,3 +93,50 @@ return `false` and the preprocessor would not be executed on the CoffeeScript fi
 [custom plugins]: ../dev/plugins.html
 [plugins]: plugins.html
 [issue788]: https://github.com/karma-runner/karma/issues/788
+
+## Order of execution
+
+If a file matches only one key in the preprocessors config object, then karma
+will execute the preprocessors over that file in the order they are listed in
+the corresponding array.  So for instance if the config object is:
+
+```js
+preprocessors: {
+  '*.js': ['a', 'b']
+}
+```
+
+Then karma will execute `'a'` before executing `'b'`.
+
+If a file matches multiple keys, karma will do its best to execute the
+preprocessors in a reasonable order.  So if you have:
+
+```js
+preprocessors: {
+  '*.js': ['a', 'b'],
+  'a.*': ['b', 'c']
+}
+```
+
+then for `a.js`, karma will run `'a'` then `'b'` then `'c'`.  If two lists
+contradict eachother, like:
+
+```js
+preprocessors: {
+  '*.js': ['a', 'b'],
+  'a.*': ['b', 'a']
+}
+```
+
+then karma will arbitrarily pick one list to prioritize over the other.  In a
+case like:
+
+```js
+preprocessors: {
+  '*.js': ['a', 'b', 'c'],
+  'a.*': ['c', 'b', 'd']
+}
+```
+
+Then `'a'` will definitely be run first, `'d'` will definitely be run last, but
+it's arbitrarily if karma will run `'b'` before `'c'` or vice versa.
diff --git a/lib/preprocessor.js b/lib/preprocessor.js
@@ -2,6 +2,7 @@ var fs = require('graceful-fs')
 var crypto = require('crypto')
 var mm = require('minimatch')
 var isBinaryFile = require('isbinaryfile')
+var combineLists = require('combine-lists')
 
 var log = require('./logger').create('preprocess')
 
@@ -64,9 +65,11 @@ var createPreprocessor = function (config, basePath, injector) {
     return p
   }
 
+  var allPreprocessors = []
   patterns.forEach(function (pattern) {
-    config[pattern].forEach(instantiatePreprocessor)
+    allPreprocessors = combineLists(allPreprocessors, config[pattern])
   })
+  allPreprocessors.forEach(instantiatePreprocessor)
 
   return function preprocess (file, done) {
     patterns = Object.keys(config)
@@ -81,36 +84,38 @@ var createPreprocessor = function (config, basePath, injector) {
           throw err
         }
 
-        var preprocessors = []
-        var nextPreprocessor = createNextProcessor(preprocessors, file, done)
-
+        var preprocessorNames = []
         for (var i = 0; i < patterns.length; i++) {
           if (mm(file.originalPath, patterns[i], {dot: true})) {
             if (thisFileIsBinary) {
               log.warn('Ignoring preprocessing (%s) %s because it is a binary file.',
                 config[patterns[i]].join(', '), file.originalPath)
             } else {
-              config[patterns[i]].forEach(function (name) {
-                var p = instances[name]
-                if (p == null) {
-                  p = instantiatePreprocessor(name)
-                }
-
-                if (p == null) {
-                  if (!alreadyDisplayedWarnings[name]) {
-                    alreadyDisplayedWarnings[name] = true
-                    log.warn('Failed to instantiate preprocessor %s', name)
-                  }
-                  return
-                }
-
-                instances[name] = p
-                preprocessors.push(p)
-              })
+              preprocessorNames = combineLists(preprocessorNames, config[patterns[i]])
             }
           }
         }
 
+        var preprocessors = []
+        var nextPreprocessor = createNextProcessor(preprocessors, file, done)
+        preprocessorNames.forEach(function (name) {
+          var p = instances[name]
+          if (p == null) {
+            p = instantiatePreprocessor(name)
+          }
+
+          if (p == null) {
+            if (!alreadyDisplayedWarnings[name]) {
+              alreadyDisplayedWarnings[name] = true
+              log.warn('Failed to instantiate preprocessor %s', name)
+            }
+            return
+          }
+
+          instances[name] = p
+          preprocessors.push(p)
+        })
+
         nextPreprocessor(null, thisFileIsBinary ? buffer : buffer.toString())
       })
     })
diff --git a/package.json b/package.json
@@ -266,6 +266,7 @@
     "body-parser": "^1.12.4",
     "chokidar": "^1.4.1",
     "colors": "^1.1.0",
+    "combine-lists": "^1.0.0",
     "connect": "^3.3.5",
     "core-js": "^2.1.0",
     "di": "^0.0.1",
diff --git a/test/unit/preprocessor.spec.js b/test/unit/preprocessor.spec.js
@@ -271,4 +271,54 @@ describe('preprocessor', () => {
       done()
     })
   })
+
+  it('should merge lists of preprocessors', (done) => {
+    var callOrder = []
+    var fakePreprocessorA = sinon.spy((content, file, done) => {
+      callOrder.push('a')
+      done(null, content)
+    })
+    var fakePreprocessorB = sinon.spy((content, file, done) => {
+      callOrder.push('b')
+      done(null, content)
+    })
+    var fakePreprocessorC = sinon.spy((content, file, done) => {
+      callOrder.push('c')
+      done(null, content)
+    })
+    var fakePreprocessorD = sinon.spy((content, file, done) => {
+      callOrder.push('d')
+      done(null, content)
+    })
+
+    var injector = new di.Injector([{
+      'preprocessor:fakeA': ['factory', () => fakePreprocessorA],
+      'preprocessor:fakeB': ['factory', () => fakePreprocessorB],
+      'preprocessor:fakeC': ['factory', () => fakePreprocessorC],
+      'preprocessor:fakeD': ['factory', () => fakePreprocessorD]
+    }])
+
+    pp = m.createPreprocessor({
+      '/*/a.js': ['fakeA', 'fakeB'],
+      '/some/*': ['fakeB', 'fakeC'],
+      '/some/a.js': ['fakeD']
+    }, null, injector)
+
+    var file = {originalPath: '/some/a.js', path: 'path'}
+
+    pp(file, (err) => {
+      if (err) throw err
+
+      expect(fakePreprocessorA).to.have.been.called
+      expect(fakePreprocessorB).to.have.been.called
+      expect(fakePreprocessorC).to.have.been.called
+      expect(fakePreprocessorD).to.have.been.called
+
+      expect(callOrder.indexOf('d')).not.to.equal(-1)
+      expect(callOrder.filter((letter) => {
+        return letter !== 'd'
+      })).to.eql(['a', 'b', 'c'])
+      done()
+    })
+  })
 })
