fix(cli): do not crash on no rules configured#328
Conversation
In flat config the resolved config is "undefined" for files that don't
have any configuration, which caused an error in the CLI utility, unlike
in legacy ESLint configuration where the empty resolved configuration is
an empty object with an empty "rules: {}".
🦋 Changeset detectedLatest commit: cd29dcb The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughThe change updates the destructuring of the Changes
Assessment against linked issues
Poem
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to 0b82629 in 30 seconds. Click for details.
- Reviewed
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. bin/cli.js:63
- Draft comment:
Good use of destructuring with a default. This change ensures that if a config is undefined, 'rules' defaults to an empty object, avoiding errors when calling Object.entries(). - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. bin/cli.js:63
- Draft comment:
Good fix: defaulting 'rules' to {} prevents the destructuring error. Consider adding an inline comment referencing issue #288 to document why we use this fallback. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
Workflow ID: wflow_p2ecqH4CC3QoYznk
You can customize 
|
I didn't find an (easy) way to incorporate this into tests, I would probably have to mock config files, not sure if it's worth it. Let me know if I missed something. |
An e2e test case could be added? And some comments could be added at least. |
|
Since running the CLI doesn't allow me to specify the config, what kind of an E2E test did you have in mind? What I can do is pass a non-JS file like It would pick up your project's |
|
|
|
Great idea, done ✅ |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/cli.test.js (1)
81-87: Good addition of a test case for empty config scenario.This test effectively verifies that the CLI doesn't crash when encountering an empty configuration, which directly addresses issue #288 mentioned in the PR objectives. The test runs the CLI script as a child process against a fixture with an empty config and implicitly checks that no error occurs.
A few suggestions to strengthen this test:
- Consider asserting the expected CLI output (should be "No rules that are unnecessary or conflict with Prettier were found.")
- Add a comment explaining what the test is checking and its relationship to issue #288
test("empty config", (done) => { + // Verifies that the CLI doesn't crash with empty configs (issue #288) + // The fixture contains an eslint.config.mjs that exports [{}] childProcess.exec( `node ${path.join(process.cwd(), "bin/cli.js")} index.js`, { cwd: path.join(__dirname, "fixtures/empty-config") }, - done + (error, stdout) => { + expect(error).toBeNull(); + expect(stdout.trim()).toBe("No rules that are unnecessary or conflict with Prettier were found."); + done(); + } ); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
bin/cli.js(1 hunks)test/cli.test.js(2 hunks)test/fixtures/empty-config/eslint.config.mjs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- test/fixtures/empty-config/eslint.config.mjs
🚧 Files skipped from review as they are similar to previous changes (1)
- bin/cli.js
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Test on windows-latest with Node.js 22
- GitHub Check: Test on windows-latest with Node.js 20
- GitHub Check: Test on windows-latest with Node.js 18
- GitHub Check: Test on windows-latest with Node.js 16
JounQin
changed the title
In the case when a flat config has no rules defined, the resolved config for a file is
undefined.This causes the CLI utility to fail to destructure
rulesout of it:and it's also necessary to initialize
rulesas well, becauseObject.entries(undefined)is an error.This will achieve the same behavior as a legacy configuration:
Fixes #288.
Important
Fixes CLI crash in
bin/cli.jsby setting defaultrules = {}for flat configs with no rules, ensuring consistent behavior with legacy configurations.bin/cli.jswhen flat config has no rules defined by setting defaultrules = {}during destructuring.This description was created by
for 0b82629. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
Summary by CodeRabbit