Fixes #65: Add `emitErrors` option #66

Vinnl · 2017-01-30T20:42:19Z

This option allows you to log errors as warnings, preventing
Webpack dev server from aborting compilation.

coveralls · 2017-01-30T20:43:47Z

Coverage increased (+0.03%) to 98.718% when pulling f7d1111 on Vinnl:65-emitErrors into ce6b9ab on JaKXz:master.

coveralls · 2017-01-30T20:43:47Z

Coverage increased (+0.03%) to 98.718% when pulling f7d1111 on Vinnl:65-emitErrors into ce6b9ab on JaKXz:master.

JaKXz

Thank you for the contribution! Especially for making the issue plus writing tests! :) 💯

This looks good to me. I might have a few nitpicks that I'll add later on but I'll merge this soon. If you have allowed edits from maintainers that'd be perfect. Thanks again!

JaKXz · 2017-01-31T06:28:03Z

One quick question though, can you make sure this works with the new lintDirtyModulesOnly option?

Vinnl · 2017-01-31T07:38:49Z

<3 tests :)

I'll try to see what I can do tonight. I've allowed edits by the maintainer, but if you add the nitpicks in comments before tonight I can incorporate those as well if you like.

JaKXz · 2017-01-31T08:38:48Z

README.md

@@ -45,6 +45,7 @@ See [stylelint options](http://stylelint.io/user-guide/node-api/#options) for th
 * `configFile`: You can change the config file location. Default: (`undefined`), handled by [stylelint's cosmiconfig module](http://stylelint.io/user-guide/configuration/).
 * `context`: String indicating the root of your SCSS files. Default: inherits from webpack config.
 * `failOnError`: Have Webpack's build process die on error. Default: `false`
+* `emitErrors`: Display Stylelint errors as actual errors, rather than just warnings. Default: `true`


Please keep this list in alphabetical order

JaKXz · 2017-01-31T08:39:31Z

test/index.test.js

@@ -57,6 +57,65 @@ describe('stylelint-webpack-plugin', function () {
      });
  });

+  it('emits errors as warnings when asked to', function () {


Please use a context for the emitErrors tests

Vinnl · 2017-01-31T16:04:12Z

(Short update: something came in between, so I probably won't be able to get to this before Friday. Sorry!)

coveralls · 2017-02-01T18:08:44Z

Coverage increased (+0.03%) to 98.718% when pulling 5da0ed8 on Vinnl:65-emitErrors into ce6b9ab on JaKXz:master.

Vinnl · 2017-02-01T18:11:10Z

@JaKXz I found a moment to squeeze this in, two questions though:

How do I write a test for the lintDirtyModulesOnly option? I couldn't figure that out by myself, unfortunately. When I tested it in my own project, I did observe that the errors were still being marked as warnings (as desired) with the option on, even after editing it and Webpack automatically recompiling. The README does state, though, that linting is skipped on start, which did not happen - it immediately reported the warning when running my app for the first time.
What do you mean by using a context, exactly? That I create a separate fixture for each test? If so, do I need to add a log outputting a test number as well, and does that mean I have to update the numbers of the other fixtures as well? (And if so, do I number them in their order of appearance in the test script? But then what about fixtures that are used multiple times, such as single-error?)

I did update the param order in the readme, so that's something :P

Vinnl · 2017-02-12T16:55:54Z

@JaKXz Mind taking a look at my questions above? (Sorry for bugging, I just want to get this in and start using it before I lose motivation myself :P)

JaKXz

Thanks for the ping! I was distracted by other projects, my apologies.

there is a lint-dirty-modules.test.js file so you could start there. What I was thinking was more an integration test so it would probably belong in index.test.js and you would do that by having both enabled and verifying the output or the compilation stats [and output verified with testdouble] are correct.
when I say a context, I mean a mocha context. There are other examples in the test file. context is just an alias for describe so it helps to organize the different sections of the tests.

I too want to get this in ASAP - then I can release v0.6.1! 😄

JaKXz · 2017-02-12T18:04:01Z

lib/run-compilation.js

@@ -46,7 +46,11 @@ module.exports = function runCompilation (options, compiler, done) {
    }

    if (errors.length) {
-      compilation.errors.push(chalk.red(options.formatter(errors)));
+      if (options.emitErrors === false) {
+        compilation.warnings.push(chalk.yellow(options.formatter(errors)));


Would it make sense to indicate that these are still errors?

If you mean to display them in red, then that was my first try. However, it was rather confusing: usually the red means you have to immediately do something, and that the compilation has failed. Thus, if e.g. another, actual error occurred, one might think the style error would have to be fixed as well.

I therefore kept it yellow, since for all intents and purposes, when you've enabled this flag errors are warnings. I'd be willing to change this if you feel red is more appropriate or if you want to prefix it with something, though. (Or feel free to change it yourself.)

What I was thinking was more like a text header or something, but the colours also make sense. Probably a combination to indicate that the errors (in red) are being "ignored" but still reported.

Alright, better like this?

This option allows you to log errors as warnings, preventing Webpack dev server from aborting compilation.

coveralls · 2017-02-13T20:15:55Z

Coverage increased (+0.03%) to 98.718% when pulling 2c2de75 on Vinnl:65-emitErrors into c222b13 on JaKXz:master.

coveralls · 2017-02-13T20:15:55Z

Coverage increased (+0.03%) to 98.718% when pulling 2c2de75 on Vinnl:65-emitErrors into c222b13 on JaKXz:master.

Vinnl · 2017-02-13T20:20:06Z

Thanks for the context hint, I was confused by the other uses of the variable, and am not too familiar with Mocha. I've grouped all the emitErrors tests under one context. Do double-check the naming, though: all other contexts are for when a specific flag is enabled, whereas I added tests checking whether both enabling and disabling it works.

As for the dirty modules test: unfortunately, I did not manage to write a working test. I had a look at lint-dirty-modules.test.js, but am not familiar enough with the Webpack and stylelint-webpack-plugin internals to figure out how it works and how to adapt it for this use case without considerable effort :(

To compensate, I did find one potential issue in that test: it says fomatter instead of formatter(missing r) on line 35 :)

JaKXz · 2017-02-14T09:44:12Z

lib/run-compilation.js

@@ -46,7 +46,11 @@ module.exports = function runCompilation (options, compiler, done) {
    }

    if (errors.length) {
-      compilation.errors.push(chalk.red(options.formatter(errors)));
+      if (options.emitErrors === false) {
+        compilation.warnings.push(chalk.yellow(options.formatter(errors)));


What I was thinking was more like a text header or something, but the colours also make sense. Probably a combination to indicate that the errors (in red) are being "ignored" but still reported.

JaKXz · 2017-02-14T09:46:03Z

test/index.test.js

+        });
+    });
+
+    it('does not emit errors as warnings when not asked to', function () {


I would say by default, or even better when emitErrors is not provided.

JaKXz · 2017-02-18T19:39:21Z

@Vinnl this looks good so far! Once CI is passing I'll be merging this in and releasing a new version :)

JaKXz · 2017-02-18T19:41:30Z

lib/run-compilation.js

-      compilation.errors.push(chalk.red(options.formatter(errors)));
+      if (options.emitErrors === false) {
+        var suppressionWarning = 'Stylelint error, reported as warning because emitErrors is set to false';
+        compilation.warnings.push(chalk.yellow(options.formatter(errors + ' - ' + suppressionWarning)));


looks like errors (the thing passed to style lint's default formatter) is meant to be an array, so you should probably use errors.concat('\n\t - ' + suppressionWarning) or something to that effect.

So that unexpectedly took me down a rabbit hole :)

Basically, I shouldn't have assumed errors to be a simple string. It's actually an array of objects containing details about stylelint errors (and warnings).

Of note is that it also contains a property severity with a value of either warning or error. This will in turn cause the formatter to display the warning with either a ⚠ or a ✖ prefixed, respectively, effectively communicating to the user when they were actually errors.

Thus, I've removed the message again and added two tests that ensure that these icons are actually prefixed when appropriate. Hopefully that's OK with you.

And given that I now understood how the stylelint errors are passed through to Webpack, I actually refactored it a bit to unmark files as containing errors, rather than messing with the stats reporter.

Two things to consider:

I've used Object.assign to keep the downgradeErrors function pure. This is only supported in Node 4+, so I've added that to package.json, if that's OK.

The two new tests are dependent on the formatter marking errors and warnings using the mentioned icons. I'm not too happy with that, but I guess it's reasonable to not expect those to change suddenly.

coveralls · 2017-02-19T11:08:20Z

Coverage increased (+0.07%) to 98.75% when pulling 7d0c275 on Vinnl:65-emitErrors into c222b13 on JaKXz:master.

coveralls · 2017-02-19T11:08:20Z

Coverage increased (+0.07%) to 98.75% when pulling 7d0c275 on Vinnl:65-emitErrors into c222b13 on JaKXz:master.

coveralls · 2017-02-19T11:08:20Z

Coverage increased (+0.07%) to 98.75% when pulling 7d0c275 on Vinnl:65-emitErrors into c222b13 on JaKXz:master.

Previously, errors would simply be added to Webpack's list of warnings. With this commit, files that include errors actually no longer get marked as such (when `emitErrors` is false). This results in stylelint errors actually being added to the list of warnings, rather than to the list of errors, removing the need to special case them when compiling the compilation stats. Since the `severity` of these errors is still set to `error`, this will still be indicated to the user - it just will no longer abort compilation.

When `emitErrors` is set to false, errors no longer abort the compilation. However, the user might still want to know which warnings are the result of stylelint errors, and which are the results of stylelint warnings. These tests ensure that.

coveralls · 2017-02-19T11:16:24Z

Coverage increased (+0.07%) to 98.75% when pulling d065a72 on Vinnl:65-emitErrors into c222b13 on JaKXz:master.

coveralls · 2017-02-19T11:16:24Z

Coverage increased (+0.07%) to 98.75% when pulling d065a72 on Vinnl:65-emitErrors into c222b13 on JaKXz:master.

JaKXz

What you've done makes sense, thanks for taking another stab at it. :)

Just have a question that may make things simpler. Let me know what you think.

JaKXz · 2017-02-19T11:19:29Z

lib/run-compilation.js

+      if (options.emitErrors === false) {
+        results = downgradeErrors(results);
+      }
+
      warnings = results.filter(function (file) {
        return !file.errored && file.warnings && file.warnings.length;


why not just check if emitErrors is disabled here [and then have the errors array return empty]?

this way we don't have to go through results more than twice, and if emitErrors is false we'll only have to go through it once.

I'm not sure I completely understand your comment: why would we be going through the results more than twice?

As for just leaving the errors array empty: due to the check for !file.errored when compiling the warnings array, errors wouldn't be added to the list of warnings either. We could of course expand the check to (!file.errored && options.emitErrors !== false), and then another check for options.emitErrors before filling the errors array. That would make the code harder to follow I think, but if you prefer that I'd be fine with changing it.

JaKXz · 2017-02-19T11:21:06Z

package.json

@@ -24,6 +24,9 @@
  "bugs": {
    "url": "https://github.com/JaKXz/stylelint-webpack-plugin/issues"
  },
+  "engines": {
+    "node": ">=4.0.0"


while .travis.yml does not test against Node < 4, I feel as though I might as well have kept it there since we're not using any language features. Based on my comment above, I think we could keep it that way.

And if we can't for some reason, I'd rather use the object-assign ponyfill for now.

Allright, I'll remove or polyfill this depending on the resolution above.

coveralls · 2017-02-20T03:19:47Z

Coverage remained the same at 98.684% when pulling 7b17813 on Vinnl:65-emitErrors into c222b13 on JaKXz:master.

JaKXz · 2017-02-20T03:21:06Z

@Vinnl I made the changes I was thinking directly on your branch, and it seems like the tests pass just fine. It's probably a good time to make sure all the cases are covered...

Anyway, with that change I don't think the engines specification is necessary.

JaKXz · 2017-02-20T03:24:06Z

test/index.test.js

+        });
+    });
+
+    it('does not emit errors as warnings when `emitErrors` is not provided', function () {


I think this test is redundant, it should be covered in other places.

Yes, it is at the moment. I added it with the idea of atomic unit tests, each testing a specific property of the code. That way, if the expected behaviour of the other unit test changes, this property will still be tested.

Vinnl · 2017-02-20T07:14:53Z

@JaKXz Ah, I see what you mean. Yes, that looks fine, and engines would indeed no longer be necessary.

coveralls · 2017-02-20T08:03:36Z

Coverage remained the same at 98.684% when pulling 174a33e on Vinnl:65-emitErrors into c222b13 on JaKXz:master.

coveralls · 2017-02-20T08:09:05Z

Coverage remained the same at 98.684% when pulling c0e1bd1 on Vinnl:65-emitErrors into c222b13 on JaKXz:master.

coveralls · 2017-02-20T08:16:28Z

Coverage remained the same at 98.684% when pulling 1ed717f on Vinnl:65-emitErrors into c222b13 on JaKXz:master.

JaKXz · 2017-02-20T08:30:08Z

So I got a bit carried away and fixed things up [because I'm excited about this feature] :) please do give master a shot in a more complex project and see how it turns out. I will be releasing it as a minor bump since it is a new feature, so look for v0.7.0 soon-ish.

What would be extremely helpful would be another PR with a more complex fixture... perhaps it's time to introduce snapshot testing for the console output? hmmm.

Vinnl · 2017-02-20T19:57:58Z

Thanks for getting it in, much appreciated :)

It does indeed now report the errors as warnings on my actual project. What's interesting, though, is that when I remove emitErrors, it still does not fail the build when errors occur.

Which led me to install 0.6.0, where the same thing happens: even though there's a stylelint error in my CSS, the build is completed successfully. In 0.5.0 (and in 0.4.2, which I was apparently still using), errors do block the build, as expected.

Resolves #65. This option allows you to log errors as warnings, preventing webpack-dev-server from aborting compilation. * tests: fix typo in lint-dirty-modules test

Vinnl mentioned this pull request Jan 30, 2017

Feature request: warnings rather than errors #65

Closed

JaKXz approved these changes Jan 31, 2017

View reviewed changes

JaKXz reviewed Jan 31, 2017

View reviewed changes

Vinnl force-pushed the 65-emitErrors branch from f7d1111 to 5da0ed8 Compare February 1, 2017 18:06

JaKXz suggested changes Feb 12, 2017

View reviewed changes

JaKXz reviewed Feb 12, 2017

View reviewed changes

Vinnl added 2 commits February 13, 2017 20:22

Fixes webpack-contrib#65: Add emitErrors option

70820f7

This option allows you to log errors as warnings, preventing Webpack dev server from aborting compilation.

Move tests for the emitErrors flag to a separate context

2c2de75

Vinnl force-pushed the 65-emitErrors branch from 5da0ed8 to 2c2de75 Compare February 13, 2017 20:13

JaKXz reviewed Feb 14, 2017

View reviewed changes

Clarify test description

7db3928

JaKXz reviewed Feb 18, 2017

View reviewed changes

Vinnl force-pushed the 65-emitErrors branch from ace5efe to 7d0c275 Compare February 19, 2017 11:04

Vinnl added 2 commits February 19, 2017 12:14

Vinnl force-pushed the 65-emitErrors branch from 7d0c275 to d065a72 Compare February 19, 2017 11:14

JaKXz reviewed Feb 19, 2017

View reviewed changes

fix: simplify logic for emitErrors

7b17813

JaKXz reviewed Feb 20, 2017

View reviewed changes

JaKXz added 2 commits February 20, 2017 03:01

tests: simplify emitErrors tests

9524767

chore(package): remove engines specification [skip ci]

174a33e

tests: fix typo in lint-dirty-modules test

c0e1bd1

JaKXz added 2 commits February 20, 2017 03:12

tests: add smoke test w/ lintDirtyModulesOnly

2941c07

style: whitespace

1ed717f

JaKXz approved these changes Feb 20, 2017

View reviewed changes

JaKXz merged commit a2e5a6d into webpack-contrib:master Feb 20, 2017

joshwiens pushed a commit that referenced this pull request Mar 31, 2018

feat: add emitErrors option (#66)

ac606f5

Resolves #65. This option allows you to log errors as warnings, preventing webpack-dev-server from aborting compilation. * tests: fix typo in lint-dirty-modules test

Fixes #65: Add emitErrors option #66

Fixes #65: Add emitErrors option #66

Conversation

Vinnl commented Jan 30, 2017

coveralls commented Jan 30, 2017

coveralls commented Jan 30, 2017 • edited Loading

JaKXz left a comment

Choose a reason for hiding this comment

JaKXz commented Jan 31, 2017 • edited Loading

Vinnl commented Jan 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vinnl commented Jan 31, 2017

coveralls commented Feb 1, 2017 • edited Loading

Vinnl commented Feb 1, 2017

Vinnl commented Feb 12, 2017

JaKXz left a comment • 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

coveralls commented Feb 13, 2017

coveralls commented Feb 13, 2017 • edited Loading

Vinnl commented Feb 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JaKXz commented Feb 18, 2017

JaKXz Feb 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Feb 19, 2017

coveralls commented Feb 19, 2017

coveralls commented Feb 19, 2017

coveralls commented Feb 19, 2017

coveralls commented Feb 19, 2017 • edited Loading

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

coveralls commented Feb 20, 2017 • edited Loading

JaKXz commented Feb 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vinnl commented Feb 20, 2017

coveralls commented Feb 20, 2017 • edited Loading

coveralls commented Feb 20, 2017 • edited Loading

coveralls commented Feb 20, 2017 • edited Loading

JaKXz commented Feb 20, 2017 • edited Loading

Vinnl commented Feb 20, 2017

Fixes #65: Add `emitErrors` option #66

Fixes #65: Add `emitErrors` option #66

coveralls commented Jan 30, 2017 •

edited

Loading

JaKXz commented Jan 31, 2017 •

edited

Loading

coveralls commented Feb 1, 2017 •

edited

Loading

JaKXz left a comment •

edited

Loading

coveralls commented Feb 13, 2017 •

edited

Loading

JaKXz Feb 18, 2017 •

edited

Loading

coveralls commented Feb 19, 2017 •

edited

Loading

coveralls commented Feb 20, 2017 •

edited

Loading

coveralls commented Feb 20, 2017 •

edited

Loading

coveralls commented Feb 20, 2017 •

edited

Loading

coveralls commented Feb 20, 2017 •

edited

Loading

JaKXz commented Feb 20, 2017 •

edited

Loading