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

Glob override configs do not support "extends" #8813

Closed
platinumazure opened this issue Jun 26, 2017 · 43 comments
Closed

Glob override configs do not support "extends" #8813

platinumazure opened this issue Jun 26, 2017 · 43 comments

Comments

@platinumazure
Copy link
Member

@platinumazure platinumazure commented Jun 26, 2017

What version are you using?

4.1.1

What did you do?

Tried to use a glob override config with an "extends" property

What happened?

Received an error.

What did you expect to happen?

No error-- glob override config "extends" should be intelligently merged with the main config's "extends".

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.

@lo1tuma
Copy link
Member

@lo1tuma lo1tuma commented Jun 27, 2017

FWIW that was the intended behavior and is even documented. I think the reason behind this was to prevent nested overrides because shared configs could also specify glob overrides.

@not-an-aardvark
Copy link
Member

@not-an-aardvark not-an-aardvark commented Jun 27, 2017

Also see #8081 (comment)

I guess the reason behind this was to prevent nested overrides

That's a good point -- we should figure out how we would handle this. Should we just ignore nested overrides? Nested overrides are a fatal error within a single config, but it might be annoying to have that be the case with an extended config too (since that would make it difficult to reuse a config as a shareable config).

@smably
Copy link
Contributor

@smably smably commented Jun 27, 2017

I haven't tested this yet, but maybe someone else knows: what's the current behaviour for overrides in a shareable config? Normally the patterns are resolved to the location of the config file. Not sure what happens if you extend a config with overrides -- would they be resolved relative to the location of the extended config, or the config doing the extending?

I wonder whether overrides should just be omitted from extended configs by default.

@lo1tuma
Copy link
Member

@lo1tuma lo1tuma commented Jun 27, 2017

I think it could also be quite handy to have glob overrides in shareable configs. You could for example create a shareable config which provides patterns for an opinionated project structure.
Then all I have to do in each project is to extend from a single shareable config which would then specify test specific rules for a pattern that matches test files, react specific rules for jsx files and so on...

@not-an-aardvark
Copy link
Member

@not-an-aardvark not-an-aardvark commented Jun 27, 2017

At the moment I think they're resolved from the location of the config file in node_modules/. I'm not sure whether this is intended behavior.

@platinumazure
Copy link
Member Author

@platinumazure platinumazure commented Jun 27, 2017

I think to understand how globs should work for glob overrides, it helps to distinguish between a direct config file and other config files.

For this comment, a direct config file is a config file directly encountered by ESLint (either via ancestor directory traversal, baseConfig in CLIEngine, or -c in CLI and its CLIEngine equivalent) and with all extends resolved. Note that shareable configs and plugin-exported configs are generally not resolved config files.

I think glob patterns should always be applied relative to the direct config file. Between that implementation approach and some common sense on the part of configuration developers, things should "just work" in a sensible way.

Example (one config)

Suppose there is a config file in a project: .eslintrc.json. It extends from plugin:foo/default configuration which uses glob override to add extra rules for ["**/*.spec.js"] files. .eslintrc.json also uses glob override to add extra rules to ["foo/**/*.js"].

Then file foo/bar.spec.js would have the following configs applied (highest to lowest precedence):

  • .eslintrc.json's folder-based override (globs applied from parent of foo directory)
  • Rest of .eslintrc.json
  • plugin:foo/default's filename-based override (globs applied from parent of foo directory)
  • Rest of plugin:foo/default

Note that all globs are applied from the location of .eslintrc.json because that is the direct config file location.

Example (multiple configs in project)

Suppose there are two configs in a project: .eslintrc.json and foo/.eslintrc.json. The root config extends from plugin:foo/default. Both .eslintrc.json and plugin:foo/default have the same overrides as the previous example. foo/.eslintrc.json just has a few extra config settings.

Then file foo/bar.spec.js would have the following configs applied (highest to lowest precedence):

  • foo/.eslintrc.json, with all its extends. Any glob patterns would apply from the foo directory.
  • .eslintrc.json's folder-based override (globs applied from parent of foo directory)
  • Rest of .eslintrc.json
  • plugin:foo/default's filename-based override (globs applied from parent of foo directory)
  • Rest of plugin:foo/default

Opinionated folder structure configs?

My approach would allow for opinionated folder structure configs per this comment, as long as projects know to extend from the shareable config or plugin-exported config at their root directory. For example, a configuration for Sails.js could assume a particular directory structure and add linting rules for certain directories.

Ambiguity

The only ambiguity I can think of is using extends with a path. To be honest, I think I would still prefer that glob overrides from the extended configuration are resolved relative to the direct config.

Drawbacks?

It could get confusing if the same shareable config or plugin-exported config is extended from config files in multiple directories in a project and the config in question uses any globs more specific than **/(some pattern). We would probably need to augment --debug to make sure it is clear how glob patterns are being applied (i.e., which directory they are being resolved with).

Am I missing anything?

@smably
Copy link
Contributor

@smably smably commented Jun 27, 2017

I agree that it would be most useful if overrides resolved relative to the direct config file.

I think we're talking about a few very similar questions in this thread and I want to make sure that they're all called out explicitly:

  1. The original issue was about including extends keys in overrides blocks: should you be able to extend a shareable config but only for certain file globs?
  2. A secondary question is whether a top-level config should be able to extend a shareable config that contains overrides. If so, how should the glob patterns in the shareable config be resolved?
  3. The overlap between 1 and 2: what happens if you extend a config with an overrides block from within an overrides block in another config?

For question 3, I think the ideal case is to allow nested overrides blocks, and resolve all the globs (including ones from shareable configs) from the location of the direct config file (as described by @platinumazure).

So, for example, if plugin:bar/default has overrides for **/*.spec.js and the root .eslintrc.js has overrides for bar/**/*.js that extend plugin:bar/default, then only files matching bar/**/*.spec.js would pick up the overrides from plugin:bar/default (i.e., the intersection of glob pattern from the overrides in each extended config up the chain).

Not sure about the performance implications of allowing nested overrides, though...

@kokarn
Copy link
Contributor

@kokarn kokarn commented Jun 30, 2017

Here's a use-case that I hope is somewhat straightforward on should be common enough.

I have my own shareable config that I use across projects.
It includes 3 different configs. Browser, Node.JS & React.

A common setup for me is to add the only project-specific things to package.json, such as this.

 "eslintConfig": {
  "extends": "kokarn/react",
  "parserOptions": {
    "ecmaFeatures": {
      "experimentalObjectRestSpread": true
    }
  },
  "rules": {
    "no-console": [
      "off"
    ]
  }
}

For me an override that let's me add other files from my project, in this case a node server and stuff like that, would be awesome.

"overrides": [
  {
    "files": [ "*.js" ],
    "extends": "kokarn/nodejs"
  }
]

However, that would mean that those files shouldn't get the base ruleset from kokarn/react but only the rules in kokarn/nodejs.

If this would be possible, it would be amazingly useful.

@yenbekbay
Copy link

@yenbekbay yenbekbay commented Aug 5, 2017

If you simply want to apply a shareable config to certain files, you can do something like this:

// .eslintrc.js
module.exports = {
  ...
  overrides: [
    Object.assign(
      {
        files: ['**/__tests__/*-test.js', '**/__mocks__/*.js'],
      },
      require('eslint-config-example/jest')
    ),
  ],
};
@kokarn
Copy link
Contributor

@kokarn kokarn commented Aug 5, 2017

@travi
Copy link

@travi travi commented Aug 15, 2017

i hate asking without having time to help move this forward, but i'm curious about the status of this.

i'm really excited to use glob configs so that i can start co-locating test files with source files, but since i use a sharable config, the lack of extends support prevents me from leveraging the glob capabilities.

is this being actively worked on? while i won't be much help contributing an implementation right now, i'd be more than happy to provide specifics about my use cases, if that is helpful.

@smably
Copy link
Contributor

@smably smably commented Aug 15, 2017

Not being worked on as far as I know. I may be able to have another look at this in about a month when I have some free time, but definitely don't depend on it.

@not-an-aardvark
Copy link
Member

@not-an-aardvark not-an-aardvark commented Aug 16, 2017

Adding this to the TSC agenda so we can officially accept the proposal. I want to make sure the proposal is stable before people start implementing it, so that we don't have to throw out peoples' work if the proposal changes after more discussion.

TSC Summary: This issue proposes adding support for using overrides and extends together. From #8813 (comment), the proposal is that globs in extended config files should be resolved from the location of the "direct config" (i.e. the last config that was resolved from the filesystem rather than an extends chain).
TSC Question: Should we accept this proposal (at least tentatively, pending an implementation)?

@not-an-aardvark
Copy link
Member

@not-an-aardvark not-an-aardvark commented Aug 18, 2017

This was brought up in today's TSC meeting, but we decided to postpone the discussion until the next meeting to give people more of a chance to read through the proposal.

@travi
Copy link

@travi travi commented Aug 18, 2017

@not-an-aardvark thank you for the updates and for helping to get this moving forward a bit more officially.

seeing the notes from the meeting, i thought i could provide a lit bit of detail of the approach i've had in mind in hopes that it could be helpful. as was highlighted in the notes, i see supporting extends and overrides as two potentially separate enhancements.

my initial goal is to be able to co-locate tests (*-test.js in my case) with source (*.js) files, but apply different rules to those two types of files. currently is separate these files into test/ and src/ directories in order to extend "additional" rule files from my shareable config to the .eslintrc in each directory. simply supporting extends under overrides would enable me to define globs for the naming patterns of the two types of files in an .eslintrc in the root of the project that reference those same "additional" rule files from my shareable config.

while supporting extends is the biggest value add for me, supporting nested overrideds, like in a shareable config, does provide significant value as well. the main value of this for me is simply to allow me to capture the convention of rules applied to test or src files in the default config within my shareable config rather than duplicating the globs under each project that uses it. then the .esintrc in the root of each project could simply extend the default config. in my opinion, adding support for nested overrides could certainly be added as a follow-up after allowing extends under overrides.

i imagine this is a scenario your team has already considered, but hopefully a somewhat concrete example helps the team consider the value of each piece a bit more clearly.

not being familiar with your meeting schedule, it looks like there are typically meetings 1 to 2 meetings per month. could you point me to details about when the next meeting might be?

@not-an-aardvark
Copy link
Member

@not-an-aardvark not-an-aardvark commented Aug 18, 2017

Thanks for the info!

not being familiar with your meeting schedule, it looks like there are typically meetings 1 to 2 meetings per month. could you point me to details about when the next meeting might be?

There are meetings scheduled every two weeks, so the next meeting will be August 31st.

@btmills
Copy link
Member

@btmills btmills commented Aug 31, 2017

In today's meeting, the TSC definitely wanted to support extends inside overrides as it's the bigger win for less effort, so I'm marking this as accepted for that portion. We then want to wait a little while once that's released and then discuss overrides inside of extends if it's still something people want.

@btmills
Copy link
Member

@btmills btmills commented Aug 31, 2017

Also @travi thank you for the detailed comment - it really helped us clarify what we were discussing and what we want to do!

@travi
Copy link

@travi travi commented Sep 1, 2017

thank you for the update. i'm glad my comment was helpful. as i mentioned, i do think both would be valuable, but the part that has been accepted is certainly the biggest value to start with. glad to see this continue forward.

@chrisgarber
Copy link

@chrisgarber chrisgarber commented Mar 7, 2019

For the newer configurers among us, it should be noted that you'll also need to use the --ext .js,.ts,.tsx flag to lint typescript files with the config above. This isn't actually configurable from the config file, but changing that can be tracked here: #10828

@myuseringithub
Copy link

@myuseringithub myuseringithub commented Mar 14, 2019

@chrisgarber Isn't files option suppose to handle this use case through using Globs pathname matching ?

module.exports = {
    "overrides": [ // Different options based on file extensions.
        {
            parser: "babel-eslint",
            "files": ["**.js"]
        },
        {
            parser: "@typescript-eslint/parser",
            "files": ["**.ts"],
        }, 
    ],    
}
@chrisgarber
Copy link

@chrisgarber chrisgarber commented Mar 14, 2019

I couldn't get that to work. As far as I can tell files can only further limit the set of files on which eslint is working. Glob patterns don't expand the initial scope of the linting call, which by default is just JavaScript files. Description of ext flag from the docs

This option allows you to specify which file extensions ESLint will use when searching for JavaScript files in the directories you specify. By default, it uses .js as the only file extension.
Note: --ext is only used when the arguments are directories. If you use glob patterns or file names, then --ext is ignored. For example, eslint lib/* --ext .js will match all files within the lib/ directory, regardless of extension.

So I think if you are calling from the command line like eslint . then by default eslint will only lint JS files; if you search for a glob, like lib/** it will match all files in the lib directory, whatever their extension. Since the closest thing to a default seems to be eslint . I thought it might be worthy of a note.

@nzakas
Copy link
Member

@nzakas nzakas commented Mar 15, 2019

@mysticatea
Copy link
Member

@mysticatea mysticatea commented Mar 16, 2019

For what it's worth there is a proposal that the files adds linting target automatically: eslint/rfcs#13

tommyip added a commit to tommyip/zulip that referenced this issue Mar 25, 2019
ESLint currently forbids using extends in the override block, ie
```
{
    "extends": ["plugin:@typescript-eslint/recommended"]
}
```
so we just have to add them manually for now.

See eslint/eslint#8813 .
tommyip added a commit to tommyip/zulip that referenced this issue Mar 25, 2019
ESLint currently forbids using extends in the override block, ie
```
{
    "extends": ["plugin:@typescript-eslint/recommended"]
}
```
so we just have to add them manually for now.

See eslint/eslint#8813 .
tommyip added a commit to tommyip/zulip that referenced this issue Mar 25, 2019
ESLint currently forbids using extends in the override block, ie
```
{
    "extends": ["plugin:@typescript-eslint/recommended"]
}
```
so we just have to add them manually for now.

See eslint/eslint#8813 .
tommyip added a commit to tommyip/zulip that referenced this issue Mar 26, 2019
ESLint currently forbids using extends in the override block, ie
```
{
    "extends": ["plugin:@typescript-eslint/recommended"]
}
```
so we just have to add them manually for now.

See eslint/eslint#8813 .
mysticatea added a commit that referenced this issue Mar 26, 2019
mysticatea added a commit that referenced this issue Mar 27, 2019
mysticatea added a commit that referenced this issue Mar 27, 2019
timabbott added a commit to zulip/zulip that referenced this issue Apr 13, 2019
ESLint currently forbids using extends in the override block, ie
```
{
    "extends": ["plugin:@typescript-eslint/recommended"]
}
```
so we just have to add them manually for now.

See eslint/eslint#8813 .
jacktigg added a commit to jacktigg/zulip that referenced this issue Apr 22, 2019
ESLint currently forbids using extends in the override block, ie
```
{
    "extends": ["plugin:@typescript-eslint/recommended"]
}
```
so we just have to add them manually for now.

See eslint/eslint#8813 .
mysticatea added a commit that referenced this issue May 10, 2019
vrongmeal added a commit to vrongmeal/zulip that referenced this issue May 17, 2019
ESLint currently forbids using extends in the override block, ie
```
{
    "extends": ["plugin:@typescript-eslint/recommended"]
}
```
so we just have to add them manually for now.

See eslint/eslint#8813 .
mysticatea added a commit that referenced this issue May 24, 2019
Limess pushed a commit to Financial-Times/eslint-config-reliability-engineering that referenced this issue Jul 9, 2019
Charlie Briggs
Now eslint/eslint#8813 is fixed this leads to much less object merging
@aggmoulik
Copy link

@aggmoulik aggmoulik commented Jul 19, 2019

How Can I create an override for some files in my eslint-plugin so that user don't have to use override in his .eslintrc?

@platinumazure
Copy link
Member Author

@platinumazure platinumazure commented Jul 19, 2019

@moulikcipherX Your plugin just needs to export a config which has the necessary overrides in it; then users can extend that config in their own config, without worrying about the details.

If you have any more questions, feel free to stop by our Gitter chat.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.