New: Add `overrides` and `files` configuration options (refs #3611) #7177

CrabDude · 2016-09-17T02:12:08Z

What is the purpose of this pull request? (put an "X" next to item)

[X] Documentation update
[X] Other, please explain:

The ability to override config values based on glob pattern matching relative to the config path.

#3611

Please check each item to ensure your pull request is ready:

I've read the pull request guide
I've included tests for my change
I've updated documentation for my change (if appropriate)

What changes did you make? (Give an overview)

Added the ability to add configurations targeting specific files based on glob patterns:

{
    rules: {
        semi: "error"
    },
    overrides: [
        {
            files: ["foo/*.js"],
            extends: "eslint:recommended",
            rules: {
                semi: "warn"
            }
        }

    ]
}

This mostly follows @nzakas' micro-proposal, but with grunt-style aggregating glob pattern arrays.

Importantly, this means files within the same directory no longer necessarily share the same config. Consequently, configs can no longer be cached per directory. To avoid performance degradation due to lack of directory config caching, a new caching strategy was implemented, which necessitated a refactoring of lib/config.js.

Specifically, a vector based caching was implemented, where a vector is an array of absolute config file paths within a hierarchy that apply to a given file, including separate entries for "overrides" sub-configs. This hierarchy can then be used to generate a hash for caching, or to merge the config hierarchy and then cached and returned from Config.prototype.getConfig(filePath).

Example
Given the following config hierarchy:

project-root
├── app
│   ├── lib
│   │   ├── foo.js
│   │   ├── fooSpec.js
│   │   ├── bazSpec.js
│   ├── components
│   │   ├── bar.js
│   │   ├── barSpec.js
│   ├── .eslintrc
├── server
│   ├── server.js
│   ├── serverSpec.js
├── .eslintrc

With corresponding configs:

// project-root/.eslintrc
{
  rules: { "no-unused-expression": 2, semi: 0 },
  overrides: [{
    files: "**/*Spec.js",
    rules: { "no-unused-expression": 0 }
  }]
}

// project-root/app/.eslintrc
{
  env: { node: true },
  rules: { no-console: 1 },
  overrides: [
    {
      files: "lib/*Spec.js",
      rules: { "no-console": 0 }
    },
    {
      files: "components/*Spec.js",
      env: { browser: true }
    },
    {
      files: "**/*Spec.js",
      env: { "mocha": true }
    }
  ]
}

For project-root/app/lib/fooSpec.js, where /project-root/ is the absolute path to project-root, the corresponding config vector and caching hash (included for illustration, but not important) would be:

// Config vector
[
  '/project-root/.eslintrc',
  '/project-root/.eslintrc:0',
  '/project-root/app/.eslintrc',
  '/project-root/app/.eslintrc:0',
  // Note the absence of sub-config 1 here
  '/project-root/app/.eslintrc:2'
]

// Config vector hash
'/project-root/.eslintrc,/project-root/.eslintrc:0,/project-root/app/.eslintrc,/project-root/app/.eslintrc:0,/project-root/app/.eslintrc:2'

And the resulting corresponding config value (cached at config.mergedCache[hash]):

{
  env: {
    node: true,
    mocha: true
  }
  rules: {
    "no-unused-expression": 0,
    "no-console": 0,
    semi: 0
  }
}

// Where the cache includes the properly expanded "env" values

And getConfig('project-root/app/lib/bazSpec.js') would load from cache.

Details
To maximize the performance benefits of refactoring the config loader caching layer, several other layers of caching (on the config instance) were implemented including config file contents caching, iterative config caching (a merged sub-vector cache), config hierarchy caching by vector and directory-based local config file hierarchy caching.

As a result, from the previous example, all of the files within project-root would load from various degrees of partial cache.

Results
The net performance gain from these caching improvements is a consistent 15% performance increase when run against the Pinterest web codebase regardless of whether an override config is utilized.

Is there anything you'd like reviewers to focus on?

Edge cases.

@lo1tuma

EDIT: Added an expanded example.

)

eslintbot · 2016-09-17T02:12:09Z

LGTM

nzakas · 2016-09-19T18:13:37Z

@CrabDude wow, thanks for digging into this. These are some major changes that I don't have enough energy to get to today, but I did want to leave a note to let you know I'll definitely look at this during this week (hopefully Wednesday).

I do have a couple of questions in the meantime:

How many overrides are applied to a file? Is is just the first matching one or all matching overrides?
What happens if an override extends a config that also has overrides?

Thanks again!

CrabDude · 2016-09-19T18:25:50Z

@nzakas Thanks for your consideration.

I added an expanded example just before you commented, so you may not yet have seen it, but it illustrates that "overrides" sub-configs are treated as peers within the config hierarchy, so just as an arbitrary number of .eslintrc files can exist within a hierarchy, so too can an arbitrary number of overrides occur.

As a result of this implementation, ATM, the same config merging rules apply to sub-configs. So...

All matching overrides w/n the hierarchy are applied and merged as full configs (as though there were an .eslintrc in an interstitial directory.
~~Presently,~~ (I need to push the fix for this) all configs are expanded as regular configs through loadConfig, so "plugsins", "extends", etc... should work, but since the "overrides" logic is manual on a per-config-file basis, nested "overrides" I assume will get merged by ConfigFile.applyExtends in loadConfig (I need to check on this also and some tests).

Thanks for the feedback. I'll add some more tests for the "extends", "plugins" and nested "extends" "overrides" cases.

CrabDude · 2016-09-20T15:58:58Z

Still working on a fix for nested overrides. Merging it with the fix for extends in overrides. Will push ~~later today~~ soon.

nzakas

Once again, thanks for doing this. I think overall you're heading in the right direction.

One thing I'd like to explore, though, is if we can pull some of the logic out of config.js and move it into either config-ops.js or config-file.js. The config.js file is really difficult to unit test, so we started breaking it apart into the smaller files (didn't quite finish).

For instance, it seems like the logic to say "given this config and this filename, give me a config that has overrides merged in could potentially live in config-ops.js (as it doesn't rely on the filesystem). And maybe the actual merging could take place in config-file.js somewhere. I'm open to feedback on this point, I just really want to keep config.js as small as possible because of the difficulty in unit testing.

nzakas · 2016-09-22T16:54:50Z

docs/user-guide/configuring.md

@@ -703,6 +703,65 @@ module.exports = {
 }
 ```

+## Glob Pattern based Configuration


Maybe "Configuration Based on Glob Patterns"?

Fixed in #8081.

nzakas · 2016-09-22T16:55:42Z

docs/user-guide/configuring.md

@@ -703,6 +703,65 @@ module.exports = {
 }
 ```

+## Glob Pattern based Configuration
+
+Sometimes a more fine-controlled configuration is necessary e.g. if the configuration for files within the same directory has to be different. Therefore you can provide configurations that will only apply to files that match a specific glob pattern (based on [minimatch](https://github.com/isaacs/minimatch)).


Is there a way to not use minimatch? I'm concerned that the differences between minimatch and globs that we use on the command line will be confusing for users (why will one thing work on the command line but not in the config?).

See also: https://github.com/eslint/eslint/blob/master/lib/util/glob-util.js

I had a look at using the existing glob code, but it seems to be a different use case (it uses node-glob, which is the filesystem-aware version of minimatch). In this case, we've already retrieved a list of candidate files from the filesystem, and we just use minimatch to filter them. Since node-glob uses minimatch under the hood, I think the results should be essentially the same, so I don't think there's too much potential for confusion.

nzakas · 2016-09-22T16:57:25Z

lib/config.js

    this.ignore = options.ignore;
    this.ignorePath = options.ignorePath;
    this.cache = {};
+    this.mergedCache = {};


Can you add some comments describing what each of these does? Lots of "caches" are hard to distinguish from one another.

Many comments added in #8081.

nzakas · 2016-09-22T16:59:09Z

lib/config.js

+
+
+/**
+ * Get the vector of applicable configs from the hierarchy for a given file (glob matching occurs here).


I'm unclear on what "vector" is in this context. Can you use a different word?

I tried to think of a better word for this but came up blank. I fell back to describing it in detail in comments in the JSDoc:

A vector is an array of config file paths, each optionally followed by one or more numbers that correspond to the indices of nested config blocks within the config file's overrides section.

nzakas · 2016-09-22T16:59:48Z

lib/config.js

+
+/**
+ * Merges all configurations for a given config vector
+ * @param {Array} vector array of config file paths or relative override indices


An array of what? If strings, then use string[]. Otherwise, please use a type-specific array. (And same note about the word "vector", I just don't understand what that means.)

Fixed in #8081.

nzakas · 2016-09-22T17:00:31Z

package.json

@@ -54,6 +54,10 @@
    "json-stable-stringify": "^1.0.0",
    "levn": "^0.3.0",
    "lodash": "^4.0.0",
+    "lodash.clonedeep": "^3.0.1",


We already include lodash as a whole, so there's no need to include individual methods.

Fixed in #8081.

nzakas · 2016-09-22T17:05:24Z

lib/config.js

-    if (this.useSpecificConfig) {
-        debug("Merging command line config file");
+/**
+ * Get the local config hierarchy for a given directory.


Similar to previous note about adding comments, it's unclear what "local" means here. Can you be more specific in this comment?

JSDoc has been rewritten and expanded in #8081.

nzakas · 2016-09-22T17:06:25Z

tests/fixtures/config-hierarchy/file-structure.json

@@ -48,6 +48,12 @@
            ".eslintrc": "{\n    \"env\": {\n        \"commonjs\": true\n    }\n}\n"
        }
    },
+    "overrides": {


Just double-checking: are you sure this won't interfere with already-existing tests?

All the tests still pass, so this seems to be OK! :)

CrabDude · 2016-10-02T02:42:19Z

Should've mentioned I've been OOO since 9/22. Will pick up where we left off this week.

nzakas · 2016-10-31T20:22:09Z

Just a heads up: we have moved to a new CLA checker on pull requests. Even if you've previously signed our CLA, we will need to you sign the new one. To do so, look at the status checks for licence/cla and click the "Details" link. Sorry for the inconvenience.

vitorbal · 2016-11-11T18:11:49Z

@CrabDude thanks again for all your hard work on this one! Definitely appreciated.

This seems like such an awesome change, it would be a shame to let it fall through the cracks. Is there anything that we could do to help move this forward?

CrabDude · 2016-11-14T22:39:04Z

@vitorbal Per #7177 (comment), it's feature complete, but needs to be refactored into more testable files / architecture, with the corresponding tests updated (and added). If you were so motivated, I suppose you could write some tests for the spec defined here, as it would help with the refactor.

Also, the encouragement helps. 🙂

kaicataldo · 2017-01-08T06:56:06Z

@CrabDude Friendly ping again. Anything you need from us to help finish this up?

CrabDude · 2017-01-09T18:31:09Z

@kaicataldo While I still intend to get back to this, I'm not sure when that will be. If you or anyone wants to speed this up, please feel free to write tests that cover the implementation as documented above.

nzakas · 2017-01-14T17:41:18Z

This is on my todo list for when I'm able to spend some extended time at a computer.

smably · 2017-02-01T01:29:20Z

I rebased this onto latest master and fixed the conflicts (plus a couple of the really easy fixes from @nzakas' code review above): https://github.com/smably/eslint/tree/overrides

I haven't had a chance to really dig into the logic, and I don't know whether I will have the time to do it, but I hope this is at least a start for whoever takes this on.

@nzakas -- do you still intend to come back to this? If not, I might be able to dig a bit deeper into this sometime later this week.

CrabDude · 2017-02-01T18:21:16Z

@smably Thanks so much for wading into this! Definitely motivating to not be working on this alone.

I don't believe @nzakas ever had/has any plans to work on this. I plan to finish this, but had to come back to it. I should be able to actively start working on this again.

Would you be open to working on it together a bit? We can figure out how specifically you would like to contribute once I dive back in.

Off the top of my head there are 2 tasks:

Refactor per @nzakas request
Comprehensive tests

not-an-aardvark · 2017-02-01T19:58:50Z

I don't believe @nzakas ever had/has any plans to work on this. I plan to finish this, but had to come back to it. I should be able to actively start working on this again.

Thanks! To clarify re. @nzakas's plans to work on this, I think @smably was referring to #7177 (comment).

smably · 2017-02-02T03:34:19Z

@CrabDude Awesome, glad to hear you're able to pick this up again. I'd love to help, though I think it'll be a few days, at least, before I can dedicate any time to this. Keep me posted on how I can best contribute!

smably · 2017-02-05T23:08:33Z

I had a look into splitting some of the logic out of config.js but ran into issues with caching support. config-file.js and config-ops.js are both collections of stateless helper functions, so I'm not sure how best to give them access to the caches (or whether that's even a good idea at all). One thought I had was to split all the caching-related logic out into a new file, config-cache.js, but I will defer to someone more experienced to advise on whether or not that is a good idea. I'm happy to help with the refactoring, but I might need a bit of help to get started, given the complexity of the caching logic.

In any case, I updated my version of the branch with a couple of other fixes and verified that all tests are still passing. Next time I have a chance to look at this, I'll see about adding some more comprehensive tests, though I do wonder whether they'll have to be rewritten anyway to support the refactor...

CrabDude · 2017-02-06T20:53:06Z

@smably The refactor should not affect the tests, as the tests should be written against the spec, as defined in my OP at the top. This is one of the reasons why I pointed to new tests that cover the spec as an easy task for anyone looking to contribute.

Thanks for the useful thoughts on the refactor, and proposing a config-cache.js.

alberto · 2017-02-07T07:56:13Z

@CrabDude I'm sorry, but we need you to sign our new CLA. Thanks!

smably · 2017-02-15T00:22:02Z

Added a bunch of updates and opened a new PR: #8081.

alberto · 2017-02-19T17:55:52Z

@CrabDude I'm sorry for the insistence, but we need you to sign our new CLA. Even if you don't plan to finish it (we understand, life changes priorities) we would need your signature to be able to continue from here.

Thanks for your contribution!

kaicataldo · 2017-04-20T22:08:44Z

Closing this as it seems abandoned and the same work is being in #8081. Please feel free to reopen if that's not the case @CrabDude, and thanks for contributing :)

CrabDude · 2017-06-29T22:39:06Z

@alberto Not sure if it's still relevant, but signed.

alberto · 2017-06-30T08:41:23Z

Yes, thank you @CrabDude!

New: Add overrides and files configuration options (refs eslint#3611

23e2b85

)

jquerybot added the CLA: Valid label Sep 17, 2016

CrabDude mentioned this pull request Sep 21, 2016

Failed to load plugin react: Cannot find module 'eslint-plugin-react' every time I open a .js file microsoft/vscode-eslint#71

Closed

nzakas requested changes Sep 22, 2016

View reviewed changes

nzakas mentioned this pull request Oct 1, 2016

New: Add excludes and includes configuration options (refs #3611) #7073

Closed

vitorbal mentioned this pull request Oct 9, 2016

File extensions in .eslintrc #7324

Closed

nzakas removed the CLA: Valid label Oct 31, 2016

vitorbal added core Relates to ESLint's core APIs and features evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Nov 11, 2016

smably mentioned this pull request Feb 15, 2017

New: Add overrides and files configuration options (refs #3611) #8081

Merged

2 tasks

smably pushed a commit to smably/eslint that referenced this pull request Feb 26, 2017

Existing work from eslint#7177, rebased and ES6ified

daed3b1

smably pushed a commit to smably/eslint that referenced this pull request Feb 27, 2017

Existing work from eslint#7177, rebased and ES6ified

4680d10

smably pushed a commit to smably/eslint that referenced this pull request Mar 13, 2017

Existing work from eslint#7177, rebased and ES6ified

cfa3089

kaicataldo closed this Apr 20, 2017

smably pushed a commit to smably/eslint that referenced this pull request Apr 25, 2017

Existing work from eslint#7177, rebased and ES6ified

61b1f4c

smably pushed a commit to smably/eslint that referenced this pull request May 7, 2017

Existing work from eslint#7177, rebased and ES6ified

b58ce6c

nevir mentioned this pull request Dec 11, 2017

Discussion: Extending core ESLint rules to "just work" with TS-specific nodes eslint/typescript-eslint-parser#77

Closed

kuzyn mentioned this pull request Jan 8, 2018

Issue with no-unpublished-require and devDependencies modules mysticatea/eslint-plugin-node#47

Closed

eslint-deprecated bot locked and limited conversation to collaborators Feb 6, 2018

eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Feb 6, 2018



		/**
		* Get the vector of applicable configs from the hierarchy for a given file (glob matching occurs here).

New: Add overrides and files configuration options (refs #3611) #7177

New: Add overrides and files configuration options (refs #3611) #7177

Conversation

CrabDude commented Sep 17, 2016 • edited

eslintbot commented Sep 17, 2016

nzakas commented Sep 19, 2016

CrabDude commented Sep 19, 2016 • edited

CrabDude commented Sep 20, 2016 • edited

nzakas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nzakas Sep 22, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CrabDude commented Oct 2, 2016

nzakas commented Oct 31, 2016

vitorbal commented Nov 11, 2016

CrabDude commented Nov 14, 2016

kaicataldo commented Jan 8, 2017

CrabDude commented Jan 9, 2017

nzakas commented Jan 14, 2017

smably commented Feb 1, 2017

CrabDude commented Feb 1, 2017

not-an-aardvark commented Feb 1, 2017

smably commented Feb 2, 2017

smably commented Feb 5, 2017

CrabDude commented Feb 6, 2017

alberto commented Feb 7, 2017

smably commented Feb 15, 2017

alberto commented Feb 19, 2017

kaicataldo commented Apr 20, 2017

CrabDude commented Jun 29, 2017

alberto commented Jun 30, 2017

New: Add `overrides` and `files` configuration options (refs #3611) #7177

New: Add `overrides` and `files` configuration options (refs #3611) #7177

CrabDude commented Sep 17, 2016 •

edited

CrabDude commented Sep 19, 2016 •

edited

CrabDude commented Sep 20, 2016 •

edited

nzakas Sep 22, 2016 •

edited