Fixes for watch mode of create-react-app #43

karlhorky · 2016-12-09T17:12:54Z

I've added this plugin to create-react-app in facebook/create-react-app#1216. This pull request improves the display of the warnings / errors and makes them appear in watch mode too.

JaKXz · 2016-12-09T17:16:11Z

@karlhorky thanks for the PR! on first glance it looks good, once CI passes I'll take a closer look :)

karlhorky · 2016-12-09T17:26:59Z

Hm, ok I'm getting this now:

failing
1) stylelint-webpack-plugin works with multiple source files:
  AssertionError: expected [ Array(1) ] to have a length of 2 but got 1
    at /home/travis/build/vieron/stylelint-webpack-plugin/test/index.js:73:50

I think this is because I'm adding only one array item to warnings with the formatted error. Should I just update the test to check for one? And maybe parse the output to check if there are really two errors reported?

karlhorky · 2016-12-09T17:31:23Z

I've done it the way I described. Let me know if you'd like this improved.

coveralls · 2016-12-09T17:33:02Z

Coverage increased (+0.2%) to 95.238% when pulling 69fc5c7 on karlhorky:create-react-app-fixes into dc97d01 on vieron:master.

coveralls · 2016-12-09T17:33:02Z

Coverage increased (+0.2%) to 95.238% when pulling 69fc5c7 on karlhorky:create-react-app-fixes into dc97d01 on vieron:master.

JaKXz

Thanks again, I think this looks like a great improvement. I'd just like to test it more throughly so we can release it as a patch rather than a new minor version since it brings lots of improvements for everyone without breaking changes (afaict)

JaKXz · 2016-12-10T17:30:26Z

lib/run-compilation.js

-  compilation.plugin && compilation.plugin('compilation', function (compilation) {
-    compilation.errors = compilation.errors.concat(errors);
-    compilation.warnings = compilation.warnings.concat(warnings);
+  compiler.plugin('compilation', function (compilation) {


If removing the guard for compilation.plugin you can remove the eslint comment above.

I guess in more recent versions of webpack the race? condition for plugin being defined have been fixed? ie. Has this been verified to work consistently?

This is done.

I wasn't sure why this guard was there - what race condition do you mean? In my tests in watch mode, it was working correctly.

I thought it was mainly there because the parameter being passed to the watch function was actually a watcher and not a compiler.

JaKXz · 2016-12-10T17:32:01Z

test/index.js

-        expect(stats.compilation.errors).to.have.length(2);
+        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');


Nit: chai's .to.contain matchers work on arrays so since you're asserting on the length above I think you can remove the index

Hm, that doesn't work, since that's not the full content of the string:

1) stylelint-webpack-plugin works with multiple source files: AssertionError: expected [ Array(1) ] to include 'test/fixtures/test7/_second.scss' at ~/projects/stylelint-webpack-plugin/test/index.js:74:45

coveralls · 2016-12-12T12:28:01Z

Coverage increased (+0.2%) to 95.238% when pulling 0eaa431 on karlhorky:create-react-app-fixes into dc97d01 on vieron:master.

karlhorky · 2016-12-14T13:40:51Z

@JaKXz any further feedback? Or can this be merged?

JaKXz · 2016-12-15T05:10:36Z

lib/run-compilation.js

-  compilation.plugin && compilation.plugin('compilation', function (compilation) {
-    compilation.errors = compilation.errors.concat(errors);
-    compilation.warnings = compilation.warnings.concat(warnings);
+  compiler.plugin('compilation', function (compilation) {


Sorry for the delayed response @karlhorky -

Please make this (and other anonymous fns) a named function [because if somehow there's some error thrown it will provide better feedback in the stack trace about where it's coming from]. I should really have a lint rule for this.

JaKXz · 2016-12-15T05:12:38Z

@karlhorky other than one minor comment in those two places, I think this is good to go! Just curious if you've tested this outside create-react-app in a trivial and / or non-trivial codebase. Thanks!

karlhorky · 2016-12-15T13:52:07Z

@JaKXz fixed.

As for your question regarding where this is being used, I have my own fork of create-react-app, which I am currently in the process of switching over to with my portfolio website. It is also the basis for new projects that we're doing at @kununu (including the kununu.com website).

karlhorky · 2016-12-16T09:09:28Z

Ah sorry, missed the eslint warnings. I'll fix the last few.

coveralls · 2016-12-16T09:15:35Z

Coverage increased (+0.2%) to 95.238% when pulling b4e6f2b on karlhorky:create-react-app-fixes into dc97d01 on vieron:master.

karlhorky · 2016-12-16T22:24:59Z

Awesome, thanks @JaKXz!

JaKXz · 2016-12-16T23:10:19Z

No worries! will get this released soon (before the end of Sunday)! Thanks for your patience @karlhorky

JaKXz · 2016-12-18T08:58:43Z

+ stylelint-webpack-plugin@0.4.1 published!

karlhorky · 2016-12-18T15:18:14Z

Thanks! Upgraded in my create-react-app PR.

I noticed that the tests were breaking because stylelint released version 7.7.0 (breaking paths with stylelint/stylelint#2134). I've opened #49 for this.

* Pass in compiler to 'watch-run' plugin * Fix incorrect variable name * Display formatted warnings and errors * Fix linting error * Fix test * Remove unnecessary eslint configuration * Name anonymous functions * correct style nits * fixes eslint nit * Fix linting errors

karlhorky added 3 commits December 9, 2016 16:24

Pass in compiler to 'watch-run' plugin

8be84f3

Fix incorrect variable name

4ce63b6

Display formatted warnings and errors

087f0c6

karlhorky mentioned this pull request Dec 9, 2016

Add stylelint facebook/create-react-app#1216

Closed

Fix linting error

18a6348

Fix test

69fc5c7

JaKXz reviewed Dec 10, 2016

View reviewed changes

Remove unnecessary eslint configuration

0eaa431

JaKXz reviewed Dec 15, 2016

View reviewed changes

karlhorky and others added 3 commits December 15, 2016 14:53

Name anonymous functions

0980ebc

correct style nits

1e70f58

fixes eslint nit

359a651

Fix linting errors

b4e6f2b

JaKXz merged commit 996eb21 into webpack-contrib:master Dec 16, 2016

karlhorky deleted the create-react-app-fixes branch December 18, 2016 15:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes for watch mode of create-react-app #43

Fixes for watch mode of create-react-app #43

karlhorky commented Dec 9, 2016

JaKXz commented Dec 9, 2016

karlhorky commented Dec 9, 2016

karlhorky commented Dec 9, 2016

coveralls commented Dec 9, 2016

coveralls commented Dec 9, 2016

JaKXz left a comment

JaKXz Dec 10, 2016

karlhorky Dec 12, 2016 •

edited

Loading

JaKXz Dec 10, 2016

karlhorky Dec 12, 2016

coveralls commented Dec 12, 2016 •

edited

Loading

karlhorky commented Dec 14, 2016

JaKXz Dec 15, 2016 •

edited

Loading

JaKXz commented Dec 15, 2016

karlhorky commented Dec 15, 2016 •

edited

Loading

karlhorky commented Dec 16, 2016

coveralls commented Dec 16, 2016 •

edited

Loading

karlhorky commented Dec 16, 2016

JaKXz commented Dec 16, 2016

JaKXz commented Dec 18, 2016

karlhorky commented Dec 18, 2016

Fixes for watch mode of create-react-app #43

Fixes for watch mode of create-react-app #43

Conversation

karlhorky commented Dec 9, 2016

JaKXz commented Dec 9, 2016

karlhorky commented Dec 9, 2016

karlhorky commented Dec 9, 2016

coveralls commented Dec 9, 2016

coveralls commented Dec 9, 2016

JaKXz left a comment

Choose a reason for hiding this comment

JaKXz Dec 10, 2016

Choose a reason for hiding this comment

karlhorky Dec 12, 2016 • edited Loading

Choose a reason for hiding this comment

JaKXz Dec 10, 2016

Choose a reason for hiding this comment

karlhorky Dec 12, 2016

Choose a reason for hiding this comment

coveralls commented Dec 12, 2016 • edited Loading

karlhorky commented Dec 14, 2016

JaKXz Dec 15, 2016 • edited Loading

Choose a reason for hiding this comment

JaKXz commented Dec 15, 2016

karlhorky commented Dec 15, 2016 • edited Loading

karlhorky commented Dec 16, 2016

coveralls commented Dec 16, 2016 • edited Loading

karlhorky commented Dec 16, 2016

JaKXz commented Dec 16, 2016

JaKXz commented Dec 18, 2016

karlhorky commented Dec 18, 2016

karlhorky Dec 12, 2016 •

edited

Loading

coveralls commented Dec 12, 2016 •

edited

Loading

JaKXz Dec 15, 2016 •

edited

Loading

karlhorky commented Dec 15, 2016 •

edited

Loading

coveralls commented Dec 16, 2016 •

edited

Loading