Reducing the potential for confusion if configuring ESLint outside of Neutrino

[edmorley]

Summary

As seen in #380, users sometimes don't follow the docs recommendations, and store eslint configuration outside of Neutrino. As part of this, they may also override neutrino-middleware-eslint's default of useEslintrc: false, which can cause a few problems.

Background

For linting, the Neutrino docs recommended:

• Keeping all linting configuration in .neutrinorc.js (or middleware called from there).
• Favouring neutrino lint over calling eslint manually.
• For cases where manual eslint usage is required (eg editor/IDE support), creating an .eslintrc.js which does nothing more than call the Neutrino API to generate a config that's equivalent to the one used by neutrino lint.

There are then three ways linting is used:

1. Via the neutrino lint command
   • This uses the ESLint API (specifically CLIEngine.executeOnFiles()).
2. Via the neutrino {build,start,...} commands
   • These use eslint-loader to hook up linting to the webpack build, which itself uses the ESLint API (specifically CLIEngine.executeOnText()).
3. Via running eslint from the CLI or via an editor/IDE plugin
   • These load the .eslintrc* files (of which there are several supported formats/filenames: docs).
   • If there is an .eslintrc.js and it's set up as recommended by the Neutrino docs, it exports a mostly equivalent config (with some keys filtered out, since the RC format differs slightly from that accepted by CLIEngine).

Alternative approaches users might take

For users that don't follow the approach recommended by the docs, I can think of a few possible variations they might use:

(A) Using an alternate ESLint RC file and useEslintrc: true:

For example (from #380)...

// .neutrinorc.js
module.exports = {
    options: {
        output: 'dist'
    },
    use: [
        ['neutrino-middleware-eslint', { eslint: { useEslintrc: true } }],
        ...
    ]
}

// .eslintrc (Note: Doesn't have a `.js` extension)
{
    "extends": "eslint:recommended",
    "parserOptions": {
        "sourceType": "script"
    },
    // ...
}

The problem with this approach is that:

• it will cause differences in what's reported between the eslint CLI/editors and the neutrino commands (unless the user manages to get the .eslintrc to match exactly the generated config)
• it's not easy to get the .eslintrc to match the generated config, since (a) there are a lot of options already set in neutrino-middleware-eslint (plus other middleware), and (b) the generated config takes precedence rather than vice versa (I've filed Documenting configuration hierarchy for CLIEngine options and 'useEslintrc: true' eslint/eslint#9526 for clarifying this in the ESLint docs).

ie: One ends up in a confusing situation where neither the .eslintrc nor the Neutrino configuration is the source of truth.

(B) Using an .eslintrc.js that calls the Neutrino API, but defines rules outside of .neutrinorc.js:

For example:

// .eslintrc.js
const { Neutrino } = require('neutrino');
const api = Neutrino();

module.exports = api.call('eslintrc', [
  ['neutrino-middleware-eslint', {
    eslint: {
      rules: { semi: 'off' }
    }
  }],
  'neutrino-preset-react'
]);

This is actually suggested in the docs as an alternative, however this approach will cause differences in what's reported between the eslint CLI/editors and the neutrino commands. Is this really a use-case that should be mentioned in the docs? (Or even supported at all?)

I guess perhaps some users might want to disable or enable certain rules in their editor only? (eg make the editor warn about things that shouldn't yet fail the build, since a project is slowly trying to fix old cases before enabling the rule for real)

Worth noting, is that if the user takes this approach and also sets useEslintrc: true in their .neutrinorc.js, then the Neutrino eslint config will get generated twice. Perhaps Neutrino should fail hard in getEslintRcConfig() if that's the case?

Proposal

IMO we should do one of:

• forbid use of useEslintrc: true entirely (what are the use-cases for using the JSON format .eslintrc instead of Neutrino generated config?)
• allow useEslintrc: true, but neutrino-middleware-eslint should then not set parserOptions itself (ie the middleware does only the bare minimum, leaving the user to manually configure everything else in .eslintrc, so at least the RC file then ends up being much closer to the source of truth)
• allow useEslintrc: true, but output a warning about the problems / emphasise the problems in the docs and source comments

Note: If we don't forbid useEslintrc: true, then we should still make getEslintRcConfig() (used by the .eslintrc.js API call only) fail hard if it's set, since as mentioned in (B) this case results in double configuration and is clearly not what the user would want.

Thoughts?

[barraponto]

It's hard to take sides since I just can't see any reason to write eslintrc files. Even in a migration scenario, it would be just as easy to incorporate the eslintrc as it is to enable eslintrc. So, I'm in favor of failing hard in case useEslintrc is set to true.

[constgen]

We can change nothing in existing core modules and add a new preset just for purposes when you need to use .eslintrc

Preview:

neutrino.use(require('neutrino-middleware-eslint'))
neutrino.config.module.rule('lint').use('eslint').options({ useEslintrc: true })

[edmorley]

I don't think that would work - several of the other options are needed when using CLIEngine, since they can't be set via an RC file.

[edmorley]

@constgen, I guess the question to ask is: what is your use-case for using a separate .eslintrc rather than setting the config via neutrino?

[constgen]

No problem. We can leave some of them.

Use case

You really don't have a ready Neutrino preset with rules for your project. Libraries may use quite different rules. NodeJS too. .eslintrc is most official and starndart way to keep rules (e.g. in Gist) and share with others. It should work because it is documented in ESLint and everybody is familiar with it.

I understand if we are not going to make it a part of the Core. We can put it on community shoulders. I am ready to support such preset.

[edmorley]

To clarify slightly - this isn't about Neutrino just not wanting to support the standard that is .eslintrc.

It's that it's very hard (if not impossible) to come up with a way of making ESLint configuration consistent, unless everything is run through Neutrino.

As mentioned in the "Background" section of the OP above, the way ESLint is invoked is different between each of neutrino lint, neutrino {build, start, ...} and running ESLint manually. Each of those ways to invoke ESLint supports different options and have caveats, even before we consider the webpack / Neutrino settings aspects. There isn't really much way around this.

[edmorley]

If it's just that you'd like to have the ESLint rules in a different file, then have you tried requiring such a file in your .neutrinorc.js and merging into the neutrino-middleware-eslint config that way?

[constgen]

I thought about this but didn't do for a reason. I have to duplicate the logic of config searching from ESLint .eslintrc -> .eslintrc.json -> .eslintrc.js

[eliperelman]

Been gathering my thoughts, here goes:

I think if you are using eslint middleware, you should configure via .neutrinorc.js.

That's a divergence from the stance we have taken with supporting eslintrc so far, but I think it's the best to maintain going forward. You can continue to use eslintrc.js for editor hints, etc., but modifying linting from there just isn't sustainable.

If you want to use the eslintrc file, you probably shouldn't be using the eslint middleware. You can configure linting yourself in this case.

That said, I still don't know if we should be forcing people away from eslintrc, so I think adding a visible, clear warning in the console may be the right thing to do. It should also mention that changing ESLint configuration outside of neutrinorc is not supported.

Thoughts?

[edmorley]

Yeah I agree, though can't decide warning vs blocking

[eliperelman]

In trying to implement this, I think we are kinda stuck. The changes we made to the API to load middleware outside the call() can't be caught and distinguished from other middleware calls to add eslint configuration. Essentially, this means we don't really know if the user is using custom middleware outside of .neutrinorc.js that matters.

I think we are going to have to relegate this to documentation.

[edmorley]

In trying to implement this, I think we are kinda stuck.

The biggest problem here IMO was when people use "alternative A" from the OP. Unlike "B", this can be detected via the API, and prevented (or warned about) rather than being docs-only, should we wish :-)

ie:

Proposal

IMO we should do one of:

• forbid use of useEslintrc: true entirely (what are the use-cases for using the JSON format .eslintrc instead of Neutrino generated config?)
• allow useEslintrc: true, but neutrino-middleware-eslint should then not set parserOptions itself (ie the middleware does only the bare minimum, leaving the user to manually configure everything else in .eslintrc, so at least the RC file then ends up being much closer to the source of truth)
• allow useEslintrc: true, but output a warning about the problems / emphasise the problems in the docs and source comments

@eliperelman, do any of those sound appealing?

[edmorley]

(Happy to close again if we think the docs addition is fine, but reopened so this doesn't get lost)

[eliperelman]

Ohhhhhhh, now I see! 👀

This is my preference:

• allow useEslintrc: true, but neutrino-middleware-eslint should then not set parserOptions itself (ie the middleware does only the bare minimum, leaving the user to manually configure everything else in .eslintrc, so at least the RC file then ends up being much closer to the source of truth)
• allow useEslintrc: true, but output a warning about the problems / emphasise the problems in the docs and source comments

Let's do both!

[eliperelman]

I want to raise this discussion again. Since we now encourage the use of .eslintrc.js with v9 and can override its options, should we change our stance or approach for a recommended fix here?

[edmorley]

Yeah now that we're using ESLint's CLI for yarn lint, and not the ESLint API, I think that changes things. In addition, eslint-loader can happily combine .eslintrc(.js) with its own settings, making using a non-Neutrino eslintrc very viable (Treeherder is now about to do so).

However to fully support it, we need to stop setting our own options when useEslintrc: true is passed, otherwise they'll overwrite the config file's settings.

In addition whilst looking into this, I see that there are a few issues with the way that @neutrinojs/eslint generates the config at the moment - eg:

• it passes non-existent options to eslint-loader, which ESLint's CLIEngine silently ignores - however it can confuse people looking at --inspect, since it seems like the options are valid/do something
• it doesn't handle all the possible baseConfig properties
• the env vs envs handling confuses users (Linting issue with neutrino v9 #1166 (comment)) - plus means it's harder to copy-paste the config from --inspect and use as the basis for a static eslintrc. We could avoid this since even CLIEngine accepts env so long as it's nested under baseConfig

Changing the useEslintrc: true handling will require separating out exactly what settings are eslint config vs eslint API vs eslint-loader specific, so I believe it makes sense to address all of this at once.