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

Make build fail on ESLint warnings if running CI #1162

Closed
wants to merge 2 commits into from
Closed

Make build fail on ESLint warnings if running CI #1162

wants to merge 2 commits into from

Conversation

scholtzm
Copy link
Contributor

@scholtzm scholtzm commented Dec 5, 2016

( Related to previous pull request #944 and issue #1150 that followed. )

This pull request reverts behaviour in build script and moves the responsibility to eslint-loader.

As a bonus, this change makes the build fail almost immediately compared to previous solution.

cc @excitement-engineer

@@ -184,7 +184,9 @@ module.exports = {
// TODO: consider separate config for production,
// e.g. to enable no-console and no-debugger only in production.
configFile: path.join(__dirname, '../.eslintrc'),
useEslintrc: false
useEslintrc: false,
failOnWarning: !!process.env.CI,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to make sure this works after ejecting too.
So // @remove-on-eject-begin needs to move inside eslint config object.

@gaearon
Copy link
Contributor

gaearon commented Dec 5, 2016

The stack trace is useless here and the error message is not very good.

screen shot 2016-12-05 at 20 18 30

Is there anything we can do to improve them? Ideally I'd like to remove Module build failed: Error: from the output completely (a hacky way is fine), and remove the ESLint loader stack as well.

@scholtzm
Copy link
Contributor Author

scholtzm commented Dec 5, 2016

Seems like all of this formatting is directly baked into webpack.

The short message after Error: comes from here.

I agree though, the stacktrace is pretty unfortunate in this case and can be confusing.

@gaearon
Copy link
Contributor

gaearon commented Dec 5, 2016

Seems like all of this formatting is directly baked into webpack.

It is, but we filter that in start script. Can we do the same here?

I don't mind dirty hacks as long as they're inside react-dev-utils and the worst case is they stop working (but don't crash anything).

@scholtzm
Copy link
Contributor Author

scholtzm commented Dec 5, 2016

Well, we could dissect the error here and just pretty-print it. I'll have a better look at this later.

@gaearon
Copy link
Contributor

gaearon commented Dec 6, 2016

Another thing I'm not very happy with is that now Babel build errors don't show the source:

screen shot 2016-12-06 at 17 36 28

They used to have the code frame:

screen shot 2016-12-06 at 17 36 12

I'm not sure if this is the right direction. Maybe we should just manually call ESLint itself and remove eslint-loader from production webpack?

@gaearon gaearon added this to the 0.8.3 milestone Dec 6, 2016
@scholtzm
Copy link
Contributor Author

scholtzm commented Dec 6, 2016

I played around with it and indeed eslint-loader will also swallow any parsing errors.

Closing this PR in favour of a better solution.

@scholtzm scholtzm closed this Dec 6, 2016
@gaearon gaearon removed this from the 0.8.3 milestone Dec 8, 2016
@lock lock bot locked and limited conversation to collaborators Jan 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants