fixes #133: Add Support for Webpack-Standard Errors #136

shellscape · 2018-02-21T15:21:10Z

This PR addresses #133 by pushing errors to compilation.warnings and compilations.errors that follow the standard (and by standard we mean presently most frequently used) webpack error message format. This format is parsed by webpack-stylish and future reporters until the RFC for revamping webpack errors is implemented.

This PR:

Adds StylelintPluginError in anticipation of future work. It inherits from WebpackError if the class exists (v2.5.0+).
Adds formatter-webpack.js which utilizes StylelintPluginError and returns an array of error objects.
Modifies run-compilation.js by consolidating the code for pushing warnings and errors to the compilation and handling the case in which a formatter returns an array instead of a string, while still accounting for a string (in the case of the default formatter).
Requires that a user set the options.formatter = 'webpack' in the options for the plugin in the webpack config. (This will be noted in webpack-stylish as well). The README for this project was updated with that info.

And as an added bonus you now have tests touching the options.formatter property.

It's worth noting that once webpack/webpack#6534 is implemented in it's final form, this plugin will start to throw warnings in the console for webpack@3, and errors for webpack@4, and will have to be updated again to follow that new standard.

add travis

npm: update dependencies

inherited from sass-webpack-plugin

npm: clean not used dependencies

npm: add version script

* Update stylelint dependency * Fix renamed rules in stylelint v7 * Add editorconfig * Simplify linter * Use chai array length assertion * Restructure with linting and coverage * Add some documentation for runCompilation and add missing use strict * Keep stylelint v7 in peerDependencies

* Update README.md

Add some jsdoc comments to the "public API"

- consistent tense in test suite - corrected editorconfig for markdown files - use `.have.length` assertion - add gitter badge

* Fixed warnings not being logged to console Warnings were not being logged unless there was an error in the css linting. * Corrected number of warnings returned

* It should work without configuration, just use default one * It should warn user if .stylelintrc file does not exit * Add warnings check

* Add micromatch * Use micromatch to check changes * Remove matchbase in micromatch

* style: correct td.verify calls to match documentation * style: use const/lets in lint-dirty-modules.test.js

Fixes downstream vulnerability: https://snyk.io/vuln/npm:braces:20180219

coveralls · 2018-02-21T15:24:40Z

Coverage decreased (-1.6%) to 97.087% when pulling 2f47abe on shellscape:master into 04e0f69 on JaKXz:master.

JaKXz

I think this is great! Thank you @shellscape for taking your time to do this and updating the docs & tests.

I have some questions and freshly picked nits. Let me know what you think.

JaKXz · 2018-02-21T23:14:49Z

README.md

+```js
+plugins: [
+  new StyleLintPlugin({
+    formatter: 'webpack'


I would prefer explicitly requireing the function, i.e. formatter: require('stylelint-webpack-plugin/lib/webpack-formatter') to keep the signature consistent and remove an (IMO) unnecessary branch in index.js. What do you think?

Experience on the core team points to that being another unnecessary step for users (that's been a very big public gripe recently)

JaKXz · 2018-02-21T23:15:21Z

index.js

@@ -10,6 +10,7 @@ var formatter = require('stylelint').formatters.string;
 var runCompilation = require('./lib/run-compilation');
 var LintDirtyModulesPlugin = require('./lib/lint-dirty-modules-plugin');
 var defaultFilesGlob = require('./lib/constants').defaultFilesGlob;
+var webpackFormatter = require('./lib/formatter-webpack');


minor nit, the filename should be kebab-cased version of the export, i.e. webpack-formatter.

if we put the context after the subject, then lint-dirty-modules-plugin should be named dirty-modules-plugin-lint as well ;) that seems inconsistent with the rest of the codebase

JaKXz · 2018-02-21T23:16:01Z

lib/formatter-webpack.js

@@ -0,0 +1,27 @@
+'use strict';
+
+const StylelintPluginError = require('./StylelintPluginError');


another minor nit, the filename should be kebab-cased version of the export, i.e. stylelint-plugin-error.

ah you're requesting that change in the classes file. that's pretty unusual for a file that exports a class :)

JaKXz · 2018-02-21T23:17:17Z

lib/StylelintPluginError.js

+
+module.exports = class StylelintPluginError extends Base {
+  constructor(message) {
+    super(message);


TIL that ES2015 will automatically do these kinds of constructors, so this export could just be:

module.exports = class StylelintPluginError extends Base {};

and can become more complicated later on as necessary.

I prefer being verbose here. It's especially relevant when the RFC changes are implemented, as it'll likely need to do some things with the constructor.

JaKXz · 2018-02-21T23:19:55Z

lib/formatter-webpack.js

+module.exports = function webpack (items) {
+  const result = [];
+
+  for (const item of items) {


What is your opinion on doing this with a .reduce and .maps? I understand this imperative version is probably simpler to read, and for now is all we need, but I like the "pure" versions because they are easier to test the individual units should they need to be extracted at some point. I took the liberty of writing this all out, just for my own understanding:

module.exports = function webpackFormatter (items) { return items.reduce(function (result, item) { const file = item.source; const warnings = item.warnings.map(function (warning) { const line = warning.line; const column = warning.column; const text = warning.text; return new StylelintPluginError(file + '\n' + line + ':' + column + ' stylelint: ' + text); }); const invalidOptionWarnings = item.invalidOptionWarnings.map(function (warning) { return new StylelintPluginError('stylelint\n0:0 ' + warning.text); }); return result.concat(warnings, invalidOptionWarnings); }, []); };

When the codebase is eventually upgraded to use things like destructuring and arrow functions this method will become much shorter :)

Hah it's six of one, half dozen of another. There's no performance gain so it's all personal preference. If you'd prefer to see this format instead, please do edit the code in the PR (github will allow you to do that here on the site)

JaKXz · 2018-02-21T23:25:25Z

README.md

+
+When using this plugin with a webpack reporter, such as
+[webpack-stylish](https://github.com/webpack-contrib/webpack-stylish), you must
+set the `formatter` option to `'webpack'` for proper error and warning output. eg:


extreme nit: s/eg/e.g.

(I'm sorry)

Please do edit the PR in the Github UX for this :)

JaKXz · 2018-02-21T23:26:02Z

README.md

+    formatter: 'webpack'
+  })
+]
+


another extreme nit, please remove extra whitespace.

Please do edit the PR in the Github UX for this :)

JaKXz · 2018-02-21T23:26:43Z

package.json

@@ -76,6 +76,9 @@
    "globals": [
      "getPath",
      "expect"
+    ],
+    "ignore": [
+      "/lib/StylelintPluginError.js"


with the automatic constructor, I think this is unnecessary.

semistandard complains about the class not being used, in the same file the class is being declared. unfortunately it's necessary for CI to pass

JaKXz · 2018-02-21T23:28:04Z

test/helpers/base-config.js

-var configFilePath = getPath('./.stylelintrc');
+module.exports = function (options) {
+  var configFilePath = getPath('./.stylelintrc');
+  const pluginOptions = Object.assign({ configFile: configFilePath }, options || {});


This is a great change! Thank you!

However, could you:

use the object-assign polyfill, to remain consistent with the rest of the codebase

update baseConfig()s usages in the tests to remove the unnecessary config objects everywhere.

This one doesn't ring as necessary. You're only testing your code on Node 4+, which don't require a polyfill.

JaKXz · 2018-02-21T23:28:26Z

lib/run-compilation.js

    }

+    format('warnings');
+    format('errors');


This is awesome, thank you. 💯 👍

shellscape · 2018-02-22T00:34:44Z

Thanks for reviewing. I honestly appreciate the thorough look. I say this with all positive intent: the codebase for the plugin could definitely use that level of scrutiny as well. I think we disagree some of the responses for the PR, but I would be completely cool if you edited the PR with those changes. This PR will set the plugin up for success and a minimal transition when the Error Alignment RFC (which I'm starting implementing of tonight) lands in webpack@3 and 4. 🍺

Transfering ownership to webpack-contrib

alexander-akait

@shellscape What about sing snapshot, it will be better and easy to reading 👍

shellscape · 2018-03-27T20:17:05Z

@evilebottnawi

What about sing snapshot

I think there's a translation problem there my friend 😆

alexander-akait

Let's tests better later. 👍

joshwiens · 2018-03-31T16:16:08Z

@shellscape - This will require a rebase against current master & can / should be reopened.

vieron and others added 30 commits March 17, 2016 14:50

initial commit

8456852

gitignore: add npm-debug.log

7bc022d

npm: add preversion hook

aa88a5f

0.0.2

e5b26fd

update readme

039a93f

fix: don’t break webpack compilation on syntax errors

bd522f4

0.1.0

126fa8a

npm: update libs

52b6d0f

add travis

59b0b98

Merge pull request #4 from vieron/travis

ba0e26f

add travis

Merge pull request #3 from AndreyUtka/update_libs

6d959d9

npm: update dependencies

npm: clean not used dependencies

eacfb12

inherited from sass-webpack-plugin

Merge pull request #5 from vieron/clean-deps

e2b7bc0

npm: clean not used dependencies

npm: add version script

6aa9d37

Merge pull request #6 from vieron/version-script

52ab42f

npm: add version script

0.2.0

a47b9cb

Badges (#19)

40be41f

* Update README.md

0.3.0

af4b3ae

Remove duplication in updating options & binding the runner (#21)

c1f6dda

Add some jsdoc comments to the "public API"

Remove unused dependencies and update dev deps (#22)

aa37e48

More minor code tweaks (#25)

9328ddc

- consistent tense in test suite - corrected editorconfig for markdown files - use `.have.length` assertion - add gitter badge

tests: promisify the in-memory compiler and remove redundant tests (#27)

5ea295d

fix: warnings not being logged to console (#26)

759e6c1

* Fixed warnings not being logged to console Warnings were not being logged unless there was an error in the css linting. * Corrected number of warnings returned

0.3.1

bb02a54

chore: ignore .env file

53a19b5

refactor(test:helpers): clean up pack compiler (#34)

edf880b

feat: use the default configuration if none is specified (#33)

3d53b98

* It should work without configuration, just use default one * It should warn user if .stylelintrc file does not exit * Add warnings check

chore(release): 0.4.0

9e561bb

build: remove prepublish script since it breaks travis

a44f4fc

JaKXz and others added 13 commits January 14, 2018 16:12

0.10.0

2096f35

fix: lintDirtyModulesOnly matching path on Windows (#124)

3688d08

* Add micromatch * Use micromatch to check changes * Remove matchbase in micromatch

0.10.1

b1e27bb

chore(package): update mocha to version 5.0.0 (#126)

82e440e

chore(package): update dotenv to version 5.0.0 (#129)

ad3158d

fix: remove duplicate error/warning logging (#130)

a15e54c

* style: correct td.verify calls to match documentation * style: use const/lets in lint-dirty-modules.test.js

0.10.2

6e9a0be

fix(package): update micromatch to version 3.1.8 (#134)

5251c9c

Fixes downstream vulnerability: https://snyk.io/vuln/npm:braces:20180219

chore: update yarn.lock

4e9fb20

0.10.3

04e0f69

add support for webpack standard errors

09fa7cb

update README with 'webpack' formatter info

3589754

refactor to avoid empty block error in CI

6b3c9f7

shellscape added 3 commits February 21, 2018 12:41

push invalidOptionWarnings when they exist

da2d21a

add stylelint prefix to webpack message

8d6cd5f

add prefix to tests

2f47abe

JaKXz reviewed Feb 21, 2018

View reviewed changes

vieron and others added 2 commits March 27, 2018 18:47

Transfering ownership to webpack-contrib

604df62

Merge pull request #143 from JaKXz/transfer-license-ownership

81a9f3b

Transfering ownership to webpack-contrib

shellscape requested review from alexander-akait and joshwiens March 27, 2018 19:43

alexander-akait requested changes Mar 27, 2018

View reviewed changes

alexander-akait approved these changes Mar 27, 2018

View reviewed changes

Merge branch 'master' into master

6550375

joshwiens closed this Mar 31, 2018

joshwiens force-pushed the master branch from c78a8e9 to 5814ec1 Compare March 31, 2018 16:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixes #133: Add Support for Webpack-Standard Errors #136

fixes #133: Add Support for Webpack-Standard Errors #136

shellscape commented Feb 21, 2018

coveralls commented Feb 21, 2018 •

edited

JaKXz left a comment

JaKXz Feb 21, 2018

shellscape Feb 22, 2018

JaKXz Feb 21, 2018

shellscape Feb 22, 2018

JaKXz Feb 21, 2018

shellscape Feb 22, 2018 •

edited

JaKXz Feb 21, 2018

shellscape Feb 22, 2018

JaKXz Feb 21, 2018

shellscape Feb 22, 2018

JaKXz Feb 21, 2018

shellscape Feb 22, 2018

JaKXz Feb 21, 2018

shellscape Feb 22, 2018

JaKXz Feb 21, 2018

shellscape Feb 22, 2018

JaKXz Feb 21, 2018 •

edited

shellscape Feb 22, 2018

JaKXz Feb 21, 2018

shellscape commented Feb 22, 2018

alexander-akait left a comment

shellscape commented Mar 27, 2018

alexander-akait left a comment

joshwiens commented Mar 31, 2018

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

		const StylelintPluginError = require('./StylelintPluginError');

fixes #133: Add Support for Webpack-Standard Errors #136

fixes #133: Add Support for Webpack-Standard Errors #136

Conversation

shellscape commented Feb 21, 2018

coveralls commented Feb 21, 2018 • edited

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

Choose a reason for hiding this comment

shellscape Feb 22, 2018 • edited

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

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 Feb 21, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shellscape commented Feb 22, 2018

alexander-akait left a comment

Choose a reason for hiding this comment

shellscape commented Mar 27, 2018

alexander-akait left a comment

Choose a reason for hiding this comment

joshwiens commented Mar 31, 2018

coveralls commented Feb 21, 2018 •

edited

shellscape Feb 22, 2018 •

edited

JaKXz Feb 21, 2018 •

edited