Fix: Pass internal config paths in FileEnumerator default (fixes #13789) #13792
Conversation
When `CLIEngine` instantiates `FileEnumerator`, it explicitly passes its own `CascadingConfigArrayFactory` instance, which now includes values for `builtInRules`, `loadRules`, `eslintRecommendedPath`, and `eslintAllPath`. This is the only place that ESLint core instantiates `FileEnumerator`, so core does not rely on the constructor's default value for `configArrayFactory`. After `CascadingConfigArrayFactory` was extracted into `@eslint/eslintrc`, it no longer assumed values for `eslintRecommendedPath` and `eslintAllPath`, which `CLIEngine` now provides. If those values are not passed, as is the case with the default `CascadingConfigArrayFactory` for `configArrayFactory`, file enumeration will encounter the exception that was reported in #13789. From the perspective of ESLint core, the default value for `configArrayFactory` is dead code. However, even though `FileEnumerator` is an undocumented API, it's called by `eslint-plugin-import`'s `no-unused-modules` rule, which hits the exception reproduced by this test.
When `CLIEngine` instantiates `FileEnumerator`, it explicitly passes its own `CascadingConfigArrayFactory` instance, which now includes values for `builtInRules`, `loadRules`, `eslintRecommendedPath`, and `eslintAllPath`. This is the only place that ESLint core instantiates `FileEnumerator`, so core does not rely on the constructor's default value for `configArrayFactory`. After `CascadingConfigArrayFactory` was extracted into `@eslint/eslintrc`, it no longer assumed values for `eslintRecommendedPath` and `eslintAllPath`, which `CLIEngine` now provides. If those values are not passed, as is the case with the default `CascadingConfigArrayFactory` for `configArrayFactory`, file enumeration will encounter the exception that was reported in #13789. From the perspective of ESLint core, the default value for `configArrayFactory` is dead code. However, even though `FileEnumerator` is an undocumented API, it's called by `eslint-plugin-import`'s `no-unused-modules` rule, which hits the exception reproduced by this test.
btmills
added
bug
|
Hi @btmills!, thanks for the Pull Request The pull request title isn't properly formatted. We ask that you update the message to match this format, as we use it to generate changelogs and automate releases.
Read more about contributing to ESLint here |
btmills
changed the title
|
Fixed the commit message check by trimming trailing whitespace left over from a copy/paste. |
mdjermanovic
left a comment
mdjermanovic
left a comment
There was a problem hiding this comment.
LGTM!
I also checked this with changes from eslint/eslintrc#14 + some built-in rules configured, and it works well (had concerns if the builtInRules argument is also necessary; that's actually how I found out that we were not validating built-in rules).
Thanks for double checking. It makes sense that file enumeration would need to know paths to internal configs but not need to touch rules. I'm glad you checked and found the validation issue! |
eslint-github-bot
bot
added
the
archived due to age
Prerequisites checklist
What is the purpose of this pull request? (put an "X" next to an item)
[ ] Documentation update
[x] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:
What changes did you make? (Give an overview)
When
CLIEngineinstantiatesFileEnumerator, it explicitly passes its ownCascadingConfigArrayFactoryinstance, which now includes values forbuiltInRules,loadRules,eslintRecommendedPath, andeslintAllPath. This is the only place that ESLint core instantiatesFileEnumerator, so core does not rely on the constructor's default value forconfigArrayFactory.After
CascadingConfigArrayFactorywas extracted into@eslint/eslintrc, it no longer assumed values foreslintRecommendedPathandeslintAllPath, whichCLIEnginenow provides. If those values are not passed, as is the case with the defaultCascadingConfigArrayFactoryforconfigArrayFactory, file enumeration will encounter the exception that was reported in #13789.From the perspective of ESLint core, the default value for
configArrayFactoryis dead code. However, even thoughFileEnumeratoris an undocumented API, it's called byeslint-plugin-import'sno-unused-modulesrule, which hits the exception reproduced by this test.Is there anything you'd like reviewers to focus on?
FileEnumeratoris an undocumented API, so we probably shouldn't be encouraging people to use it, buteslint-plugin-import/no-unused-modulesalready does. We don't have a supported API to achieve the same purpose, but the pre-lint step contemplated in RFC42 would probably serve that purpose.