Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ESLint warnings and errors ignored in npm run build #440

Closed
mjomble opened this issue Aug 14, 2016 · 7 comments
Closed

ESLint warnings and errors ignored in npm run build #440

mjomble opened this issue Aug 14, 2016 · 7 comments

Comments

@mjomble
Copy link

mjomble commented Aug 14, 2016

To reproduce:

create-react-app cra-test
cd cra-test
echo foo() > src/index.js
npm start

Output:

Warning in ./src/index.js

E:\dev\cra-test\src\index.js
  1:1  warning  'foo' is not defined  no-undef

✖ 1 problem (0 errors, 1 warning)

But if you run npm run build:

Creating an optimized production build...
Compiled successfully.

File sizes after gzip:
...

Tested with 0.2.1

The reason is explained here:

to handle all errors and warnings with the node.js API you need to test err, stats.errors and stats.warnings

stats.errors and stats.warnings are checked in start.js but build.js only checks for err.

I may be able to submit a PR for this if needed.

@gaearon
Copy link
Contributor

gaearon commented Aug 14, 2016

PR is welcome, I didn't really test this well.

@gaearon
Copy link
Contributor

gaearon commented Aug 14, 2016

Do you think the build should fail?

@mjomble
Copy link
Author

mjomble commented Aug 14, 2016

Maybe fail when there are errors and pass if there are only warnings?

@gaearon
Copy link
Contributor

gaearon commented Aug 14, 2016

We have no errors in the lint config (on purpose). And we only use warnings for stuff that really hints at possible errors (i.e. no style rules). So I think it might be better to fail.

@mjomble
Copy link
Author

mjomble commented Aug 14, 2016

Ah, that makes sense.

I also noticed that in start.js, if there are linting errors, the dev server starts up, but the "The app is running at..." message does not appear.
I might also change that in the PR, so that this message is always shown, whether there are linter warnings or not.

@gaearon
Copy link
Contributor

gaearon commented Aug 15, 2016

Sounds good.

@gaearon
Copy link
Contributor

gaearon commented Feb 11, 2017

I think this was fixed since.

@gaearon gaearon closed this as completed Feb 11, 2017
@lock lock bot locked and limited conversation to collaborators Jan 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants