Allow webpack to compile on lint error #1104
Conversation
| @@ -24,7 +24,13 @@ module.exports = { | |||
| exclude: /node_modules/, | |||
| use: [ | |||
| 'babel-loader', | |||
| { loader: 'eslint-loader', options: { fix: true } } | |||
| { | |||
| loader: 'eslint-loader', | |||
There was a problem hiding this comment.
Could we configure this behavior by environment? I'd like to make sure CI breaks when eslint is failing.
There was a problem hiding this comment.
I took a stab at doing this; it required separating out our dev and prod webpack configs, which one we use now depends on NODE_ENV.
There was a problem hiding this comment.
While I support splitting them out, I'm not sure it is required quite yet. Why not just:
options: {
fix: true,
emitWarning: process.env.NODE_ENV !== 'production'
| @@ -24,7 +24,13 @@ module.exports = { | |||
| exclude: /node_modules/, | |||
| use: [ | |||
| 'babel-loader', | |||
| { loader: 'eslint-loader', options: { fix: true } } | |||
| { | |||
| loader: 'eslint-loader', | |||
There was a problem hiding this comment.
While I support splitting them out, I'm not sure it is required quite yet. Why not just:
options: {
fix: true,
emitWarning: process.env.NODE_ENV !== 'production'
web/app/package.json
Outdated
| @@ -32,6 +32,7 @@ | |||
| "sinon-stub-promise": "^4.0.0", | |||
| "webpack": "^3.5.6", | |||
| "webpack-dev-server": "^2.7.1", | |||
| "webpack-merge": "^4.1.2", | |||
|
oh yeah! that's way simpler, I much rather that ! |
rmars
force-pushed
the
rmars/tweak-eslint
branch
from
65278d8
to
4507aca
Compare
web/app/webpack.config.js
Outdated
| loader: 'eslint-loader', | ||
| options: { | ||
| fix: true, | ||
| emitWarning: process.env.NODE_ENV !== 'production' |
There was a problem hiding this comment.
Relating to @grampelberg's comment:
I'd like to make sure CI breaks when eslint is failing.
Our CI configuration runs tests with bin/web test, without setting NODE_ENV. So I think NODE_ENV defaults to 'development' when running tests in CI, in which case branches with lint errors won't actually fail the CI run. Maybe there's a different way to configure this to ensure that CI catches lint errors?
There was a problem hiding this comment.
Looks like the build sets it correctly and testing doesn't modify from the default (development). Could change it to process.env.NODE_ENV === 'development' and then set NODE_ENV=testing in .travis.yml.
There was a problem hiding this comment.
I've added a NODE_ENV to the travis.yml and committed a lint error. If CI fails on this commit we'll know everything works.
There was a problem hiding this comment.
Eugh,CI still passes. Looks like bin/web doesn't exit even if the build function returns a non-zero exit code. Looking into it now.
This reverts commit 9cc2e09.
rmars
force-pushed
the
rmars/tweak-eslint
branch
from
a7f4a85
to
9f6e22c
Compare
…reasons this time
|
Finally! It works! I'm going to fix the lint error now, and this should be ready to go. |
#1035 added a bunch of useful eslint rules to the project. However it converted a bunch of lint warnings into errors, and webpack will fail to complile the js bundle on any eslint error. This means that if you develop locally with
webpack-dev-serverand make any lint errors, you won't get that nice webpack auto reload when you make a javascript change.This change allows webpack to finish compiling the bundle even if there is a lint error.