Fixes for watch mode of create-react-app #43
Conversation
|
@karlhorky thanks for the PR! on first glance it looks good, once CI passes I'll take a closer look :) |
|
Hm, ok I'm getting this now: 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? |
|
I've done it the way I described. Let me know if you'd like this improved. |
1 similar comment
| compilation.plugin && compilation.plugin('compilation', function (compilation) { | ||
| compilation.errors = compilation.errors.concat(errors); | ||
| compilation.warnings = compilation.warnings.concat(warnings); | ||
| compiler.plugin('compilation', function (compilation) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| 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'); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
|
@JaKXz any further feedback? Or can this be merged? |
| compilation.plugin && compilation.plugin('compilation', function (compilation) { | ||
| compilation.errors = compilation.errors.concat(errors); | ||
| compilation.warnings = compilation.warnings.concat(warnings); | ||
| compiler.plugin('compilation', function (compilation) { |
There was a problem hiding this comment.
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.
|
@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 |
|
@JaKXz fixed. As for your question regarding where this is being used, I have my own fork of |
|
Ah sorry, missed the eslint warnings. I'll fix the last few. |
|
Awesome, thanks @JaKXz! |
|
No worries! will get this released soon (before the end of Sunday)! Thanks for your patience @karlhorky |
|
|
|
Thanks! Upgraded in my I noticed that the tests were breaking because |
* 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
I've added this plugin to
create-react-appin facebook/create-react-app#1216. This pull request improves the display of the warnings / errors and makes them appear in watch mode too.