Issue #30 - add lint dirty files only flag #53

sergesemashko · 2017-01-04T22:37:59Z

add a flag requested in the Lint dirty files only #30
alpha-sorted options in readme

@JaKXz, please, take a look

coveralls · 2017-01-04T22:39:34Z

Coverage decreased (-22.6%) to 72.881% when pulling 4a54385 on sergesemashko:lintdirtyfiles into ffbab73 on vieron:master.

JaKXz · 2017-01-05T02:37:24Z

@sergesemashko thanks for taking a crack at this. My first question is from the coveralls failure: can you think of ways to test this?

Otherwise I like the way this is going.

JaKXz · 2017-01-05T02:43:39Z

index.js

+
+      var changedFiles = Object.keys(compilation.fileTimestamps).filter(function (watchfile) {
+        return (this.prevTimestamps[watchfile] || this.startTime) < (compilation.fileTimestamps[watchfile] || Infinity);
+      }.bind(this)).filter(minimatch.filter(globPatterb.join('|'), {matchBase: true}));


Can this be re-writtern with a reduce on Object.keys(compilation.fileTimestamps)? it would be a bit clearer IMO and would be fewer calls/binds. changedFiles just needs to be an updated glob or array of globs that's passed to stylelint and ultimately to node-glob.

You could even bring in lodash.reduce and split out this function into a private module in lib, since it's quite complex, and probably will have some compatibility issues with webpack 2 (#46).

Sorry, did not quite get where to put reduce. Would it be some like this?

function getChangedFiles(plugin, compilation, glob) { var updatedStyles = reduce(compilation.fileTimestamps, function (changedStyleFiles, timestamp, filename) { // Check if file has been changed first ... if ((plugin.prevTimestamps[filename] || plugin.startTime) < (compilation.fileTimestamps[filename] || Infinity) // ... then validate by the glob pattern. && minimatch(glob, {matchBase: true}) ) { changedStyleFiles.push(filename); } return changedStyleFiles; }, []); return updatedStyles; }

An example without reduce:

function getChangedFiles(plugin, compilation, glob) { // Filter only changed files first ... var updated = Object.keys(compilation.fileTimestamps).filter(function (filename) { return (plugin.prevTimestamps[filename] || plugin.startTime) < (compilation.fileTimestamps[filename] || Infinity); }); // ... then filter files by the glob pattern. var updatedStyles = updated.filter(minimatch.filter(glob, {matchBase: true})); return updatedStyles; }

The actual call would look like:

function apply(options, compiler) { var plugin = this; ... compiler.plugin('emit', function (compilation, callback) { ... var changedFiles = getChangedFiles(plugin, compilation, globPatterb.join('|')) ... } ... }

@JaKXz, Let me what option you'd prefer to go with

@sergesemashko I prefer the slightly purer first version, but I would use .concat instead of .push. I would also just return the result of reduce(obj, fn, []).

Another nitpick: would it be clearer to check if compilation.fileTimestamps[filename] is falsy? Probably not, but what do you think?

Also, the emit function should be bound to this so that it's consistent with the other lib functions.

reduce iterates through the initial keys of compilation.fileTimestamps, so filename is the key of each entry and is there for sure 😉

👍 let's see how the implementation goes and discuss further there :)

JaKXz · 2017-01-05T02:44:18Z

package.json

@@ -32,6 +32,7 @@
  "dependencies": {
    "arrify": "^1.0.1",
    "chalk": "^1.1.3",
+    "minimatch": "^3.0.3",


please use yarn add minimatch so the yarn.lock file is updated.

@sergesemashko just curious, is the filter fn that you're using from minimatch available from node-glob? could remove unnecessary dependencies :)

good catch. Minimatch is actually a dependency of node-glob which doesn't export any kind of function to use here :( But I ran yarn add minimatch, surprisingly, I did not cause any updates of yarn.lock, because latest minimatch version is already installed in ./node_modules.

sergesemashko · 2017-01-05T11:38:37Z

@JaKXz, thanks for the feedback.
As this new piece of code is executed after initial webpack run the only one idea I have to test it is to change file contents programmatically in test case or create new file after initial webpack run. I'll take a look and see where I can get with this.

- Add tests for lintDirtyFilesOnly flag

…nto lintdirtyfiles # Conflicts: # package.json # test/index.js # yarn.lock

sergesemashko · 2017-01-08T14:18:36Z

Separate logic for extracting changed files
Add tests for lintDirtyFilesOnly

@JaKXz, please, take a look once again.

coveralls · 2017-01-08T14:20:05Z

Changes Unknown when pulling f3e08b9 on sergesemashko:lintdirtyfiles into ** on vieron:master**.

sergesemashko · 2017-01-08T14:22:25Z

test/index.js

+        } else
+        if (runsCount === 2) {
+          // Check that on JS file change styleling is not triggered.
+          expect(stats.compilation.errors).to.have.length(0);


@JaKXz, initially, I had separate test for testing non-style file change. But seems like it's not possible to run multiple webpack watch instances simultaneously as tests are executed in parallel mode. So, I had to squash all watch cases into one.

Can we just mock out the complexity and assert that we are seeing the right thing? I don't want to test webpack (the framework).

It's definitely going to be easier to write a unit test for get-changed-files. I would start there in a new test file, and we can come back to the integration test later.

JaKXz

Great work so far, we just need to iron out some details and then this should be mergeable :)

JaKXz · 2017-01-08T17:31:00Z

index.js

+
+    compiler.plugin('emit', function (compilation, callback) {
+      var dirtyOptions = assign({}, options);
+      var globPatterb = dirtyOptions.files;


JaKXz · 2017-01-08T17:34:43Z

index.js

+    compiler.plugin('emit', function (compilation, callback) {
+      var dirtyOptions = assign({}, options);
+      var globPatterb = dirtyOptions.files;
+      var changedFiles = getChangedFiles(this, compilation, globPatterb.join('|'));


Can you pass an object here, naming the parameters? At some point I'll rewrite this plugin in the latest language features and use destructuring. For now it'll make it clearer why these args are being passed in.

👍🏽 thanks for taking my comments well. Liking the way this is written :)

JaKXz · 2017-01-08T17:36:03Z

test/helpers/watch.js

@@ -0,0 +1,19 @@
+'use strict';
+
+var webpack = require('webpack');


Please rebase to capture our changes to support webpack 1&2.

JaKXz · 2017-01-08T17:39:26Z

test/index.js

+        } else
+        if (runsCount === 2) {
+          // Check that on JS file change styleling is not triggered.
+          expect(stats.compilation.errors).to.have.length(0);


Can we just mock out the complexity and assert that we are seeing the right thing? I don't want to test webpack (the framework).

It's definitely going to be easier to write a unit test for get-changed-files. I would start there in a new test file, and we can come back to the integration test later.

…nto lintdirtyfiles # Conflicts: # lib/run-compilation.js # test/index.js

coveralls · 2017-01-09T12:38:57Z

Coverage decreased (-2.8%) to 92.754% when pulling e0f19c7 on sergesemashko:lintdirtyfiles into 9aab3fa on vieron:master.

sergesemashko · 2017-01-09T12:41:35Z

@JaKXz, again, thanks for the feedback.
Didn't want to get rid of integration test completely (spent quite a lot of time on it:)), so I commented it out for later use with supporting comments.
Please, take a look once again.

JaKXz · 2017-01-10T00:24:10Z

test/index.js

+    */
+  });
+
+  context('getChangedFiles', function () {


This should be in its own file [since getChangedFiles is its own module]. 👍 looks great though 😄

JaKXz · 2017-01-10T00:32:30Z

test/index.js

+          - `../helpers/watch`
+          - `path`
+     // Webpack integration test.
+     it('lints only changed files in watch mode', function (done) {


This could be marked with it.skip so we don't have to commit commented out code. ~~In fact it could be inside it's own context block.~~ missed that above, my bad!

In that block we should stub out the parts of webpack that we can assume will operate as a black box.

Then, I believe the watchCallback can be spied on and assertions can be made on individual calls of that fn. I don't mean to prescribe sinon as a solution since other libraries exist (e.g. I prefer testdouble) but I just wanted to demonstrate that I think it's possible to write this test without all the imperative code and then we can merge it :)

good call, I'll try to mock blackbox logic then

JaKXz · 2017-01-10T00:41:59Z

test/fixtures/lint-dirty-files/updated-bad.scss

+     #stuff
+{display: block
+;;
+   }


another nitpick: please make sure your editor is using EditorConfig - thanks!

JaKXz · 2017-01-10T00:43:23Z

index.js

@@ -8,11 +8,11 @@ var formatter = require('stylelint').formatters.string;

 // Modules
 var runCompilation = require('./lib/run-compilation');
+var getChangedFiles = require('./lib/get-chaged-files');


JaKXz · 2017-01-10T00:44:43Z

lib/get-chaged-files.js

+ * against cached timestamps from previous run.
+ *
+ * @param plugin - stylelint-webpack-plugin this scopr
+ * @param compilation


Do we need the entire compilation object here? If I haven't misread the code looks like we're just using the fileTimestamps property so I would just accept that as a param.

you are right, fileTimestamps should be enough

JaKXz · 2017-01-10T06:54:37Z

@sergesemashko one thing I would suggest in general is to try to extract as many operations into pure "components" or functions that can be easily tested (like get-changed-files). Then maintenance (e.g. my life) becomes much easier because we just have to take care of small tested units rather than maintain tests of the integration of this complex logic with webpack. Hope that helps!

…nto lintdirtyfiles # Conflicts: # index.js # lib/run-compilation.js # package.json # yarn.lock

coveralls · 2017-01-15T23:11:43Z

Coverage increased (+1.09%) to 98.649% when pulling 4697c3e on sergesemashko:lintdirtyfiles into 96781a3 on JaKXz:master.

coveralls · 2017-01-15T23:11:43Z

Coverage increased (+1.09%) to 98.649% when pulling 4697c3e on sergesemashko:lintdirtyfiles into 96781a3 on JaKXz:master.

sergesemashko · 2017-01-15T23:35:23Z

What's done:

extracted logic into separate LintDirtyModulesOnlyPlugin, so it can keep its own state for runs count and previous timestamps
installed testdouble.js
removed webpack integration testing
I hope, I've managed to create real "unit" tests :) (it was fun using testdouble)

@JaKXz, please, take a look once again

JaKXz

@sergesemashko great work! 🥇

I'm going to merge this, would you mind installing stylelint-webpack-plugin#master in your project(s) and kick the tires with both the lintDirtyModulesOnly flag on and off?

sergesemashko · 2017-01-16T22:39:10Z

@JaKXz, sure, I'll take a look either today or tomorrow

* resolves #30 - add lint dirty files only flag * Separate logic to extract changed style files. - Add tests for lintDirtyFilesOnly flag * Added a test case for non style file change * Add unit test for getChangedFiles * Remove unused dependencies * Quiet Stylelint during tests * Refactore lint dirty modules into plugin in separate file * Add testdouble * Fix lint warnings

Issue webpack-contrib#30 - add lint dirty files only flag

4a54385

JaKXz reviewed Jan 5, 2017

View reviewed changes

sergesemashko added 3 commits January 8, 2017 09:01

Separate logic to extract changed style files.

0f976ad

- Add tests for lintDirtyFilesOnly flag

Added a test case for non style file change

2b1083f

Merge branch 'master' of github.com:vieron/stylelint-webpack-plugin i…

f3e08b9

…nto lintdirtyfiles # Conflicts: # package.json # test/index.js # yarn.lock

sergesemashko commented Jan 8, 2017

View reviewed changes

JaKXz suggested changes Jan 8, 2017

View reviewed changes

sergesemashko added 4 commits January 9, 2017 07:27

Add unit test for getChangedFiles

a3c6397

Remove unused dependencies

02c1feb

Merge branch 'master' of github.com:vieron/stylelint-webpack-plugin i…

fa6ee14

…nto lintdirtyfiles # Conflicts: # lib/run-compilation.js # test/index.js

Quiet Stylelint during tests

e0f19c7

JaKXz reviewed Jan 10, 2017

View reviewed changes

JaKXz force-pushed the master branch from 3a1f134 to 4d55fb4 Compare January 12, 2017 02:53

sergesemashko added 4 commits January 15, 2017 18:02

Refactore lint dirty modules into plugin in separate file

f34497a

Merge branch 'master' of github.com:vieron/stylelint-webpack-plugin i…

8ea8ef6

…nto lintdirtyfiles # Conflicts: # index.js # lib/run-compilation.js # package.json # yarn.lock

Add testdouble

9423cdc

Fix lint warnings

4697c3e

JaKXz approved these changes Jan 16, 2017

View reviewed changes

JaKXz merged commit 7e60e9b into webpack-contrib:master Jan 16, 2017

JaKXz mentioned this pull request Jan 16, 2017

Refactor/lint dirty modules only #59

Merged

		@@ -0,0 +1,19 @@
		'use strict';

		var webpack = require('webpack');

Issue #30 - add lint dirty files only flag #53

Issue #30 - add lint dirty files only flag #53

Conversation

sergesemashko commented Jan 4, 2017 • edited Loading

coveralls commented Jan 4, 2017 • edited Loading

JaKXz commented Jan 5, 2017

JaKXz Jan 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sergesemashko Jan 5, 2017 • edited Loading

Choose a reason for hiding this comment

JaKXz Jan 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sergesemashko Jan 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JaKXz Jan 5, 2017 • edited Loading

Choose a reason for hiding this comment

sergesemashko Jan 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sergesemashko commented Jan 5, 2017

sergesemashko commented Jan 8, 2017

coveralls commented Jan 8, 2017 • edited Loading

sergesemashko Jan 8, 2017 • edited Loading

Choose a reason for hiding this comment

JaKXz Jan 8, 2017 • edited Loading

Choose a reason for hiding this comment

JaKXz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JaKXz Jan 8, 2017 • edited Loading

Choose a reason for hiding this comment

coveralls commented Jan 9, 2017 • edited Loading

sergesemashko commented Jan 9, 2017

JaKXz Jan 10, 2017 • edited Loading

Choose a reason for hiding this comment

JaKXz Jan 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JaKXz commented Jan 10, 2017

coveralls commented Jan 15, 2017

coveralls commented Jan 15, 2017

sergesemashko commented Jan 15, 2017

JaKXz left a comment

Choose a reason for hiding this comment

sergesemashko commented Jan 16, 2017

sergesemashko commented Jan 4, 2017 •

edited

Loading

coveralls commented Jan 4, 2017 •

edited

Loading

JaKXz Jan 5, 2017 •

edited

Loading

sergesemashko Jan 5, 2017 •

edited

Loading

JaKXz Jan 5, 2017 •

edited

Loading

sergesemashko Jan 7, 2017 •

edited

Loading

JaKXz Jan 5, 2017 •

edited

Loading

sergesemashko Jan 7, 2017 •

edited

Loading

coveralls commented Jan 8, 2017 •

edited

Loading

sergesemashko Jan 8, 2017 •

edited

Loading

JaKXz Jan 8, 2017 •

edited

Loading

JaKXz Jan 8, 2017 •

edited

Loading

coveralls commented Jan 9, 2017 •

edited

Loading

JaKXz Jan 10, 2017 •

edited

Loading

JaKXz Jan 10, 2017 •

edited

Loading