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

Fix: Pass internal config paths in FileEnumerator default (fixes #13789) #13792

Merged
merged 2 commits into from Oct 27, 2020

Conversation

@btmills
Copy link
Member

@btmills btmills commented Oct 26, 2020

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 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.

Is there anything you'd like reviewers to focus on?

FileEnumerator is an undocumented API, so we probably shouldn't be encouraging people to use it, but eslint-plugin-import/no-unused-modules already 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.

btmills added 2 commits Oct 25, 2020
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.
@eslint
Copy link
Contributor

@eslint eslint bot commented Oct 26, 2020

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.

  • The length of the commit message must be less than or equal to 72

Read more about contributing to ESLint here

@btmills btmills changed the title Fix: Pass internal config paths in FileEnumerator default (fixes #13789) Fix: Pass internal config paths in FileEnumerator default (fixes #13789) Oct 26, 2020
@btmills
Copy link
Member Author

@btmills btmills commented Oct 26, 2020

Fixed the commit message check by trimming trailing whitespace left over from a copy/paste.

Copy link
Member

@mdjermanovic mdjermanovic left a 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).

@btmills
Copy link
Member Author

@btmills btmills commented Oct 27, 2020

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!

@btmills btmills merged commit aeef485 into master Oct 27, 2020
12 checks passed
12 checks passed
Verify Files
Details
Test (ubuntu-latest, 14.x)
Details
Test (ubuntu-latest, 13.x)
Details
Test (ubuntu-latest, 12.x)
Details
Test (ubuntu-latest, 10.x)
Details
Test (ubuntu-latest, 10.12.0)
Details
Test (windows-latest, 12.x)
Details
Test (macOS-latest, 12.x)
Details
Browser Test
Details
commit-message PR title follows commit message guidelines
Details
licence/cla Contributor License Agreement is signed.
Details
release-monitor This change is semver-patch
Details
@btmills btmills deleted the issue13789 branch Oct 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.