Vue-cli: eslint process returning incorrect exit/error codes

[ffxsam]

Version

3.4.1

Reproduction link

https://github.com/ffxsam/repro-errors-passing-build

Environment info

  System:
    OS: macOS 10.14.3
    CPU: (12) x64 Intel(R) Core(TM) i7-8700B CPU @ 3.20GHz
  Binaries:
    Node: 10.15.0 - ~/.nvm/versions/node/v10.15.0/bin/node
    Yarn: 1.13.0 - /usr/local/bin/yarn
    npm: 6.8.0 - ~/.nvm/versions/node/v10.15.0/bin/npm
  Browsers:
    Chrome: 72.0.3626.119
    Firefox: 65.0.1
    Safari: 12.0.3
  npmPackages:
    @vue/babel-helper-vue-jsx-merge-props:  1.0.0-beta.2
    @vue/babel-plugin-transform-vue-jsx:  1.0.0-beta.2
    @vue/babel-preset-app:  3.4.1
    @vue/babel-preset-jsx:  1.0.0-beta.2
    @vue/babel-sugar-functional-vue:  1.0.0-beta.2
    @vue/babel-sugar-inject-h:  1.0.0-beta.2
    @vue/babel-sugar-v-model:  1.0.0-beta.2
    @vue/babel-sugar-v-on:  1.0.0-beta.2
    @vue/cli-overlay:  3.4.1
    @vue/cli-plugin-babel: ^3.4.0 => 3.4.1
    @vue/cli-plugin-eslint: ^3.4.0 => 3.4.1
    @vue/cli-service: ^3.4.0 => 3.4.1
    @vue/cli-shared-utils:  3.4.1
    @vue/component-compiler-utils:  2.6.0
    @vue/preload-webpack-plugin:  1.1.0
    @vue/web-component-wrapper:  1.2.0
    eslint-plugin-vue: ^5.2.2 => 5.2.2
    vue: ^2.6.6 => 2.6.8
    vue-eslint-parser:  5.0.0
    vue-hot-reload-api:  2.3.3
    vue-loader:  15.7.0
    vue-style-loader:  4.1.2
    vue-template-compiler: ^2.5.21 => 2.6.8
    vue-template-es2015-compiler:  1.9.1
  npmGlobalPackages:
    @vue/cli: 3.4.1

Steps to reproduce

1. yarn
2. yarn build

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

[sodatea]

https://cli.vuejs.org/config/#lintonsave

[ffxsam]

@sodatea The behavior with lintOnSave: 'error' is still broken.

1. eslint warnings cause the app to fail while running locally with yarn serve
2. eslint warnings cause yarn build to fail

This is not handled properly. Warnings are just that: warnings.

According to the eslint docs (emphasis mine):

"off" or 0 - turn the rule off
"warn" or 1 - turn the rule on as a warning (doesn’t affect exit code)
"error" or 2 - turn the rule on as an error (exit code is 1 when triggered)

[sodatea]

Umm…
Though, you can customize it with eslint-loader config

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.

[sodatea]

Related code: https://github.com/vuejs/vue-cli/blob/dev/packages/%40vue/cli-plugin-eslint/index.js#L49-L50
Maybe we should use failOnWarning/failOnError instead of emitWarning/emitError

[ffxsam]

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.

[LinusBorg]

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.

[LinusBorg]

Ok, so right now we either:

1. emit everything as warnings (lintOnSave: true)
2. emit everything as errors (lintOnSave: 'errors')

A new option

A third options would make sense that does what would be the default:

1. emit warnings as warnings and errors as errors (lintOnSave: 'normal' or what should it be called?)

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 Major

But we can improve the API with the next semver major release:

lintOnSave:

|value|effect|
|-----|------|
|true| emit warnings as warnings, errors as errors|
|'errors'|emit everything as errors|
|'warnings'|emit everything as warnings|

In all cases, behaviour in development would always be like warning

Qs:

1. What about mode === 'test'?

workaround

All 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
    })
  }
}

[ffxsam]

@LinusBorg Thanks! I think the new default of lintOnSave: true which would emit warnings as warnings, and errors as errors makes sense, for both yarn build and yarn serve (if I have a rule to be flagged as an error, I would expect it to stop my app from building during local development).

I tried your suggested workaround and noticed a few things:

1. It doesn't matter what I set emitWarnings and emitErrors to, it has the same behavior either way. Their values appear to have no effect, but the mere presence of the code you suggested does make errors get emitted properly. ❓❓
2. Adding this config.module code for eslint causes strange behavior upon running yarn build, for example:

/Users/samh/granted/src/App.vue
  5:7  error  Attribute ":two" should go before "one"  vue/attributes-order

It's complaining about this code:

<div
  one="one"
  :two="two"
  three="three"
/>

There's no rule in vue/attributes-order about v-bind attributes coming before static attributes. In fact, the example on that page shows this as being good code:

  <div
    prop-one="prop"
    :prop-two="prop"
    prop-three="prop">
  </div>

[LinusBorg]

It doesn't matter what I set emitWarnings and emitErrors to, it has the same behavior either way.

Likely because I misspelled the options: emitWarning / emitError (singular)

the mere presence of the code you suggested does make errors get emitted properly.

No idea. Please provide code to actually run and inspect.

There's no rule in vue/attributes-order about v-bind attributes coming before static attributes. In fact, the example on that page shows this as being good code:

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

[ffxsam]

@LinusBorg That's my thinking, that yarn build is using a different eslint version than the rest of the vue-cli.. somehow.

Go ahead and (in the same repo) checkout branch webpack-manual-config. Steps to reproduce:

1. yarn lint (notice there are no errors or warnings, which is correct)
2. yarn build (console.log error is expected; vue/attributes-order error is _not_ expected - conflicts with the official rule specs)

As a side note, I noticed if I run yarn build a second time, it ignores any warnings and errors and builds with a success result. Is this intended behavior? It's apparently checking if the bundled filename hash has changed, and if it hasn't, it doesn't lint the file.

[yyx990803]

To avoid breaking changes I think it's better to adjust it to the following:

• "default": emit warnings and warnings and errors as errors (what @ffxsam wants)
• "warnings": emit everything as warnings (avoid errors to pop up in overlay during development)
• "errors": emit everything as errors
• true: same as "warnings" (also same as current behavior so it doesn't have to be breaking, can be deprecated in next major)

[LinusBorg]

@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:

https://github.com/vuejs/vue-cli/commit/077343ba6accb3eec9334c0b0f023d89d704b9f9

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.

[LinusBorg]

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 ...

[ffxsam]

@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.

[ffxsam]

@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 vue.config.js:

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.

[zhao-li]

In case this helps others who happen upon this thread from Googling:

Setting lintOnSave to default did not work for me as I still got an exit code of 0 even though the linter found errors that it auto-fixed as part of the pipeline.

So I ended up running the lint command with these flags on the pipeline to get the appropriate error code:
vue-cli-service lint --max-warnings 0 --no-fix

Hope that helps :)