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

[smably]

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

• Documentation update
• Other, please explain:

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

What changes did you make? (Give an overview)

Starting from @CrabDude's work in #7177, I rebased the branch against the latest master, addressed several comments from the original PR, rewrote and expanded many of the JSDoc comments, tweaked documentation, and split as much logic as I could from lib/config.js into lib/config/config-cache.js, lib/config/config-file.js, and lib/config/config-ops.js.

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

Validating the approach -- does the separation of logic make sense? Is this still on the right track? Any edge cases that need tests?

[jsf-clabot]

All committers have signed the CLA.

[eslintbot]

LGTM

[mention-bot]

@smably, thanks for your PR! By analyzing the history of the files in this pull request, we identified @gyandeeps, @nzakas and @sethmcl to be potential reviewers.

[not-an-aardvark]

Thanks! Sorry, looks like we lost track of this PR.

To make this easier to review, how difficult would it be to split up @CrabDude's commit and your improvements? It's currently a bit difficult to identify which parts are different from #7177.

Also, does this implement the changes that @nzakas requested in #7177 (review)?

[smably]

Hi @not-an-aardvark, no worries. Unfortunately it would be pretty difficult to split up, because I had to rewrite large parts of @CrabDude's work to ES6-ify it (the diff was pretty much useless because the indentation changed when I moved everything inside a class). I can see about trying to split it up, but I think it might be best to review this from scratch as the changes are pretty substantial (particularly when it comes to the caches). Let me know what you think.

I'll add some comments on @nzakas' review on the original PR. Sorry I couldn't maintain the original PR -- not sure if there's a way to do that on a PR from a personal fork.

If there is still potential for this to get merged, I'll definitely see if I can rebase and fix the conflict later today.

[eslintbot]

LGTM

[smably]

Added comments to the original PR and fixed the merge conflict.

[eslintbot]

Thanks for the pull request, @smably! I took a look to make sure it's ready for merging and found some changes are needed:

• The commit summary needs to begin with a tag (such as Fix: or Update:). Please check out our guide for how to properly format your commit summary and update it on this pull request.

Can you please update the pull request to address these?

(More information can be found in our pull request guide.)

[smably]

@not-an-aardvark Turned out I had I still had the branch with the unsquashed version of this locally. I've updated this PR with the contents of the unsquashed branch. The first commit is the existing work, with some updates to make it mergeable but no functional changes. The subsequent commits represent the new stuff I've changed or added.

[not-an-aardvark]

Thanks!

I'm planning to review this sometime in the next day or two. I'm not very familiar with the part of the codebase that this changes, so I'm hoping someone else can also review it.

[eslintbot]

Thanks for the pull request, @smably! I took a look to make sure it's ready for merging and found some changes are needed:

• The commit summary needs to begin with a tag (such as Fix: or Update:). Please check out our guide for how to properly format your commit summary and update it on this pull request.

Can you please update the pull request to address these?

(More information can be found in our pull request guide.)

[ilyavolodin]

I haven't gone through everything yet, but some of the small issues I've found so far.

[ilyavolodin]

use path.relative(config, filePath) instead.

[smably]

Fixed.

[ilyavolodin]

It looks like for a single config with 1 override we are going to update the same key in the Map 3 times. Once here, and twice inside getConfigFromVector function. Do we need to set it in the getConfigFromVector? I think pretty much all code paths goes through getConfig.

[ilyavolodin]

Nit: I'd rather use decrementing while loop. Easier to read for me.

[smably]

Fixed.

[ilyavolodin]

We are not protecting against "../" patterns anywhere. Those will basically never work (or at least will not work in most cases). They are also going to be really hard to wrap your head around, so I think we should filter out and ignore them.

[ilyavolodin]

Also, I'm slightly concerned about absolute paths as well.

[smably]

Fixed both (will now ignore patterns with absolute paths or that contain ..).

[ilyavolodin]

We now have two config caches. One for config files themselves (used in file-finder.js) and one for config vectors. Do we need both of them?

[ilyavolodin]

This is not a very helpful comment, but all of the code that goes below seems like a black magic to me. Branching on array value types, hardcoded array indexes, etc. I sort of understand what the code is doing, but it's not very readable and is really hard to follow. Not sure what can be done to improve this (as I said, not a very helpful comment).

[not-an-aardvark]

Agreed. Using array methods like .find() instead of loops would probably be a good idea.

[ilyavolodin]

Performance numbers look pretty good (although we are not testing for anything with overrides):

Before:

Single File:
  CPU Speed is 3500 with multiplier 13000000
  Performance Run #1:  5168.491679ms
  Performance Run #2:  3631.0344720000003ms
  Performance Run #3:  3644.98604ms
  Performance Run #4:  3630.521594ms
  Performance Run #5:  3626.206455ms

  Performance budget ok:  3631.0344720000003ms (limit: 3714.285714285714ms)


Multi Files (0 files):
  CPU Speed is 3500 with multiplier 39000000
  Performance Run #1:  13425.494453ms
  Performance Run #2:  13795.64753ms
  Performance Run #3:  13960.317346ms
  Performance Run #4:  13530.801789ms
  Performance Run #5:  13425.006736ms

  Performance budget exceeded: 13530.801789ms (limit: 11142.857142857143ms)

After:

Single File:
  CPU Speed is 3500 with multiplier 13000000
  Performance Run #1:  3705.140804ms
  Performance Run #2:  3714.937271ms
  Performance Run #3:  3817.76126ms
  Performance Run #4:  3948.772472ms
  Performance Run #5:  3707.20636ms

  Performance budget exceeded: 3714.937271ms (limit: 3714.285714285714ms)


Multi Files (0 files):
  CPU Speed is 3500 with multiplier 39000000
  Performance Run #1:  13963.964251ms
  Performance Run #2:  13807.104047ms
  Performance Run #3:  14118.498804ms
  Performance Run #4:  14127.543946ms
  Performance Run #5:  13701.669443ms

  Performance budget exceeded: 13963.964251ms (limit: 11142.857142857143ms)

[not-an-aardvark]

Thanks again for the pull request. My main concern at the moment is that some of the code here is hard to follow (which was also a problem before these changes 😄). Right now it's fairly difficult to reason about the code, so I'm concerned that it might contain bugs that we won't be able to catch.

A few concrete suggestions:

• The config logic uses "vector" arrays, which seem to be holding a lot of different types of information in arrays. I'm wondering if this abstraction is necessary -- if so, then I think we should use a better datatype, and if not then we should split it up or remove it.
• Using mutable state less will also make this easier to reason about. For example, some functions in this PR end up reassigning and mutating variables many times, and it's difficult to track what's going on. Using const and splitting up logic into helper functions will help with this.
• This logic has a lot of caches. This is fine, but right now the cache logic seems to be interspersed with everything else, so it's hard to figure out what's cached at what times, etc. If something cached, I think it's much better to have the caching behavior encapsulated by a helper function (e.g. "If this thing is in the cache, return it, otherwise get it from the filesystem"). Then the rest of the code shouldn't care whether the cache was used.

[not-an-aardvark]

Is there a reason to use an array with strings and numbers here, rather than some other data structure? An array containing multiple datatypes with different meanings seems like a code smell.

Maybe an object would be better than a "vector":

{
  configFilePath: '/home/john/project/app/.eslintrc',
  overrideConfigs: [
    {
      rules: { /* ... */ }
    },
    {
      rules: { /* ... */ }
    }
  ]
}

[not-an-aardvark]

Initializing config to an empty object seems unnecessary here, because it's reassigned immediately below.

[smably]

Fixed.

[not-an-aardvark]

Using a more organized object instead of a "vector" with multiple datatypes would also make this easier to read.

[smably]

Fixed. I refactored all the code relating to vectors (still can't think of a better name) and it is now vastly more readable at the expense of a bit of performance.

[not-an-aardvark]

We already have a dependency on glob, which we use for parsing CLI options. Can you use that instead of adding another glob-parsing dependency?

[smably]

I agree that this seems redundant, but glob is specifically for matching patterns against files and directories on disk. In this case, minimatch is being used to check a file path (which is already confirmed to exist) against a specified pattern, which glob is not able to do. And glob actually uses minimatch under the hood -- it just adds the filesystem traversal logic -- so the results should be the same.

[not-an-aardvark]

Nitpick: If you use an arrow function here, you won't need to add a this argument at the end.

[smably]

Fixed.

[not-an-aardvark]

I'm not sure I'm understanding this logic -- aren't all local config files in the same directory? Why does the cache need to be checked separately for each iteration of this loop?

[smably]

Local config files can be in the current directory, but they can also be in the parent directory, or any directory above the current one in the tree.

findLocalConfigFiles actually finds all the config files in the filesystem above the specified directory. So localConfigFiles will contain config files in various directories above the current one, some of which may have already been cached. This logic stops walking up the directory tree as soon as we hit a directory that's already in the local hierarchy cache, because it already contains all the configs for that directory and its parents.

[not-an-aardvark]

This logic is hard to follow. Would it be possible to split it into multiple functions?

Specifically, it's not clear to me how the presence of a cache changes the control flow. Intuitively, I would expect the cache to be an alternate way to populate configs, but in that case its effects should be isolated, and it shouldn't be necessary to refer to the cache separately afterwards.

[not-an-aardvark]

What are "hierarchy-local config objects"?

[smably]

Configs parsed from config files in the filesystem hierarchy that apply to files in directories below them (as opposed to, say, the user's personal config from their homedir, or a config file passed as on option on the command line). Totally willing to change the terminology if you can think of a better way to describe these.

[not-an-aardvark]

The config variable is reassigned in several different places, and it's mutated later on, which makes this code hard to follow. Could it be restructured into functions to make it easier to tell what's going on?

[smably]

Fixed.

[eslintbot]

Thanks for the pull request, @smably! I took a look to make sure it's ready for merging and found some changes are needed:

• The commit summary needs to begin with a tag (such as Fix: or Update:). Please check out our guide for how to properly format your commit summary and update it on this pull request.

Can you please update the pull request to address these?

(More information can be found in our pull request guide.)

[smably]

Thanks for the comments, @ilyavolodin and @not-an-aardvark! It's been a busy week, and I haven't had a chance to address all of the issues you identified, but I've pushed my first round of fixes and responded to some of the comments. Just wanted to confirm that I haven't forgotten about this, and further fixes are coming.

[eslintbot]

Thanks for the pull request, @smably! I took a look to make sure it's ready for merging and found some changes are needed:

• The commit summary needs to begin with a tag (such as Fix: or Update:). Please check out our guide for how to properly format your commit summary and update it on this pull request.

Can you please update the pull request to address these?

(More information can be found in our pull request guide.)

[not-an-aardvark]

@smably I see you've pushed some changes since this was last reviewed. Do you think this is ready to be reviewed again? (There's no rush -- I just want to make sure we're not accidentally deadlocked if you were waiting for a review and I was waiting for you to finish.)

[smably]

Hey @not-an-aardvark, thanks for the nudge. I fixed most of the issues but tests seems to be failing on Windows (I'm guess something path-related) and I've been procrastinating figuring out the issue because I don't have a Windows machine available. I'll see if I can find some time in the next week to do some debugging and update the thread with the status of this work.

[not-an-aardvark]

It looks like the issue is that you're using Array.prototype.includes, which only exists on Node 6+, but the build runs on Node 4+. Switching to indexOf should hopefully fix the problem.

[not-an-aardvark]

Waiting another day to merge this in case anyone else on @eslint/eslint-team wants to review it.

[smably]

🎉 Thank you for the guidance and very helpful review comments! This is my first open source contribution (of code, at least) and at first I was a little worried that I'd bitten off more than I could chew. But I think it came together quite nicely.

[ilyavolodin]

Awesome job @smably This is a very high profile feature that users have been asking for a while, so great job at taking on it!

[mischah]

This is great. @smably Thanks for investing your time into that ❤️

[moeriki]

I noticed I couldn't use extends: [] in overrides: [].

Eg.

{
  overrides: [{
    files: ['test/**'],
    extends: ['my-config/mocha']
  }],
}

It that an intentional limitation?

[platinumazure]

Ping @smably @not-an-aardvark, see this comment.

If we're intentionally not supporting extends in override configs, I think we might want to see if we can enhance that (as a separate issue). My suggested approach would be to prepend the override's extends array to the parent config's extends array and removing duplicate entries that occur later in the resulting array, but there might be a better way to do that.

[smably]

@platinumazure Yup, intentionally unsupported (and called out in the docs). It wasn't in the PR I inherited, and I didn't want to add any more scope to what was already a big change. I agree that adding support for this would make a worthy follow-up ticket, and I would be happy to work on the implementation if nobody else has already started.

[platinumazure]

@smably Awesome, thanks for clarifying.

I'll create a new issue.

[CrabDude]

I am beyond impressed y'all saw this through to fruition, and honestly a little shocked. Awesome.

❤️ ❤️ @smably @not-an-aardvark @ilyavolodin

[CrabDude]

Does anyone have final performance numbers on a codebase of significant size?

[kaicataldo]

I just used this feature on a large project at work - really great stuff ❤️

[smably]

@CrabDude Thanks for getting this going and figuring out the logic! You definitely deserve a lot of the credit for this feature... no way I would have been able to get all that caching logic working on my own. ❤️