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] no-unused-modules: don't error out when running with flat config and an eslintrc isn't present #3116

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

michaelfaith
Copy link
Contributor

@michaelfaith michaelfaith commented Dec 15, 2024

This change adjusts what we're doing if an error is thrown while attempting to enumerate lintable files in the no-used-modules rule, when users are running with flat config. Now, if FileEnumerator throws due to a lack of eslintrc and the user is running with flat config, we catch the error and rethrow with a more informative message.

I also cleaned up the original aspects of the implementation that was using the proposed eslint context functions, since that proposal was walked back and the API was never introduced.

Note: This isn't an ideal state, since this rule still relies on the legacy api to understand what needs to be ignored. Remaining tethered to the legacy config system is going to need to be solved at some point.

Fixes #3079

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Looks pretty good, thanks!

.gitignore Outdated Show resolved Hide resolved
src/rules/no-unused-modules.js Outdated Show resolved Hide resolved
Copy link

codecov bot commented Dec 15, 2024

Codecov Report

Attention: Patch coverage is 25.00000% with 9 lines in your changes missing coverage. Please review.

Project coverage is 92.06%. Comparing base (8b2d570) to head (9993d21).

Files with missing lines Patch % Lines
src/rules/no-unused-modules.js 25.00% 9 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3116      +/-   ##
==========================================
- Coverage   95.04%   92.06%   -2.98%     
==========================================
  Files          84       83       -1     
  Lines        3634     3618      -16     
  Branches     1279     1276       -3     
==========================================
- Hits         3454     3331     -123     
- Misses        180      287     +107     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@michaelfaith michaelfaith force-pushed the fix/no-unused-modules branch 2 times, most recently from 4a50098 to d5ba9af Compare December 15, 2024 22:52
@michaelfaith
Copy link
Contributor Author

Addressed lint errors

@ljharb ljharb force-pushed the fix/no-unused-modules branch from d5ba9af to a515ac9 Compare December 16, 2024 06:03
@ljharb ljharb changed the title fix(no-unused-modules): don't error out when running with flat config and an eslintrc isn't present [Fix] no-unused-modules: don't error out when running with flat config and an eslintrc isn't present Dec 16, 2024
@michaelfaith michaelfaith marked this pull request as draft December 16, 2024 16:07
@michaelfaith michaelfaith force-pushed the fix/no-unused-modules branch 2 times, most recently from 7ba8372 to bc01442 Compare December 20, 2024 15:15
@michaelfaith
Copy link
Contributor Author

michaelfaith commented Dec 20, 2024

Updated, based on feedback in #3079 to not attempt to avoid the error, but to instead rethrow with a more informative message. I updated the PR description to reflect the new approach. There's not a great way to test this now. With the integration tests before, we could remove the rc config, run lint, and observe that it didn't error out. Now we're wanting it to error out when no rc is present, and there's not a great way to recreate that in the rule's unit tests or in integration tests that don't throw (unless we write a custom node script to serve as an integration test). Thoughts?

@michaelfaith michaelfaith marked this pull request as ready for review December 20, 2024 15:24
@ljharb ljharb force-pushed the fix/no-unused-modules branch from bc01442 to e17e073 Compare December 21, 2024 00:14
@ljharb
Copy link
Member

ljharb commented Dec 21, 2024

I suppose we could have a test make a temp dir (using tmp), copy an example into it, run npm install in it, and then invoke eslint inside it?

Copy link

socket-security bot commented Dec 22, 2024

@michaelfaith
Copy link
Contributor Author

michaelfaith commented Dec 22, 2024

I suppose we could have a test make a temp dir (using tmp), copy an example into it, run npm install in it, and then invoke eslint inside it?

Added a test that does the following:

  • creates a tmp folder
  • copies the v9 example folder into it
  • runs exec to install the plugin from cwd and run the lint command
  • catches the error and validates that the message includes our new verbiage

CHANGELOG.md Outdated Show resolved Hide resolved
@michaelfaith michaelfaith force-pushed the fix/no-unused-modules branch 3 times, most recently from 9993d21 to dda50b9 Compare December 22, 2024 22:41
… no eslintrc is present

This change adjusts what we're doing if an error is thrown while attempting to enumerate lintable files in the no-used-modules rule, when users are running with flat config. Now, if FileEnumerator throws due to a lack of eslintrc and the user is running with flat config, we catch the error and rethrow with a more informative message.

I also cleaned up the original aspects of the implementation that was using the proposed eslint context functions, since that proposal was walked back and the API was never introduced.

Note: This isn't an ideal state, since this rule still relies on the legacy api to understand what needs to be ignored. Remaining tethered to the legacy config system is going to need to be solved at some point.

Fixes import-js#3079
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants