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!: Look Up Config Files From Linted File #120

Merged
merged 12 commits into from
Aug 7, 2024
134 changes: 134 additions & 0 deletions designs/2024-config-lookup-from-file/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,134 @@
- Repo: eslint/eslint
- Start Date: 2024-06-14
- RFC PR: TBD
- Authors: Nicholas C. Zakas

# Look Up Config Files From Linted File

## Summary

This proposal changes the lookup behavior for flat configuration files to match the behavior of eslintrc configuration lookup, in that the search begins from the file being linted rather than from the current working directory.

## Motivation

One of the significant changes from the eslintrc configuration system to the flat configuration system was the logic of looking up configuration files. In an effort to simplify configuration lookup, the decision was made to start the search from the current working directory, with the idea that most projects would contain only one configuration file. The intent was to reduce disk access, and in turn, speed up ESLint. While this approach worked well for most projects, shortly after ESLint v9.0.0 was released, we started hearing complaints from monorepo projects that they could not easily configure their repos using this approach.

We first offered that they should put a configuration file in the root of the repo and list out overrides for specific projects, but the feedback was that for large monorepos, this created a burden and prevented them from having everything related to a project inside of the project subdirectory.

Looking up configuration files from the current working directory also proved to be problematic whenever users were using ESLint without the CLI. Notably, IDEs don't necessarily have a directory that would logically map to the current working directory, and that meant needing to calculate something that would make ESLint work.

As a result, it became obvious that the modified configuration lookup strategy in flat config would not work in the long term, and we need to go back to looking up configuration files from the file being linted.

## Detailed Design

This proposal consists of the following changes:

1. Create a new `ConfigLoader` class that manages the lookup and caching of configurations
1. Create a new `LegacyConfigLoader` class with the same interface as `ConfigLoader` to encapsulate the current configuration lookup strategy (look up from the cwd)
1. Use a feature flag to switch between the two modes in the `ESLint` class

**Compatibility:** This proposal requires the flat config system and cannot be used with the eslintrc config system.

### The `ConfigLoader` interface

The following interface will be implemented twice, once as `ConfigLoader` to encapsulate the new behavior and once as `LegacyConfigLoader` to encapsulate the current behavior. This will allow the `ESLint` class to switch between these two modes easily and remove all configuration lookup-related functionality from the `ESLint` class.

```ts
interface ConfigLoader {

/**
* Searches the file system for the right config file to use based on the
* absolute path.
*/
findConfigFileForPath(fileOrDirPath): Promise<string|undefined>;

/**
* An asynchronous call that searches for a config file from the given
* absolute path and returns the config array for that path.
*/
loadConfigArrayForPath(fileOrDirPath): Promise<FlatConfigArray>;

}
```

#### The `findConfigFileForPath()` Method

This method returns the path to the config file for a given file path. When used in `LegacyConfigLoader`, this method would search from the cwd and ignore the argument that was passed in. This replaces the [`findFlatConfigFile()` method](https://github.com/eslint/eslint/blob/455f7fd1662069e9e0f4dc912ecda72962679fbe/lib/eslint/eslint.js#L266-L271) that is currently in `lib/eslint/eslint.js`.

#### The `loadConfigArrayForPath()` Method

This method behaves similarly as the current [`ESLint#calculateConfigForFile()` method](https://github.com/eslint/eslint/blob/455f7fd1662069e9e0f4dc912ecda72962679fbe/lib/eslint/eslint.js#L1165-L1174) except that it returns the `FlatConfigArray` instead of the config for the given file. All of the logic in `ESLint#calculateConfigForFile()` will be moved to the `ConfigLoader` class and the `ESLint` class will call the `ConfigLoader` method to provide this functionality. This requires moving the [`calculateConfigArray()` function](https://github.com/eslint/eslint/blob/455f7fd1662069e9e0f4dc912ecda72962679fbe/lib/eslint/eslint.js#L369) into `ConfigLoader` (as a private method).

In most of the `ESLint` class logic, `FlatConfigArray#getConfig()` will need to be replaced by `ConfigLoader#loadConfigArrayForPath()` to ensure that the file system is always searched to find the correct configuration.

It's necessary to return a `FlatConfigArray` because we [pass a `FlatConfigArray` to `Linter`](https://github.com/eslint/eslint/blob/455f7fd1662069e9e0f4dc912ecda72962679fbe/lib/eslint/eslint.js#L498) and also use it when we [filter out code blocks](https://github.com/eslint/eslint/blob/455f7fd1662069e9e0f4dc912ecda72962679fbe/lib/eslint/eslint.js#L511-L513). Because `Linter` is a synchronous API, we need to maintain the synchronous calls, and the easiest way to do that is to access the `FlatConfigArray` directly.

### Core Changes

In order to make all of this work, we'll need to make the following changes in the core:

1. Create a new `lib/config/config-loader.js` file to contain both `ConfigLoader` and `LegacyConfigLoader`.
1. Move `findFlatConfigFile()`, `loadFlatConfigFile()`, `locateConfigFileToUse()`, and `calculateConfigArray()` from `lib/eslint/eslint.js` to `lib/config/config-loader.js`.
1. Create a new feature flag called `unstable_config_lookup_from_file` (once https://github.com/eslint/eslint/pull/18516 is merged).
1. Update `lib/eslint/eslint.js`:
1. Create a new `ConfigLoader` based on the presence or absence of the flag. This should be used in the `ESLint#lintText()` and `ESLint#lintFiles()` methods.
1. Update `ESLint#calculateConfigForFile()` to use the config loader.
1. Update `ESLint#findConfigFile()` to use the config loader. To preserve backwards compatibility, when called without an argument, this method will start the config file search from the current working directory; it will also accept an argument, which is the path to search from.
1. Update `getOrFindUsedDeprecatedRules()` to use the config loader.
Copy link
Member

Choose a reason for hiding this comment

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

ESLint#getOrFindUsedDeprecatedRules() returns an array synchronously, but both findConfigFileForFile() and loadConfigArrayForFile() return a promise, so it's not clear to me how the config loader will be used. Can you clarify?

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 100% sure at this point, but this is a minor implementation detail that I don't think we should be caught up on. Worst case scenario I just change the function to be async.

Copy link
Member

Choose a reason for hiding this comment

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

Worst case scenario I just change the function to be async.

That should work if the call to getOrFindUsedDeprecatedRules() is moved directly into processLintReport() out of the get(), because processLintReport() is called by async functions only.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, as I said, this is a pretty minor implementation detail that I'm sure has a solution once I get to that point.

1. Update `findFiles()` in `lib/eslint/eslint-helpers.js` to use the config loader. This also requires a change to the file system walking logic because `fswalk` filter functions are synchronous, but we'll need them to be asynchronous to use with the config loader. I plan on using [`humanfs`](https://github.com/humanwhocodes/humanfs/blob/main/docs/directory-operations.md#reading-directory-entries-recursively) (which I wrote).
Copy link
Member

Choose a reason for hiding this comment

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

If there's no config file in the root, but there is a config file in a subdirectory, e.g., src/eslint.config.js, what happens when user runs:

  1. eslint . from the project root. Does it error saying that there's no config file?
  2. eslint src from the project root. Does it load src/eslint.config.js and lint files in src?

Copy link
Member Author

Choose a reason for hiding this comment

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

This works the same way as in eslintrc: A config file is looked up from the file being linted not from the patterns passed on the command line. So eslint . and eslint src work the same for the same files. If there's ./src/foo.js and ./src/eslint.config.js, then when ESLint goes to lint ./src/foo.js it loads ./src/eslint.config.js. Then, in the same run, if there's a ./tests/eslint.config.js, that will be used for ./tests/foo.test.js.

Copy link
Member

Choose a reason for hiding this comment

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

In that case, how does the search determine which directories should be traversed when running eslint . and there is no config file in the root? Will it skip those matched by default ignores?

Copy link
Member Author

Choose a reason for hiding this comment

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

Everything is dependent on the directory that's visited. For each directory, we search up to find a config file and use that to determine whether or not the directory should be skipped. The same is true for files. We always search for the relevant config file.

Copy link
Member

Choose a reason for hiding this comment

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

That would mean we'd search node_modules too, and lint files in it if some packages have an eslint.config.js file published?

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 think we can better answer these questions with a prototype, so I'm going to work on that.

Copy link
Member

Choose a reason for hiding this comment

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

I've tried the above scenario with the issue18385b branch. Both npx eslint . --flag unstable_config_lookup_from_file and npx eslint src --flag unstable_config_lookup_from_file result in ESLint couldn't find an eslint.config.(js|mjs|cjs) file. error.

Furthermore, if I add an empty (export default []) config file in the root, both commands now work but neither seems to use src/eslint.config.js.

Copy link
Member

Choose a reason for hiding this comment

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

I've tried the latest version of the issue18385b branch with a test project that has a src/eslint.config.js file but no root eslint.config.js file.

Running eslint . from the root fails this way:

$ npx eslint . --flag unstable_config_lookup_from_file --debug
  eslint:cli CLI args: [ '.', '--flag', 'unstable_config_lookup_from_file', '--debug' ] +0ms
  eslint:cli Using flat config? true +3ms
  eslint:cli Running on files +4ms
  eslint:cli Checking for inactive flags +1ms
  eslint:eslint Using file patterns: . +0ms
  eslint:eslint Deleting cache file at C:\projects\tmp\tmp\.eslintcache +1ms
  eslint:config-loader Calculating config for directory C:/projects/tmp/tmp/node_modules +0ms
  eslint:config-loader Searching for eslint.config.js +1ms

Oops! Something went wrong! :(

ESLint: 9.5.0

ESLint couldn't find an eslint.config.(js|mjs|cjs) file.

From ESLint v9.0.0, the default configuration file is now eslint.config.js.
If you are using a .eslintrc.* file, please follow the migration guide
to update your configuration file to the new format:

https://eslint.org/docs/latest/use/configure/migration-guide

If you still have problems after following the migration guide, please stop by
https://eslint.org/chat/help to chat with the team.

Does this mean that there must be an eslint.config.js in the root (or ancestry) of the search pattern?

Running eslint src from the root fails in the same way:

$ npx eslint src --flag unstable_config_lookup_from_file --debug
  eslint:cli CLI args: [ 'src', '--flag', 'unstable_config_lookup_from_file', '--debug' ] +0ms
  eslint:cli Using flat config? true +3ms
  eslint:cli Running on files +3ms
  eslint:cli Checking for inactive flags +1ms
  eslint:eslint Using file patterns: src +0ms
  eslint:eslint Deleting cache file at C:\projects\tmp\tmp\.eslintcache +1ms
  eslint:config-loader Calculating config for directory C:/projects/tmp/tmp/node_modules +0ms
  eslint:config-loader Searching for eslint.config.js +0ms

Oops! Something went wrong! :(

ESLint: 9.5.0

ESLint couldn't find an eslint.config.(js|mjs|cjs) file.

From ESLint v9.0.0, the default configuration file is now eslint.config.js.
If you are using a .eslintrc.* file, please follow the migration guide
to update your configuration file to the new format:

https://eslint.org/docs/latest/use/configure/migration-guide

If you still have problems after following the migration guide, please stop by
https://eslint.org/chat/help to chat with the team.

This is, I believe, an unexpected behavior because the argument is src so the absence of config files in other directories shouldn't matter I think. Running cd src && npx eslint . --flag unstable_config_lookup_from_file --debug works well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that's unexpected for sure. You should not need a root eslint.config.js file for this to work.

I'll take a look.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, so I've updated the behavior such that traversing directories does not require a config file but will honor one if it's found. There will be no error thrown when a config file is not found when traversing directories.

There will be an error thrown when attempting to lint a file that doesn't have a config file in its ancestry.

I believe this behavior mimics what we have now.


Copy link
Member

Choose a reason for hiding this comment

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

Currently, the CLI flag -c and the corresponding ESLint constructor option overrideConfigFile can be used to override the default config lookup with an arbitrary file. It's not obvious how these options will behave when unstable_config_lookup_from_file is set. I can imagine three possibilities:

  1. Keep the current behavior and always use the config specified with -c in place of the per-file configs that the new config loader would otherwise look up.
  2. Merge the config specified with -c with the per-file configs loaded by the new config loader.
  3. Ignore/disallow the -c flag. In this case we should also ignore/disallow the constructor option overrideConfigFile when it's not 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.

Good call out. Option 1 is what will happen. I'll add a FAQ.

Copy link
Member

@fasttime fasttime Jun 19, 2024

Choose a reason for hiding this comment

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

Option 1 should also make it possible to opt out of the new config resolution when it becomes the default. If a user for some reason prefers to keep the current behavior, they could do so by specifying a config file explicitly, e.g.:

npx eslint -c eslint.config.js

Copy link
Member Author

Choose a reason for hiding this comment

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

People can also "opt out" by having a eslint.config.js in the root of their project and nowhere else.

### Rollout Plan

#### v9.x

* Add the `unstable_config_lookup_from_file` flag while the feature is in development.
* Once stabilized, retire the `unstable_config_lookup_from_file` flag and create the `v10_config_lookup_from_file` flag, which will let people opt-in to the breaking behavior.

Choose a reason for hiding this comment

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

I think it's unlikely that anyone is running ESLint from a completely different tree than the file they're linting; I would think anyone who got v9 to work likely moved everything into the root config, so searching from a given file will still find that same config.

Is it possible that this change could be made as the default without waiting for a major?

(Maybe someone had worked around this config issue by creating multiple config files and sharing them in every dir, rewriting a shared config to ensure the paths work, but that seems less than likely without helpers to enable that sort of thing reliably.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Is it possible that this change could be made as the default without waiting for a major?

Yes, with a flag, as described in the RFC. 😄

Choose a reason for hiding this comment

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

Yes, that's why I'm commenting on this line! A flag can't be set in a config, so there's no way to force it to be on except to always remember to pass it at the CLI, insert it in scripts, and/or configure editors to pass the flag in. And if I can pass the flag in, I can probably also just confgure tooling to cd before running eslint or similar.

Of course, it can't be a config option, because this option is intended to control config lookup, so it's a chicken and the egg problem.

Hence, hoping that this could just "be changed" without going through the full cycle if nobody in this situation was able to correctly configure things anyway.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is the process we need to go through for any breaking change, which this definitely is (it's breaking a core assumption of how ESLint works). The flags allow people to try it out before it's fully released and know when it's stable enough to rely on.

IIRC, you mentioned that you're using the ESLint class for linting DefinitelyTyped? In which case, you can pass the flags to the constructor to try it out.

Copy link

@jakebailey jakebailey Jun 18, 2024

Choose a reason for hiding this comment

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

Yes, we can pass whatever flags we want in dtslint, but we can't also enforce that for people using their editors on DT, or anyone else in a similar situation. It's also probable that anyone in this situation has not migrated to flat config anyway. It just seems to me like this won't actually break anyone in practice (rather, help people go to v9).

Copy link
Member Author

Choose a reason for hiding this comment

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

@jakebailey Not to sound like a broken record (pun intended), but we have this process for breaking changes for a reason. It's not enough that you think something won't affect people that much. Semver is a contract that we take seriously, and while all of your concerns are valid, they do not change how we implement breaking changes.

As I said in the original issue, I'm not opposed to doing another major version release this year, so you'll just have to be patient.

At this point, the most helpful you can do is read through the RFC, leave your feedback about how it would affect your use case, and once implemented, try it out and leave feedback on the implementation.


#### v10.x

* Remove the `v10_config_lookup_from_file` flag and make this behavior the only available config lookup scheme.
* Remove `LegacyConfigLoader`.

## Documentation

The primary place to update is the section on [configuration file resolution](https://eslint.org/docs/latest/use/configure/configuration-files#configuration-file-resolution).

## Drawbacks

1. **Performance.** There may be a performance impact to this change, as we'll be hitting the file system more frequently and switching from sync to async functions in certain places. This is unavoidable to implement this proposal.
1. **More flat config changes.** With some of the criticisms around flat config, we may get some blowback for making another change while most of the ecosystem hasn't yet converted. Hopefully, though, this change will have minimal impact on most users.

## Backwards Compatibility Analysis

While a breaking change, this proposal will have a minimal impact on most users of flat config. Because linted files typically exist under the current working directory, and the configuration file is either in the current working directory or an ancestor directory, projects that currently use a flat config file will likely not see any difference in the way ESLint works. The same config file will be found during the resolution process.

This is breaking primarily because users might have other config files deeper in the project directory hierarchy that might be found instead of the one found by searching from the cwd.

## Alternatives

* **Lookup strategy CLI/`ESLint` Option.** Instead of migrating towards a new config lookup strategy, we could also let people opt in via a command line argument and associated option to the `ESLint` constructor. Something like `--config-lookup-from file` with the default being `--config-lookup-from cwd`. The downsides of this approach are that it requires people to opt-in (at least until we can switch the default) and we'll be stuck maintaining two different config lookup strategies.
* **Configurable lookup strategy.** We could also allow people to specify a lookup strategy, or even a custom implementation of a lookup strategy, using a configuration file (either in `eslint.config.js` or as a distinct file). This would be more complicated to implement and maintain.

## Open Questions

N/A

## Help Needed

N/A

## Frequently Asked Questions

### What will the behavior be of `-c` when `*_config_lookup_from_file` is enabled?

The same as the current implementation: The `-c` option completely overrides which config file to use and there will be no config lookup performed.

## Related Discussions

* https://github.com/eslint/eslint/discussions/18574
* https://github.com/eslint/eslint/discussions/16960
* https://github.com/eslint/eslint/discussions/18353
* https://github.com/eslint/eslint/discussions/18101
* https://github.com/eslint/eslint/discussions/18456
* https://github.com/eslint/eslint/discussions/17853
* https://github.com/eslint/eslint/discussions/16202