Skip to content
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: Refactor eslint-loader and eslintrc config generation #1182

Merged
merged 1 commit into from Oct 21, 2018

Conversation

@edmorley
Copy link
Member

edmorley commented Oct 19, 2018

  • Adds option validation to @neutrinojs/eslint, so that it now throws when passed options that are not supported by eslint-loader.
  • The eslintrc conversion now correctly handles all supported options under baseConfig rather than silently ignoring some of them.
  • The eslintrc conversion no longer looks for settings in locations that are invalid for eslint-loader (such as top-level settings, extends, root or overrides options).
  • The eslintrc conversion now no longer errors if passed options that were previously missing from the omit() list, such as ignore and reportUnusedDisableDirectives.
  • The custom lint merge() function has been replaced with ESLint's own merging function, which correctly handles several cases that were broken before.
  • All non-loader related configuration has now been moved under the baseConfig key, since the options under it more closely relate to the options that users will have previously seen in .eslintrc files, thereby reducing user-confusion and making it possible to copy and paste the contents of the baseConfig section to a static .eslintrc.*.
  • Support has been added for projects that want to use their own non-generated .eslintrc.* file. They now only need to set useEslintrc to true, which will make the linting presets only set the loader-related options, preventing conflicts with the options set in their static .eslintrc.* file.
  • The @neutrinojs/loader-merge middleware has been removed, since its functionality was not sufficient for merging lint configuration correctly, and was not used for any other purpose.

Fixes #1181.
Fixes #382.

@edmorley edmorley added this to the v9 milestone Oct 19, 2018
@edmorley edmorley self-assigned this Oct 19, 2018
@edmorley edmorley force-pushed the edmorley:eslint-refactor branch from a67f7b6 to d9ac70f Oct 19, 2018

### Information

If you want your preset or middleware to also extend from another **ESLint configuration or preset** that you have made

This comment has been minimized.

Copy link
@edmorley

edmorley Oct 19, 2018

Author Member

This is now covered by all of the defaults above being listed under baseConfig instead

a dependency, you must use `baseConfig.extends` rather than just `extends`. This is a limitation of ESLint, not this
middleware.

### Override configuration

This comment has been minimized.

Copy link
@edmorley

edmorley Oct 19, 2018

Author Member

This section is unnecessary IMO (given most people should not be using this method of customisation), and given the pain points we see around lint configuration, I think we need to take a strict "less is more" approach to what we put in the docs for it.

@@ -168,64 +168,26 @@ module.exports = {
use: [
['@neutrinojs/airbnb-base', {
eslint: {
rules: {

This comment has been minimized.

Copy link
@edmorley

edmorley Oct 19, 2018

Author Member

It's worth noting that this form still works, it's just not what we're using for the defaults / what we're recommending. Any preset that pass in the old form will still correctly override the values under baseConfig, since ESLint's CLIEngine (plus our eslintrc conversion function) merges them in such a way that the top-level options take precedence.

],
'babel/no-invalid-this': 'off',
'babel/no-unused-expressions': [
'warn',

This comment has been minimized.

Copy link
@edmorley

edmorley Oct 19, 2018

Author Member

This case is an example of one of the tests that would have failed previously - since before the resultant config would have discarded the rule settings, and just had the value of 'warn'.

@@ -42,68 +42,53 @@ const eslint = require('@neutrinojs/eslint');
// Usage shows default values
neutrino.use(eslint, {
test: neutrino.regexFromExtensions(), // Uses extensions from neutrino.options.extensions
// Uses extensions from neutrino.options.extensions

This comment has been minimized.

Copy link
@edmorley

edmorley Oct 19, 2018

Author Member

I've adjusted the wrapping here so the code block in the docs doesn't need to have horizontal scroll bars.

env: {
es6: true
},
extends: [],

This comment has been minimized.

Copy link
@edmorley

edmorley Oct 19, 2018

Author Member

I've deliberately included empty config options here to give hints (both in the docs and via --inspect) as to where the correct place is to pass each of the options.

const { ConfigurationError, DuplicateRuleError } = require('neutrino/errors');

const merge = (source, destination) => {

This comment has been minimized.

Copy link
@edmorley

edmorley Oct 19, 2018

Author Member

This old merge function missed several cases:

  • it didn't apply the rule-merge to baseConfig.rules
  • the rule-merge doesn't keep the settings of the old rule, when wanting to just adjust the off/warn/error level (unlike eslint's own merge)
  • it doesn't merge overrides correctly

See these testcases:
https://github.com/eslint/eslint/blob/5984820670c3085becc98b75827fc4899001e605/tests/lib/config/config-ops.js#L305-L319
https://github.com/eslint/eslint/blob/5984820670c3085becc98b75827fc4899001e605/tests/lib/config/config-ops.js#L481-L494

: merge(lintOptions, {
baseConfig: {
env: {
'jest/globals': true

This comment has been minimized.

Copy link
@edmorley

edmorley Oct 19, 2018

Author Member

I've converted the top-level envs array form to the object form under baseConfig.env, since:

  • users will be more familiar with it (ref #1166 (comment))
  • it then means it's possible to override an env by passing eg 'jest/globals': false (with arrays manual filtering would have to be performed)
  • it means the resultant --inspect output is much closer to a form that can be copy-pasted into an .eslintrc.* file
Copy link
Member

eliperelman left a comment

💯

* Adds option validation to `@neutrinojs/eslint`, so that it now
  throws when passed options that are not supported by eslint-loader.
* The eslintrc conversion now correctly handles all supported options
  under `baseConfig` rather than silently ignoring them.
* The eslintrc conversion no longer looks for settings in locations
  that are invalid for eslint-loader (such as top-level `settings`,
  `extends`, `root` or `overrides` options).
* The eslintrc conversion now no longer errors if passed options that
  were previously missing from the `omit()` list, such as `ignore`
  and `reportUnusedDisableDirectives`.
* The custom lint `merge()` function has been replaced with ESLint's
  own merging function, which correctly handles several cases that
  were broken before.
* All non-loader related configuration has now been moved under the
  `baseConfig` key, since the options under it more closely relate
  to the options that users will have previously seen in `.eslintrc`
  files, thereby reducing user-confusion and making it possible to
  copy and paste the contents of the `baseConfig` section to a static
  `.eslintrc.*`.
* Support has been added for projects that want to use their own
  non-generated `.eslintrc.*` file. They now only need to set
  `useEslintrc` to `true`, which will make the linting presets only
  set the loader-related options, preventing conflicts with the
  options set in their static `.eslintrc.*` file.
* The `@neutrinojs/loader-merge` middleware has been removed, since
  its functionality was not sufficient for merging lint configuration
  correctly, and was not used for any other purpose.

Fixes #1181.
Fixes #382.
@edmorley edmorley force-pushed the edmorley:eslint-refactor branch from d9ac70f to 175a964 Oct 21, 2018
@edmorley edmorley merged commit a13feee into neutrinojs:master Oct 21, 2018
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details
@edmorley edmorley deleted the edmorley:eslint-refactor branch Oct 21, 2018
edmorley added a commit to edmorley/neutrino that referenced this pull request Nov 23, 2018
The refactor in neutrinojs#1182 inadvertently used babel-merge's `merge()` to
merge ESLint configuration in this preset rather than deepmerge, since
the existing `merge` import was not the same as in other presets from
which the eslint merging block was copied.

In a later PR I think we should stop exporting a merge function from
`@neutrinojs/compile-loader` named `merge`, to avoid the reduce the
chance of this kind of naming conflict.

Fixes:
taskcluster/taskcluster-web-server#59 (comment)
edmorley added a commit to edmorley/neutrino that referenced this pull request Nov 23, 2018
The refactor in neutrinojs#1182 inadvertently used babel-merge's `merge()` to
merge ESLint configuration in this preset rather than deepmerge, since
the existing `merge` import was not the same as in other presets from
which the lint rule merging block was copied.

In a later PR I think we should stop exporting a merge function from
`@neutrinojs/compile-loader` named `merge`, to avoid the reduce the
chance of this kind of naming conflict.

Fixes:
taskcluster/taskcluster-web-server#59 (comment)
edmorley added a commit that referenced this pull request Nov 23, 2018
The refactor in #1182 inadvertently used babel-merge's `merge()` to
merge ESLint configuration in this preset rather than deepmerge, since
the existing `merge` import was not the same as in other presets from
which the lint rule merging block was copied.

In a later PR I think we should stop exporting a merge function from
`@neutrinojs/compile-loader` named `merge`, to avoid the reduce the
chance of this kind of naming conflict.

Fixes:
taskcluster/taskcluster-web-server#59 (comment)
edmorley added a commit to edmorley/neutrino that referenced this pull request Sep 9, 2019
In neutrinojs#1182, the previous broken ESLint config merging functionality was
replaced by using an internal ESLint `merge` function from `config-ops`.

Unfortunately in ESLint 6 that function no longer exists in a form that
is easy for us to use, so in order to support ESLint 6, I have replaced
it with simpler manual merges. These are virtually identical, with one
difference - `rules` are now merged by a later rule entry completely
replacing the first, rather than updating it.

For example, merging these:
`'foo': ['error', {'someSetting': false}]`
`'foo': 'warn'`

...used to give:
`'foo': ['warn', {'someSetting': false}]`

...but now results in:
`'foo': 'warn'`

I think this difference is a reasonable compromise for not having to
rely on ESLint internals and/or implement our own more complex merging.
I also believe hitting this will be rare, since:
* it doesn't affect merging with rules defined inside an `extends`
  external file
* it won't make a difference for rules that were already at default
  settings (or that were overridden with custom settings in the later
  rule definition being merged in)

This more simplistic merging behaviour is also what's used by Neutrino 8
(neutrinojs#1182 landed for Neutrino 9 only) - and we still have the other bug
fixes included in neutrinojs#1182 that were the primary motivation for refactoring
in the first place.

Refs neutrinojs#1423.
edmorley added a commit that referenced this pull request Sep 9, 2019
In #1182, the previous broken ESLint config merging functionality was
replaced by using an internal ESLint `merge` function from `config-ops`.

Unfortunately in ESLint 6 that function no longer exists in a form that
is easy for us to use, so in order to support ESLint 6, I have replaced
it with simpler manual merges. These are virtually identical, with one
difference - `rules` are now merged by a later rule entry completely
replacing the first, rather than updating it.

For example, merging these:
`'foo': ['error', {'someSetting': false}]`
`'foo': 'warn'`

...used to give:
`'foo': ['warn', {'someSetting': false}]`

...but now results in:
`'foo': 'warn'`

I think this difference is a reasonable compromise for not having to
rely on ESLint internals and/or implement our own more complex merging.
I also believe hitting this will be rare, since:
* it doesn't affect merging with rules defined inside an `extends`
  external file
* it won't make a difference for rules that were already at default
  settings (or that were overridden with custom settings in the later
  rule definition being merged in)

This more simplistic merging behaviour is also what's used by Neutrino 8
(#1182 landed for Neutrino 9 only) - and we still have the other bug
fixes included in #1182 that were the primary motivation for refactoring
in the first place.

Refs #1423.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants
You can’t perform that action at this time.