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

feat: Flat config extends #126

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

feat: Flat config extends #126

wants to merge 6 commits into from

Conversation

nzakas
Copy link
Member

@nzakas nzakas commented Nov 14, 2024

Summary

This RFC proposes adding an extends key to flat config in order to make it easier to mix and match other configs.

Related Issues

eslint/eslint#19116

@nzakas nzakas added the Initial Commenting This RFC is in the initial feedback stage label Nov 14, 2024
recommended: {
plugins: { "#": null },
rules: {
"#/no-duplicate-keys": "error",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't using this rewrite mechanism then disallow people to use the rules directly from plugin config objects? Today, it's not uncommon to just spread a plugin's config's rules into a config. In this model, if they did that, the rules wouldn't have the token replaced, right? If that's the case, it seems like taking that usage pattern away would be a big side effect.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The intent isn't to take that usage pattern away, but rather to have another option.

Also, people are accessing rules directly right now because there isn't another way to easily extend configs. I'm not sure how necessary that pattern is in this proposal.

@michaelfaith
Copy link

michaelfaith commented Nov 14, 2024

I think everything up until extending named configs is a solid enhancement and would address the pain points you highlight. The named configs piece, though, feels a bit like a step on the road to rc-configs part 2. There's a lot of "magic" baked into that, and it shifts more away from the pure JavaScript spirit that the Flat config was based on. It also doesn't seem to move the needle that much more than passing the configs into extends as regular objects / arrays.

I do really like the fact that you can pass objects or arrays into extends and users don't need to know the underlying implementation anymore.

];
```

Here, the `files` keys will be combined and the `ignores` key will be inherited, resulting in a final config that looks like this:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that files should be combined -- if I write

{
  files: ["**/*.ts"],
  extends: [someConfig],
}

My expectation would definitely be that I'm overriding the files, eg:

{
  ...someConfig,
  files: ["**/*.ts"],
}

Merging just seems odd, IMO.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth noting that overriding is the behaviour we went with for typescript-eslint's extends property and so far we haven't had anyone report issues asking for a merge instead (it has been live for ~9 months now)

Copy link
Member

@aladdin-add aladdin-add Nov 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how to merge:

{ files: ["src/**/*"], extends: [{files: ["**/*.js"]}], }
  • option A: files: ["src/**/*"]
  • option B: files: ["**/*.js"]
  • option C: files: ["src/**/*", "**/*.js"]

IMHO, all these are inappropriate. most likely it's expected: files: ["src/**/*.js"]

Copy link

@bradzacher bradzacher Nov 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO Option A is the correct result. It's the least magical and most straightforward answer, IMO.

Option C is what this RFC currently proposes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a typical use case for shareable configs specifying files is to provide different configs for different file extensions, while a typical use case for end-user configs specifying files along with extends would be to restrict the sharable config to certain directories only, so what would make the most sense, I think, is resulting config that matches the intersection of files, i.e., files that match both user-specified files and sharable config's files. In @aladdin-add's example, that would be files that match src/**/* AND **/*.js, which is indeed files: ["src/**/*.js"] (though I'm not sure if we would be able to implement this kind of calculations, but rather introduce another mechanism for specifying intersections in the resulting config).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link

@bradzacher bradzacher Nov 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My 2c as a user of eslint is that:

  1. I want the least surprising behaviour, and
  2. I want to be able to override that behaviour

If you do something magic internally like intersecting the globs to create new globs then:

  • that violates (1) because I wouldn't have expected such magic -- similar magic isn't applied elsewhere in flat configs AFAIK - it's all pretty "non-magical" in its behaviour.
  • that violates (2) because I cannot specify an override manually and I would have to opt-out of extends to override the behaviour.

If you merge the files arrays together then:

  • for me at least that violates (1) because I expect that that my array overrides the config array
    • I expect this works same as how my rule config array rule: ["error", "option2"] overrides the config's rule config array rule: ["warn", "option1"]
  • that violates (2) because I cannot specify an override manually and I would have to opt-out of extends to override the behaviour.

What I would reiterate is that typescript-eslint has used the override behaviour for the last 9 months and nobody has complained yet. Which is decent signal, IMO, that people expect overrides.


Note that the intersection or array merge behaviours would be trivial to implement on the user side as an opt-in. For example one might consider a util like intersect(...globs: Array<string | FlatConfig | Array<FlatConfig>>): string[] which would let me do some magic glob merging. Eg

{
  files: intersect("src/**/*", plugin.recommended),
  extends: [plugin.recommended],
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was surprised by typescript-eslint overriding extended files patterns. It was not what I wanted or expected.

My 2 cents, reiterating my comment here: eslint/eslint#19116 (comment)

If an extended config specifies files, and I also specify files, I expect both patterns to apply.

It's easy enough to understand: the extended config filters the list of input files, then my config filters it again. This seems useful.

It's easy enough to implement: instead of glob merging, just use an array-of-arrays format that requires input files to match any pattern in every array.

It's also easy enough to provide an escape hatch like files: () => ["src/**/*"] when you don't want the merging behaviour.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do actually have an intersection syntax already supported in flat config (see eslint/eslint#18966), which we were planning on removing in v10 because we weren't using it. We can certainly leverage that for this proposal as I find the arguments of @mdjermanovic and @aaronadamsCA compelling.

@bradzacher I did take a look at what you were doing with extends and I don't believe that's the correct behavior.


#### Extending Arrays

Arrays can also be used in `extends` (to eliminate the guesswork of what type a config is). When evaluating `extends`, ESLint internally calls `.flat()` on the array and then processes the config objects as discussed in the previous example. Consider the following:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ESLint internally calls .flat() on the array

Is the plan just flat() or flat(depth)?
It might be worth doing flat(Infinity)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call. 👍


The extended objects are evaluated in the same order and result in the same final config.

#### Extending Named Configs

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd just like to reiterate my comment from the original issue in that I strongly believe that supporting this is a bad idea.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noted.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think supporting nested arrays is good, as currently most recommended configs are non-arrays and if they would have to become arrays then that would become a breaking change unless it can be consumed in the same way as a non-array

1. It encourages plugin authors to use the `configs` key on their plugin in order to allow this usage.
1. It allows ESLint to modify configs before they are used (see below).

#### Reassignable Plugin Configs

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be moved to a separate RFC?
seems like it's largely unrelated to adding extends.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. We like to keep related ideas in the same proposal because it makes it easier to evaluate the whole picture. Part of why we ended up in the mess we had with eslintrc was each proposal was being considered separately rather than seeing how it fit into the whole.

If consensus is that people don't like this, we can always remove it later.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @bradzacher, this should be two different RFC:s


Here, the user has assigned the namespace `j` to the JSON plugin. When ESLint loads this config, it sees `extends` with a value of `"j/recommended"`. It looks up the plugin referenced by `j` and then the config named `"recommended"`. It then sees that there's a plugin entry for `"#"` and replaces that with an entry for `j` that points to the JSON plugin. The next step is to look through the config for all the `#` references and replace that with `j` so the references are correct.

### Implementation Details

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's worth referencing the implementation that typescript-eslint already has that is used in the ecosystem (first released in February 2024)

https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/typescript-eslint/src/config-helper.ts

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The docs for that function: https://typescript-eslint.io/packages/typescript-eslint#flat-config-extends

Not really functionally equivalent in all parts?

];
```

Here, `js/recommended` refers to the plugin defined as `js`. Internally, ESLint will look up the plugin with the name `js`, looks at the `configs` key, and retrieve the `recommended` key as a replacement for the string `"js/recommended"`.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would discourage this.

The fact that exported configs needs to reference the very plugin object that they are a property of (configs in js.configs needs to reference js in their plugins.js) makes creation of them quite convoluted and is something that should be moved away from rather than encouraged.

Since all plugin definitions that share the same name needs to be === to each other that means that the plugin configs needs to reference the very plugin object that exposes the recommended configs.

This has led to bugs like: https://github.com/eslint/eslint/blob/0583c87c720afe0b9aef5367b1a0a77923eefe9d/lib/config/flat-config-schema.js#L373-L375

And required workarounds like: neostandard/neostandard@26b472b

Right now plugins typically do:

const base = {
    meta: {
        name: pkg.name,
        version: pkg.version,
    },
    rules: {
        "callback-return": require("./rules/callback-return"),
   },
};
 
base.configs = {
  "flat/recommended": { plugins: { n: base }, ...recommendedConfig.flat },
};

module.exports = base;

I would prefer if one would do eg:

const plugin = {
    meta: {
        name: pkg.name,
        version: pkg.version,
    },
    rules: {
        "callback-return": require("./rules/callback-return"),
   },
};
 
const configs = {
  "flat/recommended": { plugins: { n: plugin }, ...recommendedConfig.flat },
};

module.exports = { plugin, configs };

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand what you're saying. This proposal would solve the problem you're highlighting here.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nzakas This proposal contains many proposals, one of which aims to solve this by adding magic that modifies the config. That part of the proposal feels like a suggestion that should be made separately as a follow up to this RFC and is needlessly complicated compared to what I highlight / suggest here.


Here, we are hardcoding the namespace `json` even though that might not be the namespace that the user assigns to this plugin. This is something we can now address with the use of `extends` because we have the ability to alter the config before inserting it.

Instead of using a hardcoded plugin namespace, plugins can instead use `#` to indicate that they'd like to have the plugin itself included and referenced using the namespace the user assigned. For example, we could rewrite the JSON plugin like this:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or one could define it as:

const plugin = {
  // ...
};

const configs = {
        recommended: (pluginAlias = 'js') => {
            plugins: { pluginAlias: plugin },
            rules: {
                `${pluginAlias}/no-duplicate-keys`: "error",
            },
        },
};

module.exports = { plugin, configs };

And use it like:

js.configs.recommended('foo');

Simpler and less magic. No need for ESLint to modify the config, the config would modify itself.

One could even have "j/recommended" result in js.configs.recommended('j'); if one strongly wants to support the "j/recommended" shortcut.


Generating configs from a function is a pattern that we eg. use in neostandard:

import neostandard from 'neostandard'

export default neostandard({
  noStyle: true,
  ts: true,
});

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exporting a function then muddies the water of a config is, forcing people to understand not just whether something is an object or array (a problem we trying to remedy with this proposal), but then they'd also have to know whether something was a function.

As stated in this RFC Motivation section, right now, end users needing to configure things differently based on how plugins export config is a big problem and source of confusion. I'm trying to remove the necessity for end users to think about this.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would split the two goals of this RFC into one RFC each. The case for extends is more proven and clear to me than the need for and even more so the solution for discoverability of shared configs.


Here, the user has assigned the namespace `j` to the JSON plugin. When ESLint loads this config, it sees `extends` with a value of `"j/recommended"`. It looks up the plugin referenced by `j` and then the config named `"recommended"`. It then sees that there's a plugin entry for `"#"` and replaces that with an entry for `j` that points to the JSON plugin. The next step is to look through the config for all the `#` references and replace that with `j` so the references are correct.

### Implementation Details

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The docs for that function: https://typescript-eslint.io/packages/typescript-eslint#flat-config-extends

Not really functionally equivalent in all parts?

Comment on lines +398 to +400
1. Enable nested arrays in `FlatConfigArray`
1. Create a new `ConfigExtender` class that encapsulates the functionality for extending configs.
1. Update `FlatConfigArray` to use `ConfigExtender` inside of the `normalize()` and `normalizeSync()` methods.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since FlatConfigArray is not something that's exported (see discussion in eslint/eslint#18619) this would not be something that the config-inspector can make use of but something that it would need to re-implement, which would be a lot of extra work and a risk of divergence / differences in implementation.

I would strongly suggest that the non-exporting of FlatConfigArray is reconsidered if all this is added there.

An option could be that the ConfigExtender is added as part of @eslint/config-array instead and that ConfigArray rather than FlatConfigArray is updated to use it in normalize(), and that it behaves like nested arrays: Its something one opts into / out of for a config array

Thoughts @antfu?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, the fact I love the flat config format also makes config-inspector possible is that it's very transparent, where you can import the config file as a plain js module without eslint to get all the information. What I understand is that we now want to move to a bit of the middle ground to have ESLint interop the config a bit to provide better DX, which I would be love to see.

In that regard, in order to keep the inspector working (where we still present the final resolved flat configs to users), I agree with @voxpelli that we do need ESLint to expose those API to handle the config resolutions to the final config array.

Copy link
Member Author

@nzakas nzakas Nov 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's good feedback re: FlatConfigArray and a good argument for adding this functionality into @eslint/config-array directly. I'll take a look at what this would look like and update the RFC once I've figured it out.

I'm still hesitant to export FlatConfigArray itself because we are still making changes and I don't want to be tied to that API by exposing it.

Comment on lines +494 to +498
### Why is this functionality added to `FlatConfigArray` instead of `@eslint/config-array`

In order to support named configs, we need the concept of a plugin. The generic `ConfigArray` class has no concept of plugins, which means the functionality needs to live in `FlatConfigArray` in some way. There may be an argument for supporting `extends` with just objects and arrays in `ConfigArray`, with `FlatConfigArray` overriding that to support named configs, but that would increase the complexity of implementation.

If we end up not supporting named configs, then we can revisit this decision.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I mentioned in another comment, this makes it harder for eg. config-inspector to mimic the same logic since FlatConfigArrayis currently only available internally in ESLint (see discussion in eslint/eslint#18619)

// intersected files and original ignores
{
name: "myconfig > config1",
files: [["**/src/*.js", "**/*.cjs.js"]],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would be files in the final config when files in the user config and extended config have multiple items? For example:

{
    files: ["src/**", "lib/**"],
    extends: [{ files: ["**/*.js", "**/*.mjs"] }]
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In order to get the intersection correct, it would look like this:

{
    files: [["src/**", "**/*.js"], ["lib/**", "**/*.js"], ["src/**", "**/*.mjs"], ["lib/**", "**/*.mjs"]]
}


Design Summary:

1. Allow arrays in config arrays (in addition to objects)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also support Promise in the configs array? Currently, ESLint support returning a single promise as default exports. If any of the shared config returns a promise, users need to either use TLA or wrapper everything with Promise.all, like:

export default Promise.all([
    js.configs.recommended,
    ...tailwind.configs["flat/recommended"],
    ...reactPlugin.configs.flat.recommended,
    somePromise,
]).flat()

which:

  1. It's raise the bar to know how for users
  2. Verbose to write
  3. Need to undo the flatten on the user side again,

The promise can also return either an Object or an array, so I see the type might need to be:

Array<ConfigItem | Array<ConfigItem> | Promise<ConfigItem | Array<ConfigItem>>>

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's beyond the scope of this RFC. If you'd like to suggest that, please open a separate issue as a starting point.

Copy link

@voxpelli voxpelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would split this RFC into two then.

One which addresses the difficulty to extend another config and introduces the extends and another one which addresses the discoverability of exported configs.

I feel that the case for extends is quite solid. I wouldn’t say the same about the discoverability of exported configs.


Here, we are hardcoding the namespace `json` even though that might not be the namespace that the user assigns to this plugin. This is something we can now address with the use of `extends` because we have the ability to alter the config before inserting it.

Instead of using a hardcoded plugin namespace, plugins can instead use `#` to indicate that they'd like to have the plugin itself included and referenced using the namespace the user assigned. For example, we could rewrite the JSON plugin like this:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would split the two goals of this RFC into one RFC each. The case for extends is more proven and clear to me than the need for and even more so the solution for discoverability of shared configs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Initial Commenting This RFC is in the initial feedback stage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants