-
-
Notifications
You must be signed in to change notification settings - Fork 256
fix(cli): do not crash on no rules configured #328
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
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 (
|
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.
Important
Looks good to me! 👍
Reviewed everything up to 0b82629 in 30 seconds. Click for details.
- Reviewed
13
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
2
draft 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 [CLI] Handle undefined computed config #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 by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
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 ✅ |
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.
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 [CLI] Handle undefined computed config #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
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.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
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.
Pull Request Overview
This PR fixes a bug in the CLI where an empty flat configuration (i.e., one with no rules specified) mistakenly caused a crash.
- Fixes CLI crash by defaulting missing rules to an empty object
- Adds a test to verify CLI behavior with an empty configuration
- Updates changeset metadata for the patch release
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
File | Description |
---|---|
test/fixtures/empty-config/eslint.config.mjs | Provides an empty configuration for testing |
test/cli.test.js | Adds a test verifying the CLI does not crash with empty config |
.changeset/lazy-dingos-cheat.md | Documents the patch release for this bug fix |
Comments suppressed due to low confidence (1)
test/cli.test.js:82
- Consider asserting the output message or exit code in the empty config test to ensure that the CLI behavior matches the expected "No rules that are unnecessary or conflict with Prettier were found." message.
test("empty config", (done) => {
| datasource | package | from | to | | ---------- | ---------------------- | ------ | ------ | | npm | eslint-config-prettier | 10.1.1 | 10.1.5 | ## [v10.1.5](https://github.com/prettier/eslint-config-prettier/blob/HEAD/CHANGELOG.md#1015) ##### Patch Changes - [#332](prettier/eslint-config-prettier#332) [`60fef02`](prettier/eslint-config-prettier@60fef02) Thanks [@JounQin](https://github.com/JounQin)! - chore: add `funding` field into `package.json` ## [v10.1.4](https://github.com/prettier/eslint-config-prettier/blob/HEAD/CHANGELOG.md#1014) ##### Patch Changes - [#328](prettier/eslint-config-prettier#328) [`94b4799`](prettier/eslint-config-prettier@94b4799) Thanks [@silvenon](https://github.com/silvenon)! - fix(cli): do not crash on no rules configured ## [v10.1.3](https://github.com/prettier/eslint-config-prettier/blob/HEAD/CHANGELOG.md#1013) ##### Patch Changes - [#325](prettier/eslint-config-prettier#325) [`4e95a1d`](prettier/eslint-config-prettier@4e95a1d) Thanks [@pilikan](https://github.com/pilikan)! - fix: this package is `commonjs`, align its types correctly ## [v10.1.2](https://github.com/prettier/eslint-config-prettier/blob/HEAD/CHANGELOG.md#1012) ##### Patch Changes - [#321](prettier/eslint-config-prettier#321) [`a8768bf`](prettier/eslint-config-prettier@a8768bf) Thanks [@Fdawgs](https://github.com/Fdawgs)! - chore(package): add homepage for some 3rd-party registry - see [#321](prettier/eslint-config-prettier#321) for more details
| datasource | package | from | to | | ---------- | ---------------------- | ------ | ------ | | npm | eslint-config-prettier | 10.1.1 | 10.1.5 | ## [v10.1.5](https://github.com/prettier/eslint-config-prettier/blob/HEAD/CHANGELOG.md#1015) ##### Patch Changes - [#332](prettier/eslint-config-prettier#332) [`60fef02`](prettier/eslint-config-prettier@60fef02) Thanks [@JounQin](https://github.com/JounQin)! - chore: add `funding` field into `package.json` ## [v10.1.4](https://github.com/prettier/eslint-config-prettier/blob/HEAD/CHANGELOG.md#1014) ##### Patch Changes - [#328](prettier/eslint-config-prettier#328) [`94b4799`](prettier/eslint-config-prettier@94b4799) Thanks [@silvenon](https://github.com/silvenon)! - fix(cli): do not crash on no rules configured ## [v10.1.3](https://github.com/prettier/eslint-config-prettier/blob/HEAD/CHANGELOG.md#1013) ##### Patch Changes - [#325](prettier/eslint-config-prettier#325) [`4e95a1d`](prettier/eslint-config-prettier@4e95a1d) Thanks [@pilikan](https://github.com/pilikan)! - fix: this package is `commonjs`, align its types correctly ## [v10.1.2](https://github.com/prettier/eslint-config-prettier/blob/HEAD/CHANGELOG.md#1012) ##### Patch Changes - [#321](prettier/eslint-config-prettier#321) [`a8768bf`](prettier/eslint-config-prettier@a8768bf) Thanks [@Fdawgs](https://github.com/Fdawgs)! - chore(package): add homepage for some 3rd-party registry - see [#321](prettier/eslint-config-prettier#321) for more details
| datasource | package | from | to | | ---------- | ---------------------- | ------ | ------ | | npm | eslint-config-prettier | 10.1.1 | 10.1.5 | ## [v10.1.5](https://github.com/prettier/eslint-config-prettier/blob/HEAD/CHANGELOG.md#1015) ##### Patch Changes - [#332](prettier/eslint-config-prettier#332) [`60fef02`](prettier/eslint-config-prettier@60fef02) Thanks [@JounQin](https://github.com/JounQin)! - chore: add `funding` field into `package.json` ## [v10.1.4](https://github.com/prettier/eslint-config-prettier/blob/HEAD/CHANGELOG.md#1014) ##### Patch Changes - [#328](prettier/eslint-config-prettier#328) [`94b4799`](prettier/eslint-config-prettier@94b4799) Thanks [@silvenon](https://github.com/silvenon)! - fix(cli): do not crash on no rules configured ## [v10.1.3](https://github.com/prettier/eslint-config-prettier/blob/HEAD/CHANGELOG.md#1013) ##### Patch Changes - [#325](prettier/eslint-config-prettier#325) [`4e95a1d`](prettier/eslint-config-prettier@4e95a1d) Thanks [@pilikan](https://github.com/pilikan)! - fix: this package is `commonjs`, align its types correctly ## [v10.1.2](https://github.com/prettier/eslint-config-prettier/blob/HEAD/CHANGELOG.md#1012) ##### Patch Changes - [#321](prettier/eslint-config-prettier#321) [`a8768bf`](prettier/eslint-config-prettier@a8768bf) Thanks [@Fdawgs](https://github.com/Fdawgs)! - chore(package): add homepage for some 3rd-party registry - see [#321](prettier/eslint-config-prettier#321) for more details
| datasource | package | from | to | | ---------- | ---------------------- | ------ | ------ | | npm | eslint-config-prettier | 10.1.1 | 10.1.5 | ## [v10.1.5](https://github.com/prettier/eslint-config-prettier/blob/HEAD/CHANGELOG.md#1015) ##### Patch Changes - [#332](prettier/eslint-config-prettier#332) [`60fef02`](prettier/eslint-config-prettier@60fef02) Thanks [@JounQin](https://github.com/JounQin)! - chore: add `funding` field into `package.json` ## [v10.1.4](https://github.com/prettier/eslint-config-prettier/blob/HEAD/CHANGELOG.md#1014) ##### Patch Changes - [#328](prettier/eslint-config-prettier#328) [`94b4799`](prettier/eslint-config-prettier@94b4799) Thanks [@silvenon](https://github.com/silvenon)! - fix(cli): do not crash on no rules configured ## [v10.1.3](https://github.com/prettier/eslint-config-prettier/blob/HEAD/CHANGELOG.md#1013) ##### Patch Changes - [#325](prettier/eslint-config-prettier#325) [`4e95a1d`](prettier/eslint-config-prettier@4e95a1d) Thanks [@pilikan](https://github.com/pilikan)! - fix: this package is `commonjs`, align its types correctly ## [v10.1.2](https://github.com/prettier/eslint-config-prettier/blob/HEAD/CHANGELOG.md#1012) ##### Patch Changes - [#321](prettier/eslint-config-prettier#321) [`a8768bf`](prettier/eslint-config-prettier@a8768bf) Thanks [@Fdawgs](https://github.com/Fdawgs)! - chore(package): add homepage for some 3rd-party registry - see [#321](prettier/eslint-config-prettier#321) for more details
| datasource | package | from | to | | ---------- | ---------------------- | ------ | ------ | | npm | eslint-config-prettier | 10.1.1 | 10.1.5 | ## [v10.1.5](https://github.com/prettier/eslint-config-prettier/blob/HEAD/CHANGELOG.md#1015) ##### Patch Changes - [#332](prettier/eslint-config-prettier#332) [`60fef02`](prettier/eslint-config-prettier@60fef02) Thanks [@JounQin](https://github.com/JounQin)! - chore: add `funding` field into `package.json` ## [v10.1.4](https://github.com/prettier/eslint-config-prettier/blob/HEAD/CHANGELOG.md#1014) ##### Patch Changes - [#328](prettier/eslint-config-prettier#328) [`94b4799`](prettier/eslint-config-prettier@94b4799) Thanks [@silvenon](https://github.com/silvenon)! - fix(cli): do not crash on no rules configured ## [v10.1.3](https://github.com/prettier/eslint-config-prettier/blob/HEAD/CHANGELOG.md#1013) ##### Patch Changes - [#325](prettier/eslint-config-prettier#325) [`4e95a1d`](prettier/eslint-config-prettier@4e95a1d) Thanks [@pilikan](https://github.com/pilikan)! - fix: this package is `commonjs`, align its types correctly ## [v10.1.2](https://github.com/prettier/eslint-config-prettier/blob/HEAD/CHANGELOG.md#1012) ##### Patch Changes - [#321](prettier/eslint-config-prettier#321) [`a8768bf`](prettier/eslint-config-prettier@a8768bf) Thanks [@Fdawgs](https://github.com/Fdawgs)! - chore(package): add homepage for some 3rd-party registry - see [#321](prettier/eslint-config-prettier#321) for more details
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
rules
out of it:and it's also necessary to initialize
rules
as 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.js
by setting defaultrules = {}
for flat configs with no rules, ensuring consistent behavior with legacy configurations.bin/cli.js
when 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