-
-
Notifications
You must be signed in to change notification settings - Fork 6.3k
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 process returning incorrect exit/error codes #3552
Comments
@sodatea The behavior with
This is not handled properly. Warnings are just that: warnings. According to the eslint docs (emphasis mine):
|
Umm… There must have been a design tradeoff or implementation mistake here but I'm too sleepy to further investigate into this. Will have a look at this later. |
Related code: https://github.com/vuejs/vue-cli/blob/dev/packages/%40vue/cli-plugin-eslint/index.js#L49-L50 |
Thanks for taking a look. I'll look at that webpack config. But besides that, I'm speaking of the vue-cli developer experience out of the box. The way eslint behaves is incorrect and shouldn't require any config for it to work the way it should. |
The implementation has a few pounds of historic baggage from the old days of the webpack template, and some issues in making both eslint-loader and friendly-errors-webpack-plugin play nice together. Additionally, the default of not emitting any errors was chosen so the linting errors doesn't halt HMR updates during development (See: https://github.com/webpack-contrib/eslint-loader#emitwarning-default-false) But I think we could try and improve the behaviour for production builds. |
Ok, so right now we either:
A new optionA third options would make sense that does what would be the default:
Do we need a 3rd option or can we change behaviour of one of the existing ones? I say we can't d it in a minor release, as that would be a breaking change. Change in next MajorBut we can improve the API with the next semver major release:
In all cases, behaviour in Qs:
workaroundAll of this can be changed in webpack, today, like this: module.exports = {
lintOnSave: true,
chainWebpack: config => {
config.module.rule('eslint').use('eslint-loader').tap(opts => {
opts.emitWarnings = yourValueHere
opts.emitErrors = yourValueHere
})
}
} |
@LinusBorg Thanks! I think the new default of I tried your suggested workaround and noticed a few things:
It's complaining about this code: <div
one="one"
:two="two"
three="three"
/> There's no rule in <div
prop-one="prop"
:prop-two="prop"
prop-three="prop">
</div> |
Likely because I misspelled the options:
No idea. Please provide code to actually run and inspect.
Sounds like it's likely unrelated to the CLI, and more likely related to eslint-plugin-vue. Maybe you are still using v4? The rule worked differently back then https://github.com/vuejs/eslint-plugin-vue/blob/4.x/docs/rules/attributes-order.md |
@LinusBorg That's my thinking, that Go ahead and (in the same repo) checkout branch
As a side note, I noticed if I run |
To avoid breaking changes I think it's better to adjust it to the following:
|
@ffxsam Hm, looking at the implementation, there'S a reference to an older issue about two versions of eslint, and supposedly, this commit was meant to fix it: I'll check out your repro later tonight, but it seems like a bug unrelated to the improvement discussed in this issue here, so we should maybe open a new one for the bug. @yyx990803 Agreed, I'll submit a PR for this. |
Submitted a PR to add the 3rd option. @ffxsam Also installed your repro and only receive one error: that a bout the console. Can you delete the lockfile and /node_modules/.cache and reinstall? maybe some dependency mixup... If the problem persists for you, we should investigate (in a separate issue), but right now i can't reproduce what you are xperiencing, and the commit I previously linked to should have fixed it ... |
@LinusBorg Thanks! I'm happy to wait till this PR is merged and published, then I can open a new issue if the problem still persists. |
@sodatea @LinusBorg I'd suggest this be the default behavior in vue-cli projects. I noticed it was not, but once I put this in module.exports = {
lintOnSave: 'default'
} Then it behaved as expected. I imagine if someone wants eslint to throw errors, they'd want it to prevent their build from succeeding. |
In case this helps others who happen upon this thread from Googling: Setting So I ended up running the lint command with these flags on the pipeline to get the appropriate error code: Hope that helps :) |
Version
3.4.1
Reproduction link
https://github.com/ffxsam/repro-errors-passing-build
Environment info
Steps to reproduce
What is expected?
I expect an error return code (so the CI build would fail)
What is actually happening?
Return code 0 (success) is returned, so app deploys
The text was updated successfully, but these errors were encountered: