Crash the build during CI whenever linter warnings are encountered #944

excitement-engineer · 2016-10-22T14:26:10Z

Fixes issue #906.

Whenever someone runs the build script with the environmental variable CI set to true then it will crash when any linter warnings are encountered.

CI=true npm run build

I have tested this locally by generating an app using create-react-app and the updated react-scripts. Is this enough or should there be an automated way of testing this functionality? If so, can anyone give some advice on how to go about testing this?

I am also struggling to find the best location to document this feature. I considered updating the npm run build section of the README with this information however, I am afraid that explaining this feature in that location will only detract from the clarity to the reader. Perhaps I should create a new section in the user guide where this is explained, any thoughts?

…ings are encountered.

gaearon · 2016-10-28T12:53:54Z

Thanks! Maybe add a paragraph to https://github.com/facebookincubator/create-react-app/blob/master/packages/react-scripts/template/README.md#continuous-integration?

Can you post a screenshot of what the log looks like with warnings?

excitement-engineer · 2016-10-28T18:06:02Z

Here is a screenshot of the output:)

excitement-engineer · 2016-10-28T20:12:27Z

I have updated the docs, let me know what you think!

fson · 2016-11-22T10:07:15Z

packages/react-scripts/template/README.md


-If you find yourself doing this often in development, please [file an issue](https://github.com/facebookincubator/create-react-app/issues/new) to tell us about your use case because we want to make watcher the best experience and are open to changing how it works to accommodate more workflows.
+The build command will check for linter warning and fail if any are found.


This should be "linter warnings".

fson · 2016-11-22T10:10:03Z

packages/react-scripts/scripts/build.js

@@ -142,6 +142,11 @@ function build(previousSizeMap) {
      process.exit(1);
    }

+    if (process.env.CI && stats.compilation.warnings.length) {
+     printErrors('Failed to compile. Note, the build has crashed because it is being run with the environment variable CI set to true. In this mode the build crashes when any warnings are encountered.', stats.compilation.warnings);


Note, the build has crashed because it is being run with the environment variable CI set to true.

I think this wording makes it sound like having the CI env variable set to true is the problem, not the warnings themself. I hope people won't "fix" this by trying to set the env variable to false...

Good comment! We wouldn't want this message to cause any confusion. I will rephrase it:)

I decided to remove the description! People can refer to the updated README for more information about running builds in CI:) Do you agree?

fson · 2016-12-03T12:39:25Z

👍

scholtzm · 2016-12-04T09:48:47Z

This patch triggers build errors even when webpack emits a warning, e.g.:

This seems to be a pre-built javascript file. Though this is possible, it's not recommended. Try to require the original source to get better results.

Since I have no access to webpack.config.js, I can't use module.noParse.

I also tried to import the main js file from the src directory, but my bundle now includes import statements - it's not processed by webpack at all.

Is there a proper way to fix this besides ejecting?

fson · 2016-12-04T10:05:50Z

I think we might have to change the way we catch the ESLint errors here – failing for any Webpack warning is not good, because mostly harmless warnings are sometimes also triggered by UglifyJS etc. Is there a way we can only catch ESLint errors?

fson · 2016-12-04T10:23:45Z

@scholtzm Can you open a new issue about this problem so we can keep track of it? A merged pull request can't be reopened.

scholtzm · 2016-12-04T10:24:45Z

Yep, was going to suggest it as well.

excitement-engineer · 2016-12-04T17:48:18Z

That is a side affect I did not take into account:( I hope this gets fixed soon! Thanks a lot for finding this bug @scholtzm!

jayphelps · 2016-12-14T02:44:50Z

FWIW I didn't see this addition to the docs (totally my fault), and the error was not clear that this was because of process.env.CI was set (we didn't know it was). Consider logging an explicit error message describing something like When process.env.CI = true, warnings are treated as failures. Most CI servers set this automatically. I can totally see in hindsight why this was done, but it was pretty frustrating trying to figure out what was different about my local machine. Just one user's experience 💃

gaearon · 2016-12-14T11:03:28Z

@jayphelps 👍 Wanna send a PR?

excitement-engineer · 2016-12-14T11:10:56Z

Great feedback! This error message was included in the original PR but I removed it because I thought that just having it in the docs was enough, but I guess I was wrong ;)

gaearon · 2016-12-14T11:20:43Z

Yeah. I think @jayphelps’s wording is more clear than originally was in the PR so I’d be glad to get that in.

excitement-engineer · 2016-12-14T12:06:44Z

Yep, I agree:)

…acebook#944) * Added functionality to crash the build during CI whenever linter warnings are encountered. * Updated the docs with a description on how to use the build in CI * Fixed small typo * Fixed description of build error.

…lures Related: facebook#944

mikew · 2017-03-26T22:57:07Z

Any thought on reconsidering this? I've received flak when trying to add similar functionality to linters, with their reasoning being the rules can be configured to be an error or a warning. Turning a warning into an error doesn't make a whole lot of sense.

And that logic still applies here: eslint lets you configure things to be errors or warnings. It's great that you consider unused vars to be an error. Then make it an error in the first place.

gaearon · 2017-03-26T23:58:37Z

We intentionally show lint errors very obtrusively (e.g. undefined variable doesn't let you see the app at all—it's treated as a built crash). So making making more minor issues errors would be too disruptive.

…acebook#944) * Added functionality to crash the build during CI whenever linter warnings are encountered. * Updated the docs with a description on how to use the build in CI * Fixed small typo * Fixed description of build error.

varnav · 2017-06-01T08:53:07Z

Okay, how to make the build NOT to crash during build with CI set to true?

Added functionality to crash the build during CI whenever linter warn…

bfc11ef

…ings are encountered.

facebook-github-bot added the CLA Signed label Oct 22, 2016

gaearon added this to the 0.8.0 milestone Oct 28, 2016

Updated the docs with a description on how to use the build in CI

90d1c39

Fixed small typo

2b57b8e

fson reviewed Nov 22, 2016

View reviewed changes

Fixed description of build error.

93f687a

fson added the tag: new feature label Dec 1, 2016

fson merged commit cdd17a6 into facebook:master Dec 3, 2016

scholtzm mentioned this pull request Dec 4, 2016

Webpack warnings trigger build errors when running on CI #1150

Closed

karlhorky mentioned this pull request Dec 4, 2016

Run linting in continuous integration #906

Closed

scholtzm mentioned this pull request Dec 5, 2016

Make build fail on ESLint warnings if running CI #1162

Closed

jayphelps mentioned this pull request Jan 5, 2017

More descriptive error message for env.CI = true warnings causing failures #1352

Closed

jayphelps added a commit to jayphelps/create-react-app that referenced this pull request Feb 8, 2017

More descriptive error message for env.CI = true warnings causing fai…

837fbdb

…lures Related: facebook#944

varnav mentioned this pull request Jun 2, 2017

Option not to fail build on warnings in CI environment #2453

Closed

lock bot locked and limited conversation to collaborators Jan 21, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Crash the build during CI whenever linter warnings are encountered #944

Crash the build during CI whenever linter warnings are encountered #944

excitement-engineer commented Oct 22, 2016

gaearon commented Oct 28, 2016

excitement-engineer commented Oct 28, 2016

excitement-engineer commented Oct 28, 2016

fson Nov 22, 2016

fson Nov 22, 2016

excitement-engineer Nov 24, 2016

excitement-engineer Nov 27, 2016

fson commented Dec 3, 2016

scholtzm commented Dec 4, 2016 •

edited

Loading

fson commented Dec 4, 2016

fson commented Dec 4, 2016

scholtzm commented Dec 4, 2016

excitement-engineer commented Dec 4, 2016

jayphelps commented Dec 14, 2016 •

edited

Loading

gaearon commented Dec 14, 2016

excitement-engineer commented Dec 14, 2016

gaearon commented Dec 14, 2016

excitement-engineer commented Dec 14, 2016

mikew commented Mar 26, 2017

gaearon commented Mar 26, 2017

varnav commented Jun 1, 2017


		If you find yourself doing this often in development, please [file an issue](https://github.com/facebookincubator/create-react-app/issues/new) to tell us about your use case because we want to make watcher the best experience and are open to changing how it works to accommodate more workflows.
		The build command will check for linter warning and fail if any are found.

Crash the build during CI whenever linter warnings are encountered #944

Crash the build during CI whenever linter warnings are encountered #944

Conversation

excitement-engineer commented Oct 22, 2016

gaearon commented Oct 28, 2016

excitement-engineer commented Oct 28, 2016

excitement-engineer commented Oct 28, 2016

fson Nov 22, 2016

Choose a reason for hiding this comment

fson Nov 22, 2016

Choose a reason for hiding this comment

excitement-engineer Nov 24, 2016

Choose a reason for hiding this comment

excitement-engineer Nov 27, 2016

Choose a reason for hiding this comment

fson commented Dec 3, 2016

scholtzm commented Dec 4, 2016 • edited Loading

fson commented Dec 4, 2016

fson commented Dec 4, 2016

scholtzm commented Dec 4, 2016

excitement-engineer commented Dec 4, 2016

jayphelps commented Dec 14, 2016 • edited Loading

gaearon commented Dec 14, 2016

excitement-engineer commented Dec 14, 2016

gaearon commented Dec 14, 2016

excitement-engineer commented Dec 14, 2016

mikew commented Mar 26, 2017

gaearon commented Mar 26, 2017

varnav commented Jun 1, 2017

scholtzm commented Dec 4, 2016 •

edited

Loading

jayphelps commented Dec 14, 2016 •

edited

Loading