Lint and build tasks display lint warnings differently

[constgen]

Test suite: https://mega.nz/#!mRlWkToS!7o74K5v1RKOAJ64JxQ8bAG77Ch6YUgP4ugmQ9Fj0a6o

That is a minimal config to reproduce the issue.

Run npm run lint. It will find a single ESLint rules violation. This is expected:

> neutrino lint

✖ Running lint failed

/Users/owner/Desktop/preset-eslint-issue/src/index.js
  5:1  error  Trailing spaces not allowed  no-trailing-spaces

✖ 1 problem (1 error, 0 warnings)
  1 error, 0 warnings potentially fixable with the `--fix` option.

Now run npm run build. Besides the previous error there is one additional warning.

> neutrino build

✖ Building project failed
./src/index.js
Module build failed: Module failed because of a eslint error.

/Users/owner/Desktop/preset-eslint-issue/src/index.js
  1:1  warning  'use strict' is unnecessary inside of modules  strict
  5:1  error    Trailing spaces not allowed                    no-trailing-spaces

✖ 2 problems (1 error, 1 warning)
  1 error, 1 warning potentially fixable with the `--fix` option.

This is not expected as 'use strict' is not a violation by rules.

'use strict' violations has "warning" severity, by the way, in .eslintrc file. That's why it is not error but warning.

"sourceType": "script" is also declared to allow strict directive, but build task thinks that it's "sourceType": "module" for some reason.

[eliperelman]

So, the point of the .neutrinorc.js is to try and unify the configuration for many tools. My first proposition would be to move the configuration for the linting into the .neutrinorc.js.

Second, webpack is a module bundler, and the code it generates are technically modules, not scripts. the eslint middleware already sets the sourceType to module, since all the code is written in ES modules or CJS modules. Hence, a strict directive is unnecessary, and the linting should probably stay as module.

[edmorley]

So, the point of the .neutrinorc.js is to try and unify the configuration for many tools. My first proposition would be to move the configuration for the linting into the .neutrinorc.js.

If it helps, this is documented here:
https://neutrino.js.org/middleware/neutrino-middleware-eslint/#eslintrc-config

[constgen]

But neutrino lint works correctly, you see? It uses the same Webpack config.

Also if you try to override this option explicitly it will have no effect:

['neutrino-middleware-eslint', { 
  eslint: { 
    useEslintrc: false,
    parserOptions: {
      sourceType: 'script'
    }
  } 
}],

[edmorley]

Also if you try to override this option explicitly it will have no effect:

See the impliedStrict mention on:
https://eslint.org/docs/rules/strict#rule-details

...since impliedStrict is set here:
https://github.com/mozilla-neutrino/neutrino-dev/blob/6ea0728c95ac1a7c061428a8dd7a33db7dfd6192/packages/neutrino-middleware-eslint/index.js#L79

[edmorley]

But neutrino lint works correctly, you see? It uses the same Webpack config.

Ah I think I have an idea what's going on:

• Using the testcase zip, I get the exact errors/warnings as listed in the OP.
• If I edit .eslintrc to change the strict rule from warn to error, then both neutrino lint and neutrino build report exactly two errors (one for strict, one for no-trailing-spaces).
• If I revert those .eslintrc changes, and instead remove every other rule apart from strict, then both neutrino lint and neutrino build report exactly one warning (from the strict rule).

ie: The current behaviour of the neutrino lint command seems to be "only show the warnings if there were no errors, otherwise just show the errors and hide the warnings", whereas neutrino build instead shows all warnings+errors all the time.

That seems confusing and perhaps not what is intended.

[eliperelman]

Yeah, that's not the intention.

[constgen]

Ok. Overriding impliedStrict worked and it helped to resolve my task. So in summary it looks like disabling some preconfigured values. I leave this here for those who faced the same problem with .eslintrc:

neutrino.config
   .module.rule('lint')
      .use('eslint')
         .tap(function(options){
             options.useEslintrc = true;
             options.parserOptions = {};
             return options;
         });

@edmorley, I confirm the wrong behavior you mentioned

The current behaviour of the neutrino lint command seems to be "only show the warnings if there were no errors, otherwise just show the errors and hide the warnings", whereas neutrino build instead shows all warnings+errors all the time.

[edmorley]

That's great :-)

To summarise slightly, I think there were a few factors involved:

1. The ESLint strict rule is unfortunately one of the confusing ones whose behaviour can be affected by multiple settings.
2. The neutrino-middleware-eslint defaults include two of the settings that affect strict, meaning that both have to be overridden. It's possible the middleware doesn't need to be defining both (which would mean less to override) - for which I've filed See if any of the @neutrinojs/eslint defaults are unnecessary #383.
3. Using useEslintrc: true (and a separate JSON .eslintrc) is not something that's recommended / has potential for confusion, since the Neutrino options (eg from neutrino-middleware-eslint) override the options in .eslintrc (unless parserOptions is also overridden). Either we should warn or fail early in this configuration (to prevent user frustration) or try to improve it. For this I've opened Reducing the potential for confusion if configuring ESLint outside of Neutrino #382.
4. There is currently inconsistency in how warnings are displayed for lint vs build (as described in my last comment), which is a Neutrino bug and is what I've adjusted this issue's summary to be about.

[edmorley]

Reduced STR:

1. Create the following:

// .neutrinorc.js
module.exports = {
  use: [
    'neutrino-preset-web',
    ['neutrino-middleware-eslint', {
      eslint: {
        rules: {
          'no-trailing-spaces': 'error',
          'strict': 'warn',
        }
      }
    }]
  ]
};

// ./src/index.js
'use strict';

// There is trailing whitespace at the end of this line -->     

2. yarn add neutrino@7.2.3 neutrino-preset-web@7.2.3 neutrino-middleware-eslint@7.2.3
3. yarn run neutrino lint
4. yarn run neutrino build

Expected:

• The same errors/warnings are shown for both lint and build (1 warning, 1 error).

Actual:

• Build shows 1 warning, 1 error.
• Lint shows just the error (and if that error is fixed by correcting index.js, re-running lint then shows the warning).

[eliperelman]

The current behaviour of the neutrino lint command seems to be "only show the warnings if there were no errors, otherwise just show the errors and hide the warnings", whereas neutrino build instead shows all warnings+errors all the time.

I haven't re-checked the source yet, but my guess is this is a discrepancy between how eslint-loader handles warnings and errors using CLIEngine in the build command, and how we are differing using CLIEngine in the lint command. We can either change these options, or have a look at the eslint-loader code to see if we should be doing something better, maybe by adopting options for failOnWarning and failOnError.

[eliperelman]

I found an issue here trying to debug this. Calling this:

const errors = CLIEngine.getErrorResults(report.results);
const formatted = formatter(report.results);

The purpose of this was just to get the count of errors to determine whether to reject. Turns out it actually does an in-place mutation of report.results to only have the errors. If you move the formatter call to before the getErrorResults call, you get the warnings!

I'll push some fixes shortly.

[edmorley]

Turns out it actually does an in-place mutation of report.results to only have the errors.

Oh good spot!