Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat!: Look Up Config Files From Linted File #120
Changes from 4 commits
6949683
e0a95e8
51a75f0
a6afa8a
d94deba
89d41a2
e9637e1
b0d51dd
0806d56
3865ab6
6e0f3fa
2dcef73
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
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 bothfindConfigFileForFile()
andloadConfigArrayForFile()
return a promise, so it's not clear to me how the config loader will be used. Can you clarify?There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That should work if the call to
getOrFindUsedDeprecatedRules()
is moved directly intoprocessLintReport()
out of theget()
, becauseprocessLintReport()
is called by async functions only.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:eslint .
from the project root. Does it error saying that there's no config file?eslint src
from the project root. Does it loadsrc/eslint.config.js
and lint files insrc
?There was a problem hiding this comment.
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 .
andeslint 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
.There was a problem hiding this comment.
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?There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 aneslint.config.js
file published?There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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. Bothnpx eslint . --flag unstable_config_lookup_from_file
andnpx eslint src --flag unstable_config_lookup_from_file
result inESLint 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 usesrc/eslint.config.js
.There was a problem hiding this comment.
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 asrc/eslint.config.js
file but no rooteslint.config.js
file.Running
eslint .
from the root fails this way: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: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. Runningcd src && npx eslint . --flag unstable_config_lookup_from_file --debug
works well.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 correspondingESLint
constructor optionoverrideConfigFile
can be used to override the default config lookup with an arbitrary file. It's not obvious how these options will behave whenunstable_config_lookup_from_file
is set. I can imagine three possibilities:-c
in place of the per-file configs that the new config loader would otherwise look up.-c
with the per-file configs loaded by the new config loader.-c
flag. In this case we should also ignore/disallow the constructor optionoverrideConfigFile
when it's nottrue
.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.:
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, with a flag, as described in the RFC. 😄
There was a problem hiding this comment.
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 runningeslint
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.
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.