Issues with @neutrinojs/eslint

[edmorley]

Whilst looking into #382 I've found a few issues with the current @neutrinojs/eslint implementation, which I'll fix as part of a refactor. I'm documenting these issues here so as to make the reasoning behind the changes in the upcoming PR clearer.

First some background:

• Our linting approach changed significantly for Neutrino 9 due to Morph Neutrino API and CLI into middleware injectors for external CLI tools #852 - in that for yarn lint we no longer use ESLint's API (via CLIEngine.executeOnFiles()), and instead rely on .eslintrc.js. This is good news since it means we've gone from 3 ways linting may occur (see breakdown in Reducing the potential for confusion if configuring ESLint outside of Neutrino #382) to 2 ways (.eslintrc.js and eslint-loader).
• However there is still a difference in configuration for both, since our generated .eslintrc.js must adhere to the schema here, whereas eslint-loader uses CLIEngine.executeOnText() which has subtly different options.
• This means that @neutrinojs/eslint has to convert between the two formats when using the eslintrc() output handler
• eslint-loader itself is quite frustrating to configure correctly, since:
  • invalid options are silently ignored, since neither the loader nor CLIEngine performs validation (I've filed Validate options passed to eslint-loader  webpack-contrib/eslint-loader#252 and Validate options passed to CLIEngine API eslint/eslint#10272) - making it hard to debug why a config isn't working as expected when the wrong names were used
  • some options have names that are very subtly different between their top-level and baseConfig versions (for example envs vs env)
  • some options have completely different data types when top-level options vs when under baseConfig (for example globals is an array of strings, whereas baseConfig.globals is an object whose keys are the global names, and the value is true or false)
  • some options are available as both top-level and baseConfig variants, whereas for others (such as settings and extends) the only way to set them is under baseConfig
  • the eslint-loader docs only really discuss in detail the options that are used by eslint-loader itself, rather than give examples/list any of the gotchas above (I'll open a PR at some point)
  • the ESLint CLIEngine docs don't explain baseConfig in detail, meaning a user has to work out on their own that it relates to the schema in conf/config-schema.js (I'll open a PR at some point)

On to the issues with @neutrinojs/eslint:

1. The eslintrc mapping process ignores all loader options nested under baseConfig, apart from settings and extends (other possible options are: env, globals, overrides, parser, parserOptions, plugins, root and rules). Notably, overrides cannot be set anywhere other than inside baseConfig (it's not supported as a top-level option), so this bug prevents people from using overrides with eslint-loader.

2. The eslintrc mapping process merges in several loader options that are actually not valid (for example top-level extends, settings, overrides, root options), which encourages people to use broken loader configurations that give different linting results depending on whether used via .eslintrc.js or eslint-loader. This is made worse by (1), since the valid location for many of these options is actually ignored, meaning even if someone knows the correct way to write the configuration, the options will be ignored by the eslintrc mapping.

3. The eslintrc mapping process has an incomplete list of which keys must be filtered out, meaning if options like ignore or reportUnusedDisableDirectives are used, then the .eslintrc.js file will fail to load with a validation error. Any key not specified in the schema must be removed, however there are many eslint-loader options (and more that are undocumented) as well as CLIEngine options. As such a whitelist rather than blacklist approach would probably be simpler long-term.

4. The custom merge implementation here only applies the special rule-merging logic to the top-level rules option, and not also baseConfig.rules. It also doesn't merge rules the same way that ESLint does internally for certain cases. It would be best to use ESLint's own merge function instead to ensure accuracy/consistency and also reduce our maintenance burden.

5. It doesn't validate the options passed to the preset (which would be less of an issue if eslint-loader or CLIEngine did so themselves).

6. It uses the top-level globals and envs options, which whilst slightly more convenient (since they are arrays rather than the object form under baseConfig), requires users to know how they relate to the more commonly seen .eslintrc.js form when trying to understand the output of --inspect. This has been known to cause confusion when debugging (Linting issue with neutrino v9 #1166 (comment)), makes it harder to just copy and paste the config into a static .eslintrc.js, and means one can't simply merge in eg env: { foo: false } to disable an option set by an existing preset (instead requiring manual array filtering).

7. There is no easy way to suppress the default ESLint-config-specific options in @neutrinojs/eslint when a project wants to instead use their own non-generated .eslintrc.js (see Reducing the potential for confusion if configuring ESLint outside of Neutrino #382).

I have a branch locally that fixes these - will open a PR once I've finished updating the docs/tests.

[edmorley]

• the ESLint CLIEngine docs don't explain baseConfig in detail

I've open PR eslint/eslint#10995 to improve these docs.