refactor: lint dirty modules plugin (#59)

* refactor: use filter on filetimestamps' keys

as opposed to reduce. also use lodash.defaultto for comparing numeric
values to avoid falsy JS bugs

* refactor: use Arrange Act Assert model for tests

and clean up some assertions and specs

* refactor: pulls out defaultFilesGlob constant

* refactor: clean up typos

* refactor: use .test.js file suffix

to enable the recursive flag in mocha.

* refactor: better names for all fixtures

* tests: add coverage for without plugin config

* tests: update NoErrorsPlugin tests

Verify that existing behaviour works as expected

* refactor: minor test tweaks

* tests: correct test of empty plugin config

* tests: remove extra context from lint dirty modules plugin

index.js

@@ -9,6 +9,7 @@ var formatter = require('stylelint').formatters.string;
 // Modules
 var runCompilation = require('./lib/run-compilation');
 var LintDirtyModulesPlugin = require('./lib/lint-dirty-modules-plugin');
+var defaultFilesGlob = require('./lib/constants').defaultFilesGlob;
 
 function apply (options, compiler) {
   options = options || {};
@@ -19,7 +20,7 @@ function apply (options, compiler) {
   }, options, {
     // Default Glob is any directory level of scss and/or sass file,
     // under webpack's context and specificity changed via globbing patterns
-    files: arrify(options.files || '**/*.s?(c|a)ss').map(function (file) {
+    files: arrify(options.files || defaultFilesGlob).map(function (file) {
       return path.join(context, '/', file);
     }),
     context: context

lib/constants.js

@@ -0,0 +1,5 @@
+'use strict';
+
+module.exports = {
+  defaultFilesGlob: '**/*.s?(c|a)ss'
+};

lib/lint-dirty-modules-plugin.js

@@ -1,25 +1,25 @@
 'use strict';
-var minimatch = require('minimatch');
-var reduce = require('lodash.reduce');
+
 var assign = require('object-assign');
+var defaultTo = require('lodash.defaultto');
+var minimatch = require('minimatch');
 var runCompilation = require('./run-compilation');
 
 /**
  * Binds callback with provided options and stores initial values.
  *
  * @param compiler - webpack compiler object
  * @param options - stylelint nodejs options
- * @param callback <function(options, compilitaion)> - callback to call on emit
  */
 function LintDirtyModulesPlugin (compiler, options) {
   this.startTime = Date.now();
   this.prevTimestamps = {};
   this.isFirstRun = true;
   this.compiler = compiler;
   this.options = options;
-  compiler.plugin('emit',
-    this.lint.bind(this) // bind(this) is here to prevent context overriding by webpack
-  );
+
+  // bind(this) is here to prevent context overriding by webpack
+  compiler.plugin('emit', this.lint.bind(this));
 }
 
 /**
@@ -31,16 +31,18 @@ function LintDirtyModulesPlugin (compiler, options) {
  * @param callback - to be called when execution is done
  * @returns {*}
  */
-LintDirtyModulesPlugin.prototype.lint = function (compilation, callback) {
+LintDirtyModulesPlugin.prototype.lint = function lint (compilation, callback) {
   if (this.isFirstRun) {
     this.isFirstRun = false;
     this.prevTimestamps = compilation.fileTimestamps;
     return callback();
   }
+
   var dirtyOptions = assign({}, this.options);
   var glob = dirtyOptions.files.join('|');
   var changedFiles = this.getChangedFiles(compilation.fileTimestamps, glob);
   this.prevTimestamps = compilation.fileTimestamps;
+
   if (changedFiles.length) {
     dirtyOptions.files = changedFiles;
     runCompilation.call(this, dirtyOptions, this.compiler, callback);
@@ -57,18 +59,16 @@ LintDirtyModulesPlugin.prototype.lint = function (compilation, callback) {
  * @param fileTimestamps - an object with keys as filenames and values as their timestamps.
  * e.g. {'/filename.scss': 12444222000}
  * @param glob - glob pattern to match files
+ * @returns {Array} list of globs that contain changed files
  */
-LintDirtyModulesPlugin.prototype.getChangedFiles = function (fileTimestamps, glob) {
-  return reduce(fileTimestamps, function (changedStyleFiles, timestamp, filename) {
-    // Check if file has been changed first ...
-    if ((this.prevTimestamps[filename] || this.startTime) < (fileTimestamps[filename] || Infinity) &&
-      // ... then validate by the glob pattern.
-      minimatch(filename, glob, {matchBase: true})
-    ) {
-      changedStyleFiles = changedStyleFiles.concat(filename);
-    }
-    return changedStyleFiles;
-  }.bind(this), []);
+LintDirtyModulesPlugin.prototype.getChangedFiles = function getChangedFiles (fileTimestamps, glob) {
+  return Object.keys(fileTimestamps).filter(function (filename) {
+    return hasFileChanged.call(this, filename, fileTimestamps[filename]) && minimatch(filename, glob, {matchBase: true});
+  }.bind(this));
 };
 
+function hasFileChanged (filename, timestamp) {
+  return defaultTo(this.prevTimestamps[filename], this.startTime) < defaultTo(timestamp, Infinity);
+}
+
 module.exports = LintDirtyModulesPlugin;

package.json

@@ -33,7 +33,7 @@
   "dependencies": {
     "arrify": "^1.0.1",
     "chalk": "^1.1.3",
-    "lodash.reduce": "^4.6.0",
+    "lodash.defaultto": "^4.14.0",
     "minimatch": "^3.0.3",
     "object-assign": "^4.1.0",
     "stylelint": "^7.7.0"

test/index.js → test/index.test.js

@@ -7,12 +7,11 @@ var webpack = require('./helpers/webpack');
 var baseConfig = require('./helpers/base-config');
 
 var configFilePath = getPath('./.stylelintrc');
-require('./lib/lint-dirty-modules-plugin');
 
 describe('stylelint-webpack-plugin', function () {
   it('works with a simple file', function () {
     var config = {
-      context: './test/fixtures/test1',
+      context: './test/fixtures/lint-free',
       entry: './index'
     };
 
@@ -25,7 +24,7 @@ describe('stylelint-webpack-plugin', function () {
 
   it('sends errors to the errors output only', function () {
     var config = {
-      context: './test/fixtures/test3',
+      context: './test/fixtures/single-error',
       entry: './index'
     };
 
@@ -38,7 +37,7 @@ describe('stylelint-webpack-plugin', function () {
 
   it('fails on errors when asked to', function () {
     var config = {
-      context: './test/fixtures/test3',
+      context: './test/fixtures/single-error',
       entry: './index',
       plugins: [
         new StyleLintPlugin({
@@ -58,15 +57,15 @@ describe('stylelint-webpack-plugin', function () {
 
   it('works with multiple source files', function () {
     var config = {
-      context: './test/fixtures/test7',
+      context: './test/fixtures/multiple-sources',
       entry: './index'
     };
 
     return pack(assign({}, baseConfig, config))
       .then(function (stats) {
         expect(stats.compilation.errors).to.have.length(1);
-        expect(stats.compilation.errors[0]).to.contain('test/fixtures/test7/_second.scss');
-        expect(stats.compilation.errors[0]).to.contain('test/fixtures/test7/test.scss');
+        expect(stats.compilation.errors[0]).to.contain('test/fixtures/multiple-sources/_second.scss');
+        expect(stats.compilation.errors[0]).to.contain('test/fixtures/multiple-sources/test.scss');
       });
   });
 
@@ -83,46 +82,74 @@ describe('stylelint-webpack-plugin', function () {
       });
   });
 
-  it('works without StyleLintPlugin configuration but posts warning .stylelintrc file not found', function () {
+  // TODO use snapshots to ensure something is printed to the console
+  it.skip('sends messages to console when quiet prop set to false', function () {
     var config = {
-      context: './test/fixtures/test9',
+      context: './test/fixtures/syntax-error',
       entry: './index',
       plugins: [
-        new StyleLintPlugin()
+        new StyleLintPlugin({
+          configFile: configFilePath,
+          quiet: true
+        })
       ]
     };
 
     return pack(assign({}, baseConfig, config))
       .then(function (stats) {
-        expect(stats.compilation.errors).to.have.length(0);
+        expect(stats.compilation.errors).to.have.length(1);
         expect(stats.compilation.warnings).to.have.length(0);
       });
   });
 
-  // TODO use snapshots to ensure something is printed to the console
-  it.skip('sends messages to console when quiet prop set to false', function () {
+  it('fails when .stylelintrc is not a proper format', function () {
     var config = {
-      context: './test/fixtures/syntax-error',
       entry: './index',
+      context: './test/fixtures/single-error',
       plugins: [
         new StyleLintPlugin({
-          configFile: configFilePath,
+          configFile: getPath('./.badstylelintrc'),
           quiet: true
         })
       ]
     };
 
     return pack(assign({}, baseConfig, config))
-      .then(function (stats) {
-        expect(stats.compilation.errors).to.have.length(1);
-        expect(stats.compilation.warnings).to.have.length(0);
+      .then(expect.fail)
+      .catch(function (err) {
+        expect(err.message).to.contain('Failed to parse').and.contain('as JSON');
       });
   });
 
+  context('without StyleLintPlugin configuration', function () {
+    var config = {
+      context: './test/fixtures/lint-free',
+      entry: './index',
+      plugins: [
+        new StyleLintPlugin()
+      ]
+    };
+
+    it('works by using stylelint#cosmiconfig under the hood', function () {
+      return pack(assign({}, baseConfig, config))
+        .then(function (stats) {
+          expect(stats.compilation.errors).to.have.length(0);
+          expect(stats.compilation.warnings).to.have.length(0);
+        });
+    });
+
+    it('finds the right stylelintrc', function () {
+      return pack(assign({}, baseConfig, config, { context: './test/fixtures/rule-warning' }))
+        .then(function (stats) {
+          expect(stats.compilation.warnings).to.have.length(1);
+        });
+    });
+  });
+
   context('interop with NoErrorsPlugin', function () {
     it('works when failOnError is false', function () {
       var config = {
-        context: './test/fixtures/test3',
+        context: './test/fixtures/single-error',
         entry: './index',
         plugins: [
           new StyleLintPlugin({
@@ -139,9 +166,9 @@ describe('stylelint-webpack-plugin', function () {
         });
     });
 
-    it('throws when failOnError is true', function () {
+    context('when failOnError is true', function () {
       var config = {
-        context: './test/fixtures/test3',
+        context: './test/fixtures/single-error',
         entry: './index',
         plugins: [
           new StyleLintPlugin({
@@ -153,44 +180,34 @@ describe('stylelint-webpack-plugin', function () {
         ]
       };
 
-      return pack(assign({}, baseConfig, config))
-        .catch(function (err) {
-          expect(err).to.be.instanceof(Error);
-        });
-    });
-  });
-
-  it('fails when .stylelintrc is not a proper format', function () {
-    var config = {
-      entry: './index',
-      context: './test/fixtures/test3',
-      plugins: [
-        new StyleLintPlugin({
-          configFile: getPath('./.badstylelintrc'),
-          quiet: true
-        })
-      ]
-    };
+      it('throws when there is an error', function () {
+        return pack(assign({}, baseConfig, config))
+          .then(expect.fail)
+          .catch(function (err) {
+            expect(err).to.be.instanceof(Error);
+          });
+      });
 
-    return pack(assign({}, baseConfig, config))
-      .then(expect.fail)
-      .catch(function (err) {
-        expect(err.message).to.contain('Failed to parse').and.contain('as JSON');
+      it('does not throw when there are only warnings', function () {
+        return pack(assign({}, baseConfig, config, { context: './test/fixtures/rule-warning' }))
+          .then(function (stats) {
+            expect(stats.compilation.warnings).to.have.length(1);
+          });
       });
+    });
   });
 
   context('lintDirtyModulesOnly flag is enabled', function () {
     it('skips linting on initial run', function () {
       var config = {
-        context: './test/fixtures/test3',
+        context: './test/fixtures/single-error',
         entry: './index',
         plugins: [
           new StyleLintPlugin({
             configFile: configFilePath,
             quiet: true,
             lintDirtyModulesOnly: true
-          }),
-          new webpack.NoErrorsPlugin()
+          })
         ]
       };