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

Change Request: Make it easier to inherit flat configs from the repo root #18385

Open
1 task done
jakebailey opened this issue Apr 23, 2024 · 8 comments
Open
1 task done
Labels
core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint needs design Important details about this change need to be discussed

Comments

@jakebailey
Copy link

ESLint version

v8.57.0

What problem do you want to solve?

When using a flat config, the CLI only ever loads one config; this is resolved depending on where you invoke it. In a monorepo, you may want to have multiple eslint.config.js files; this is common for non-flat configs, where child .eslintrc.json files merge with their parents.

It's a design choice that flat configs no longer merge with parent directories; this is fine, and it seems reasonable that one could write a config like:

import rootConfig from "../../../eslint.config.js";

export default [
    ...rootConfig,
    // Some more stuff
]

However, this doesn't acutally work!

  • If you run eslint . at the root of the monorepo, eslint only loads the root config. All other configs are ignored, so any of the overrides done by other configs are not reflected.
  • If you run eslint . inside of the nested dir, now the other config is loaded, so it sometimes works, depending on the current working directory.
  • All of the paths inherited from the root array will be relative to the wrong dir entirely, e.g. ignoring scripts/** in the root will also ignore script/** in the local dir, even if the intent was that only the top level dir was ignored. Or, maybe a rule applied to a completely different location and shouldn't be included at all.

The only way to do this is to forego multiple configs entirely. The root eslint.config.js could include the overrides for everything, or invent a custom config loading mechanism, like (in psuedocode):

import { globSync } from "glob";

const allConfigs = globSync("**/eslint.config.json");

export default [
    // ...
    allConfigs.map((f) => {
        // Somehow read in the config, then remap all of its internal `files` globs to match a subdir?
    })
];

But, now you're loading more configs than may be needed for a given file. This is especially a problem for DefinitelyTyped; the 9244 package monorepo currently has 2247 .eslintrc.json files:

$ find . -iname 'package.json' | wc -l
9244
$ find . -iname '.eslintrc.json' | wc -l
2247

Normally, this would make me quite worried because if you run eslint in parallel per package (like DT does), that could mean 9,244 * 2,247 = 2,0771,268 config loads. But, under flat config, each config is a module, so the runtime should theoretically be reusing that module, so the reality would likely be closer to just "load 2000 JSON files once" so long as the config is not invalidated.

That doesn't solve the problem of "what does this config file look like", however; not everything can be represented in JSON anymore with flat config.

What do you think is the correct solution?

ESLint could look for the closest eslint.config.js file. This is similar to old configs, but explicitly without the merging semantics. This would make ESLint working directory invariant; no matter where you run things, you get the same result.

I understand that there is some concern about the performance of this, but I don't actually think it will cost too much; you only need to walk a given dir once and remember which config applied to it. All in all, that's no more accesses to the disk than you already need to actually be able to list the files you're going to lint.

However, this does not fix the other flaw; inheriting another directory's config does not imply any modifications of file globs. If I spread in a parent dir's eslint config, none of the paths will line up, and I have no solution to that.


Another thing I thought of was to have/suggest a layout like:

.
├── eslint.base.config.js - contains the actual shared config
├── eslint.config.js      - imports shared config, all child dir configs
└── packages/
    ├── foo/
    │   └── eslint.config.js  - imports only shared config
    └── bar/
        └── eslint.config.js  - imports only shared config

This would also be cwd-invariant, but is pretty complicated, and also does not solve the problem of re-rooting paths from other configs / removing ones that don't apply to the subdir.

Participation

  • I am willing to submit a pull request for this change.

Additional comments

I have yet to do perf testing on the "load 2247 config files" method or the "base config" idea; I've been really busy and have not had time to work on DT related stuff like this.

@jakebailey jakebailey added core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint labels Apr 23, 2024
@nzakas
Copy link
Member

nzakas commented Apr 25, 2024

Thanks for writing this all up, it's very helpful.

Some followup questions:

  • How do you usually run ESLint inside the repo? Is it one npx eslint . that lints the entire repo? Or are you always linting inside of one package directory at a time?
  • Are you passing individual filenames or directory names?

Anything else about how you're using ESLint would be helpful in thinking this through.

@nzakas nzakas added the needs design Important details about this change need to be discussed label Apr 25, 2024
@jakebailey
Copy link
Author

Answering directly first:

  • How do you usually run ESLint inside the repo? Is it one npx eslint . that lints the entire repo? Or are you always linting inside of one package directory at a time?

We run eslint programmatically via new ESLint, or in the editor with vscode-eslint.

  • Are you passing individual filenames or directory names?

Individual file names, but this is an artifact of DT's old layout where the only source of config were tsconfig.json files.


Getting into more details:

DT is sort of weird; we have a wrapper dtslint which operates more-or-less per package, doing new ESLint and calling it via the API. Then a higher-level tool dtslint-runner runs all modified packages in parallel. A large number of our checks are in ESLint, but not all.

It's extra weird becuase dtslint only runs ESLint on a subset of the files (those that are reachable from a tsconfig.json), so some are unlinted, and some have separate overrides applied on top (as our lint rules can actually use more than one TS version at a time and at the moment only dtslint knows that info).

So, no, we aren't actualy linting all in one go. However, I would like to be able to modify things such that eslint . would "just work" without that preprocessing, moving that logic down. This would make the editor experience more consistent with running dtslint.

That being said, downstream tooling that uses ESLint has to manually create new ESLint instances based on where configs are. For example, vscode-eslint has to figure out which working directories would be needed, then spin up multiple instances of ESLint to handle that case: microsoft/vscode-eslint#1644 (comment)


However, DT is itself very odd in that we can get away with never running npx eslint .; my impression from talking to others is that they have similar issues trying to configure flat ESLint in a monorepo that would benefit from some improvement here.

@jakebailey
Copy link
Author

jakebailey commented Apr 26, 2024

I also forgot to mention again that one of DT's main features is "self service", where a given package can have owners that are allowed to approve PRs to their code, and authors/owners can merge their own code. Moving all config to the top level breaks that model, as any lint rule changes that need to be made will require DT maintainer (so, TS team) approval, as the repo root is considered "infrastructure" and cannot be touched by anyone. So, we need to have configs in the subdirs, somehow.

@nzakas
Copy link
Member

nzakas commented Apr 26, 2024

I think that figuring out how to have configs in the subdirectories is our main goal here. 👍

DT is sort of weird; we have a wrapper dtslint which operates more-or-less per package, doing new ESLint and calling it via the API. Then a higher-level tool dtslint-runner runs all modified packages in parallel. A large number of our checks are in ESLint, but not all.

How is this initiated? Are people already in the package directory and then run npm run lint? Or are they running it from some other location?

Some possible hypothetical approaches going through my head (none of which have been thoroughly thought through):

  1. eslint --config-lookup-from=path/to/package - instead of looking up from the cwd, look up from this other directory regardless of where ESLint is executed.
  2. eslint --config-lookup-mode=file - instead of looking up from the cwd, look up from the directory of each linted file. This is what eslintrc did (and is what we were trying to avoid with flat config due to the amount of work this entails).

(Of course, both flags would need to be represented in the ESLint class as well, but I think of things in terms of the CLI.)

Do either of those seem like they'd solve your use case?

re: VS Code - this continues to be a problem, primarily because we don't maintain that extension and therefore have very little control over how it does what it does. We can request changes, but ultimately have zero control. This is something we need to dig into separately on a more holistic level.

@jakebailey
Copy link
Author

How is this initiated? Are people already in the package directory and then run npm run lint? Or are they running it from some other location?

At the moment, they run pnpm test <their package name> from the repo root. Ideally, they could run pnpm test inside their package and that work, but the package.json monorepo layout is fairly new and we haven't improved that tooling. But this difference doesn't much matter because dtslint invokes ESLint via its API.

Do either of those seem like they'd solve your use case?

Yes and no; right now, we always invoke ESLint ourselves via the API, we can already provide it a cwd, and therefore it would "just work" already so long as we don't have any other eslint configs that aren't at the package root. This turns out to not be true, as there are .eslintrc.json files in nested dirs which would not behave properly with flat configs.

The first listed idea is no different than passing in a cwd. The second is what I was suggesting always be on in my original idea, but doesn't handle the more general problem of inheriting parent configs being a challenge due to paths being rooted in the wrong place.

But, to be clear, I'm not trying to frame this issue in terms of DefinitelyTyped only, but as a more general "flat config is hard in monorepo" issue. The only saving grace for us is that we programmatically invoke ESLint and don't yet support running ESLint on its own, except in the editor, which now works at least in VS Code because it reimplemented the config file search algorithm to know when to invoke ESLint more than once.

So, I still don't have a good idea, because in addition to the cwd thing, making flat configs work nicely in monorepos when inhereting a parent config is challenging, because:

  • On one hand, you want to inherit the parent config, but only what actually applied to the current dir, so you can extend it
  • On the other hand, maybe you're actually inhereting from a shared config in another package which has nothing to do with the monorepo and only contains generic globs.

Not sure how to distinguish those, and I apologize for the lack of clarity. It's just a bit hard to figure out what the "right thing" is, compared to the strictly hierarchical old config system.

re: VS Code - this continues to be a problem, primarily because we don't maintain that extension and therefore have very little control over how it does what it does. We can request changes, but ultimately have zero control. This is something we need to dig into separately on a more holistic level.

I'm not sure I follow this; no matter who implements an editor integration, the fact that eslint packages/foo/index.js and cd packages/foo; eslint index.js differ in behavior makes getting things right a challenge. Before flat config, this distinction didn't matter.

@nzakas
Copy link
Member

nzakas commented Apr 26, 2024

But, to be clear, I'm not trying to frame this issue in terms of DefinitelyTyped only, but as a more general "flat config is hard in monorepo" issue.

See, I'm not sure that's true. So far, aside from DT, when maintainers have contacted us about monorepo setup, they almost always end up finding a way to make it work within the confines of flat config. That's part of the benefit of flat config -- you can define your own loading mechanism right in the config file. That's why we were able to externalize the eslintrc loading mechanism in @eslint/eslintrc.

I can see determining where to start the search as an issue, but as you mentioned, you can always set cwd on the ESLint instance to handle that. As I've been exploring more, I can see how this would useful.

However, I'm unclear on how many monorepos prefer to lint the entire repo, in which case one config file at the root makes a lot of sense, as opposed to monorepos where they prefer to lint just an individual package. That's the first question.

The second question is, for those who prefer to lint inside of individual packages only, do they use the cascade to apply shared configs? And is there any reason that the shared configuration can't be moved to a file that is just imported into the package-level config file?

That said, it would help to have a concrete example of how you're currently using cascade in DT. Can you point to a package and the config files it relies on?

@jakebailey
Copy link
Author

However, I'm unclear on how many monorepos prefer to lint the entire repo, in which case one config file at the root makes a lot of sense, as opposed to monorepos where they prefer to lint just an individual package. That's the first question.

An example I can think of is those using lint-staged or similar to lint only changed files; those tools give path paths, so the easiest method to lint them would be to pass theminto the eslint CLI like eslint packages/foo/index.js packages/bar/index.js. But if they have different configs in each directory, you can't lint like that, you need to know that they have different configs and go into their specific directories and then lint them there.

The other case is people opening subdirs, where the behavior should match if you opened the entire repo.

The second question is, for those who prefer to lint inside of individual packages only, do they use the cascade to apply shared configs? And is there any reason that the shared configuration can't be moved to a file that is just imported into the package-level config file?

I haven't worked in a repo that didn't use cascades. TS itself isn't a monorepo per se, but did use nested cascading configs, which I had to undo for consistent behavior in my WIP flat config PR: microsoft/TypeScript#57684

But nobody else is chiming in here about the problems they're having, so it's not super easy for me to made wider claims.

That said, it would help to have a concrete example of how you're currently using cascade in DT. Can you point to a package and the config files it relies on?

Sure, for DT:

There's a root config, then packages or subdirs within those packages override the settings, but within JSON (statically analyzable using our bot infra without executing code).

I have not yet figured out what we're going to do on DT at all for flat config on the loading front; the fact that the code is exectuable is a security and infra processing gotcha for us (we can't really statically analyze a flat config...), so it's not super clear what to do. It may be the case that we give up on letting anyone write eslint.config.mjs files at all, and just invent or own config (not called .eslintrc.json) and force the repo root eslint.config.mjs file to load everything and eat the perf cost of doing so for packages that aren't referenced. Something like mergeESlintConfigs in #18353 (comment) but not loading real configs.

@nzakas
Copy link
Member

nzakas commented Apr 29, 2024

Thanks, that's helpful. I think there are two things the TSC needs to consider:

  1. Should we always search for a config file from the files passed to ESLint? This would be a breaking change. We could ease into this by creating a CLI flag/ESLint constructor option that lets people opt-in to this behavior in v9 and then switch to make it the default in v10. I'm not opposed to doing a v10 soonish (later this year) to address this. I am currently in favor of this approach, as I think this helps with both the monorepo and IDE use cases. We will lose some of the perf gains we currently have by only searching for a config file once, but this seems like the right thing to do.
  2. Should we crawl up the directory structure merging config files like in eslintrc? If we implement item 1, then this might be best implemented by creating a standalone utility that people could use in their config files if they so choose rather than adding it back into the core. I don't think folks should pay the cost of always doing this search if they aren't going to use.

I have not yet figured out what we're going to do on DT at all for flat config on the loading front; the fact that the code is exectuable is a security and infra processing gotcha for us (we can't really statically analyze a flat config...), so it's not super clear what to do.

It seems like in order to work for your setup, ESLint would need to implement both items 1 and 2 in the core and also allow JSON config files, that is undoing a lot of the simplicity we got with flat config. So yeah, you may need to come up with a custom solution for that.

@eslint/eslint-tsc would like some other opinions here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint needs design Important details about this change need to be discussed
Projects
Status: Evaluating
Development

No branches or pull requests

2 participants