refactor: drop @typescript-eslint/utils on production#362
Conversation
🦋 Changeset detectedLatest commit: cfb000d 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 |
WalkthroughThis update refactors type imports throughout the codebase to reduce reliance on Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant RuleCreator
participant RuleDefinition
participant applyDefault
participant deepMerge
User->>RuleCreator: Create new rule with meta, options, and docsUrl
RuleCreator->>RuleDefinition: Inject docsUrl into meta
RuleCreator->>applyDefault: Merge user options with defaults
applyDefault->>deepMerge: Deep merge object options if needed
applyDefault-->>RuleCreator: Return merged options
RuleCreator-->>User: Return ESLint rule module with merged options and docsUrl
Possibly related issues
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
src/rules/dynamic-import-chunkname.tsOops! Something went wrong! :( ESLint: 9.28.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find module '/lib/index.js' imported from /eslint.config.js src/rules/extensions.tsOops! Something went wrong! :( ESLint: 9.28.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find module '/lib/index.js' imported from /eslint.config.js src/rules/no-unused-modules.tsOops! Something went wrong! :( ESLint: 9.28.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find module '/lib/index.js' imported from /eslint.config.js
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (17)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (16)
⏰ Context from checks skipped due to timeout of 90000ms (20)
✨ Finishing Touches
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 (
|
WalkthroughThis pull request refactors the codebase to remove Changes
|
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the codebase to drop direct usage of production types from @typescript-eslint/utils and instead use types from @typescript-eslint/types while still relying on @typescript-eslint/utils for testing. Key changes include updating type imports and references in test and rule files, adding a new utility for deep merging objects, and updating package.json dependencies and peerDependencies to reflect the refactor.
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/utils.ts | Updated type imports from @typescript-eslint/utils/ts-eslint to use TSESLint type. |
| test/rules/no-empty-named-blocks.spec.ts | Adjusted parser type imports to use TSESLint.Parser interface. |
| src/utils/import-declaration.ts | Replaced AST_NODE_TYPES import to use @typescript-eslint/types. |
| src/utils/get-value.ts | Updated TSESTree import from @typescript-eslint/types. |
| src/utils/deep-merge.ts | New utility implementation for deep merging objects. |
| src/utils/create-rule.ts | Refactored rule creator to use a custom RuleCreator function. |
| src/utils/apply-default.ts | Added a helper to merge default options with user-provided options. |
| src/rules/* | Updated various rule files to use the new typings for fixer and parser. |
| package.json | Updated dependency declarations to drop @typescript-eslint/utils in production and adjust peerDependencies. |
| .changeset/legal-squids-cover.md | Updated changeset with a minor refactor description. |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to a603e02 in 1 minute and 0 seconds. Click for details.
- Reviewed
445lines of code in13files - Skipped
1files when reviewing. - Skipped posting
14draft 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. .changeset/legal-squids-cover.md:1
- Draft comment:
Ensure the changeset message accurately reflects the removal of @typescript-eslint/utils from production dependencies. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to ensure that the changeset message accurately reflects a change. This falls under the rule of not asking the author to update the PR description or similar documentation. Therefore, this comment should be removed.
2. package.json:67
- Draft comment:
Peer dependency for '@typescript-eslint/utils' is now declared while production code uses '@typescript-eslint/types'. Verify that these version constraints align with your tooling. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is about dependency version constraints, which is generally not something I should comment on unless it's a specific suggestion or confirmation request. This comment asks the author to verify alignment, which is not allowed.
3. src/rules/dynamic-import-chunkname.ts:148
- Draft comment:
Updated fixer parameter type to TSESLint.RuleFixer for better type clarity. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative, as it only states what was changed without providing any actionable feedback or suggestions. It doesn't ask for confirmation or suggest improvements.
4. src/rules/export.ts:1
- Draft comment:
Importing AST_NODE_TYPES from '@typescript-eslint/types' improves consistency. Ensure downstream code is compatible. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is asking the PR author to ensure compatibility with downstream code, which violates the rule against asking the author to ensure things are tested or verified. It does not provide a specific suggestion or point out a specific issue.
5. src/rules/extensions.ts:24
- Draft comment:
Using 'satisfies JSONSchema.JSONSchema4' provides explicit type validation. Confirm that the JSONSchema type from @typescript-eslint/utils is the intended one. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking for confirmation about the use of a specific type from a library, which is not allowed according to the rules. It does not provide a specific code suggestion or ask for a test to be written. Therefore, it should be removed.
6. src/rules/extensions.ts:370
- Draft comment:
Changed fixer parameter type to TSESLint.RuleFixer to maintain consistency in rule fix functions. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative, explaining a change made to maintain consistency. It does not provide a suggestion, ask for confirmation, or highlight a potential issue.
7. src/rules/no-unused-modules.ts:8
- Draft comment:
Updated import for TSESTree to use '@typescript-eslint/types' for consistency with other modules. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative, as it only states what was changed without providing any actionable feedback or suggestions. It does not ask for confirmation or suggest any improvements.
8. src/utils/apply-default.ts:19
- Draft comment:
Using structuredClone for deep cloning of default options is efficient; ensure the Node version meets the requirement. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is suggesting to ensure that the Node version supportsstructuredClone. This is a purely informative comment, as it doesn't provide a specific suggestion or ask for a specific action. It violates the rule against making purely informative comments.
9. src/utils/create-rule.ts:27
- Draft comment:
The custom RuleCreator now leverages applyDefault for merging options. Confirm that this abstraction remains compatible with existing rule implementations. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to confirm compatibility with existing rule implementations, which violates the rule against asking for confirmation or ensuring behavior is intended. It does not provide a specific code suggestion or ask for a specific test to be written.
10. src/utils/deep-merge.ts:8
- Draft comment:
The isObjectNotArray and deepMerge functions correctly avoid merging arrays. Verify this behavior is intended for all use cases. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is asking the PR author to verify the behavior of two functions, which violates the rule against asking for confirmation of intention or ensuring behavior is intended. It does not provide a specific suggestion or point out a clear issue.
11. src/utils/get-value.ts:1
- Draft comment:
Importing TSESTree from '@typescript-eslint/types' aligns with recent refactors. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative, as it only states that the import aligns with recent refactors. It doesn't provide any actionable feedback or suggestions for improvement.
12. src/utils/import-declaration.ts:1
- Draft comment:
Refactored AST_NODE_TYPES import to use '@typescript-eslint/types'; maintains consistency across modules. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative, explaining a change that was made without providing any actionable feedback or suggestions. It does not align with the rules for useful comments.
13. test/rules/no-empty-named-blocks.spec.ts:15
- Draft comment:
Updated parser type to use TSESLint.Parser.LooseParserModule for clarity in test cases. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative, explaining a change made in the code without suggesting any action or asking for clarification. It doesn't provide any actionable feedback or raise any concerns about the code change.
14. test/utils.ts:70
- Draft comment:
Updated type helpers (e.g. GetRuleModuleTypes) to use TSESLint.RuleModule ensuring consistent type inference across tests. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative, explaining what was done without providing any actionable feedback or suggestions. It doesn't ask for confirmation or suggest any improvements.
Workflow ID: wflow_KFOq3arXD9NcXj0C
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferences |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #362 +/- ##
==========================================
+ Coverage 96.02% 96.04% +0.01%
==========================================
Files 93 95 +2
Lines 4884 4926 +42
Branches 1834 1827 -7
==========================================
+ Hits 4690 4731 +41
- Misses 193 194 +1
Partials 1 1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
commit: |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/utils/deep-merge.ts (2)
20-23: Consider tightening return type for better type safety.The function parameters are typed as
ObjectLikebut the return type isRecord<string, unknown>. Consider using a more specific return type.-): Record<string, unknown> { +): ObjectLike {
34-46: Verify edge case handling and add test coverage.The merge logic is well-structured, but some branches lack test coverage according to static analysis. Consider these edge cases:
- Stack overflow risk: Deep recursion with circular references could cause issues
- Performance: Large object merging could be expensive
Please ensure test coverage for:
- Line 39: Recursive merge branch
- Lines 43-45: Single object key handling
Consider adding a depth limit or circular reference detection for production safety:
export function deepMerge( first: ObjectLike = {}, second: ObjectLike = {}, + maxDepth = 10, + _currentDepth = 0, ): ObjectLike { + if (_currentDepth >= maxDepth) { + return { ...first, ...second } + } // existing logic... value = isObjectNotArray(firstValue) && isObjectNotArray(secondValue) - ? deepMerge(firstValue, secondValue) + ? deepMerge(firstValue, secondValue, maxDepth, _currentDepth + 1) : secondValue🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 39-39: src/utils/deep-merge.ts#L39
Added line #L39 was not covered by tests
[warning] 43-45: src/utils/deep-merge.ts#L43-L45
Added lines #L43 - L45 were not covered by testssrc/utils/apply-default.ts (1)
22-22: Add test coverage for uncovered branches.Static analysis indicates that lines 22 and 33 are not covered by tests. These represent important code paths:
- Line 22: Early return when
userOptionsis null- Line 33: Direct assignment branch when options are not objects
Would you like me to help generate test cases to cover these scenarios?
Also applies to: 33-33
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 22-22: src/utils/apply-default.ts#L22
Added line #L22 was not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (13)
.changeset/legal-squids-cover.md(1 hunks)package.json(2 hunks)src/rules/dynamic-import-chunkname.ts(2 hunks)src/rules/export.ts(1 hunks)src/rules/extensions.ts(6 hunks)src/rules/no-unused-modules.ts(1 hunks)src/utils/apply-default.ts(1 hunks)src/utils/create-rule.ts(1 hunks)src/utils/deep-merge.ts(1 hunks)src/utils/get-value.ts(1 hunks)src/utils/import-declaration.ts(1 hunks)test/rules/no-empty-named-blocks.spec.ts(2 hunks)test/utils.ts(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/utils/apply-default.ts (1)
src/utils/deep-merge.ts (2)
isObjectNotArray(8-10)deepMerge(20-50)
🪛 GitHub Check: codecov/patch
src/utils/apply-default.ts
[warning] 22-22: src/utils/apply-default.ts#L22
Added line #L22 was not covered by tests
[warning] 33-33: src/utils/apply-default.ts#L33
Added line #L33 was not covered by tests
src/utils/deep-merge.ts
[warning] 39-39: src/utils/deep-merge.ts#L39
Added line #L39 was not covered by tests
[warning] 43-45: src/utils/deep-merge.ts#L43-L45
Added lines #L43 - L45 were not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (159)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Lint and Test with Node.js 24 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js lts/* and ESLint 9 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js lts/* and ESLint 9 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 9 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on macos-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 8.56 on macos-latest
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Lint and Test with Node.js 24 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js lts/* and ESLint 9 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js lts/* and ESLint 9 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 9 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on macos-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 8.56 on macos-latest
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Lint and Test with Node.js 24 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js lts/* and ESLint 9 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js lts/* and ESLint 9 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 9 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on macos-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 8.56 on macos-latest
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Lint and Test with Node.js 24 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js lts/* and ESLint 9 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js lts/* and ESLint 9 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 9 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on macos-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 8.56 on macos-latest
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Lint and Test with Node.js 24 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js lts/* and ESLint 9 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js lts/* and ESLint 9 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 9 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on macos-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 8.56 on macos-latest
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Lint and Test with Node.js 24 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js lts/* and ESLint 9 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js lts/* and ESLint 9 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 9 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on macos-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 8.56 on macos-latest
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Lint and Test with Node.js 24 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js lts/* and ESLint 9 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js lts/* and ESLint 9 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 9 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on macos-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 8.56 on macos-latest
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Lint and Test with Node.js 24 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js lts/* and ESLint 9 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js lts/* and ESLint 9 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 9 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on macos-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 8.56 on macos-latest
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Lint and Test with Node.js 24 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js lts/* and ESLint 9 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js lts/* and ESLint 9 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 9 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on macos-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 8.56 on macos-latest
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Lint and Test with Node.js 24 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js lts/* and ESLint 9 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js lts/* and ESLint 9 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 9 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on macos-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 8.56 on macos-latest
🔇 Additional comments (21)
src/utils/deep-merge.ts (2)
1-1: LGTM: Clean type definition.The
ObjectLiketype alias provides a clear, generic way to represent objects with string keys.
8-10: LGTM: Robust type guard implementation.The
isObjectNotArrayfunction correctly identifies objects while explicitly excluding arrays and null values. The logic is sound and well-documented.src/utils/get-value.ts (1)
1-1: LGTM: Consistent with dependency refactoring objectives.The import change from
@typescript-eslint/utilsto@typescript-eslint/typesaligns perfectly with the PR goal of dropping utils from production dependencies..changeset/legal-squids-cover.md (1)
1-6: LGTM: Clear and accurate changelog entry.The changeset properly documents the refactoring work with appropriate versioning (minor) and a clear description that matches the PR objectives.
src/rules/export.ts (1)
1-2: LGTM! Proper separation of runtime values and types.The change correctly moves
AST_NODE_TYPES(runtime enum) to@typescript-eslint/typeswhile keepingTSESTree(type-only) in@typescript-eslint/utils. This aligns with the PR objective to reduce production dependencies on@typescript-eslint/utils.src/rules/no-unused-modules.ts (1)
8-9: LGTM! Consistent with the refactoring strategy.Moving
TSESTreeto@typescript-eslint/typesis correct since it contains runtime values likeAST_NODE_TYPES(used on line 86). This change aligns with the pattern of moving runtime enums to the types package while keeping ESLint-specific types in utils.src/rules/dynamic-import-chunkname.ts (2)
3-3: LGTM! Proper import consolidation.The consolidated import statement centralizes types under the
@typescript-eslint/utilsnamespace, reducing individual imports while maintaining type safety.
148-148: LGTM! Correct namespace type usage.The type annotation correctly uses the namespaced
TSESLint.RuleFixerinstead of the individual import, consistent with the consolidated import strategy.src/rules/extensions.ts (3)
3-3: LGTM! Proper import consolidation.The consolidated import statement reduces individual type imports by using the namespace approach, which aligns with the refactoring strategy.
24-24: LGTM! Correct namespace type usage.The type annotations correctly use
JSONSchema.JSONSchema4instead of the individual import, maintaining type safety with the consolidated import strategy.Also applies to: 29-29, 61-61
372-372: LGTM! Consistent namespace access for fixer types.All fixer function parameters correctly use
TSESLint.RuleFixernamespace access, maintaining consistency with the consolidated import approach.Also applies to: 417-417, 447-447
test/rules/no-empty-named-blocks.spec.ts (1)
2-2: LGTM! Clean namespace consolidation.The import refactoring to use the
TSESLintnamespace from@typescript-eslint/utilsand the corresponding type annotation update are consistent with the broader codebase refactoring. This improves import organization and aligns with the dependency restructuring.Also applies to: 15-15
src/utils/apply-default.ts (3)
27-27: Type casting is necessary but well-documented.The type casting
(options as unknown[])is required to work around TypeScript's limitation with generic array iteration. The comment above explains the rationale clearly.
11-38: Well-implemented utility function with clear semantics.The
applyDefaultfunction correctly implements pure option merging with proper cloning to avoid mutation. The deep merge logic for objects while preserving direct assignment for other types is appropriate.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 22-22: src/utils/apply-default.ts#L22
Added line #L22 was not covered by tests
[warning] 33-33: src/utils/apply-default.ts#L33
Added line #L33 was not covered by tests
19-19:Details
❓ Verification inconclusive
Verify browser compatibility for
structuredClone.The
structuredCloneAPI is relatively new (ES2022) and may not be available in older browsers or Node.js versions. Consider whether this library needs to support older environments.
🏁 Script executed:
#!/bin/bash # Check the minimum Node.js version specified in package.json echo "Checking minimum Node.js version requirement..." cat package.json | jq -r '.engines.node' echo -e "\nChecking structuredClone support in specified Node.js versions..." echo "structuredClone was added in Node.js v17.0.0" echo "The minimum version ^18.18.0 supports structuredClone ✓"Length of output: 544
To determine if browser targets require a fallback for
structuredClone, let’s check your package/browser configuration:
🏁 Script executed:
#!/bin/bash # Inspect browserslist in package.json echo "package.json › browserslist:" jq -r '.browserslist // "not defined"' package.json # Look for standalone Browserslist config files echo -e "\nLooking for .browserslistrc or browserslist files:" find . -maxdepth 2 -type f \( -name ".browserslistrc" -o -name "browserslist" \) || echo "none found" # Check TS compilation targets echo -e "\ntsconfig.json › compilerOptions.target and lib:" jq -r '.compilerOptions | "target: \(.target // "unset"), lib: \(.lib // [])"' tsconfig.jsonLength of output: 608
Unable to confirm browser targets for structuredClone
It looks like there’s no
browserslistin package.json, no standalone Browserslist file, and notsconfig.jsonto infer your compilation targets. Please verify whether your supported browsers all implement the ES2022structuredCloneAPI.
If you need to support older environments, consider one of the following:
- Add a polyfill (e.g., use the @ungap/structured-clone package).
- Fallback to a deep-clone utility (e.g.,
lodash.cloneDeeporJSON.parse(JSON.stringify(...))) in environments lackingstructuredClone.package.json (1)
67-67: Excellent dependency restructuring for production optimization.The refactoring to make
@typescript-eslint/utilsan optional peer dependency while adding@typescript-eslint/typesas a direct dependency is well-designed:
- Reduces production bundle size by making utils optional
- Separates type-only dependencies from runtime dependencies
- Maintains development functionality by keeping utils as dev dependency
- Version constraints are appropriate and consistent
This aligns perfectly with the codebase refactoring to separate type imports from runtime utilities.
Also applies to: 72-74, 80-80, 130-130
test/utils.ts (1)
8-8: Consistent namespace consolidation throughout utility types.All type references have been properly updated to use
TSESLint.RuleModulefrom the consolidated namespace. The changes maintain type safety while aligning with the new import structure. JSDoc comments are also kept in sync.Also applies to: 71-71, 79-84, 88-88, 108-108, 115-116, 144-144
src/utils/create-rule.ts (4)
1-1: LGTM!The type-only import correctly removes the runtime dependency on
@typescript-eslint/utils, aligning with the PR objective.
40-61: Well-implemented helper function!The
createRule_function correctly applies default options before invoking the rule logic, maintaining proper separation of concerns.
70-70: LGTM!The export maintains backward compatibility while using the new implementation.
15-16:Details
❌ Incorrect review comment
Remove or update the outdated TODO comment.
The referenced TypeScript PR #26349 is from 2018 and has likely been resolved. This comment should be removed or updated to reflect the current state.
🌐 Web query:
What is the status of TypeScript PR #26349 and has it been merged?💡 Result:
TypeScript PR #26349, titled "Support defaultProps as a static property on function components," was opened in the Microsoft/TypeScript GitHub repository in September 2018. The purpose of this PR was to add support for recognizing
defaultPropsdefined as a static property directly on function components—a commonly requested feature in the React community.As of the current date, May 31, 2025, PR #26349 has not been merged into the TypeScript codebase. The PR remains open and, over the years, has seen periodic discussion and debate. The maintainers have generally cited concerns about increased complexity and maintenance burdens, as well as the broader architectural direction of TypeScript and its React JSX support. The recommended approach remains using TypeScript’s built-in support for default values in function parameters, rather than static
defaultProps, with the overall trend in the React ecosystem also moving away fromdefaultPropsfor function components.You can verify the current, unmerged status by checking the official Microsoft/TypeScript GitHub pull request list for PR #26349[4]. No official plans to merge this PR have been announced, and the feature has not been incorporated into any TypeScript release as of May 2025.
In summary:
- PR #26349 is still open and has not been merged.
- The feature proposed in the PR is not part of TypeScript as of the current date.
- There is no indication from the maintainers that this will change in the near future[4].
Citations:
- 1: ianstormtaylor/slate#4003
- 2: https://www.typescriptlang.org/docs/handbook/declaration-merging.html
- 3: https://forum.buildkite.community/t/conditions-to-check-if-pr-is-closed-or-merged/3696
- 4: https://github.com/microsoft/typescript/pulls
- 5: https://dev.to/nickytonline/comment/a85d
- 6: https://docs.github.com/repositories/configuring-branches-and-merges-in-your-repository/managing-protected-branches/about-protected-branches
- 7: https://graphite.dev/guides/understanding-a-pull-requests-status
- 8: https://www.omarileon.me/blog/typescript-merge-objects
- 9: https://docs.gitlab.com/api/merge_requests/
- 10: https://docs.gitlab.com/ci/pipelines/merge_request_pipelines/
Keep the TODO comment; PR #26349 is still open
The referenced TypeScript PR is still open and hasn’t been merged. The TODO—“when this is merged; add type checking for the context.report
dataproperty”—remains valid. No change needed.Likely an incorrect or invalid review comment.
a603e02 to
3834c3f
Compare
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 3834c3f in 1 minute and 37 seconds. Click for details.
- Reviewed
659lines of code in17files - Skipped
1files when reviewing. - Skipped posting
6draft 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. src/rules/export.ts:1
- Draft comment:
Consider importing TSESTree from '@typescript-eslint/types' instead of '@typescript-eslint/utils' for consistency. Other modules (e.g. no-unused-modules.ts) have migrated, so aligning here would help. - Reason this comment was not posted:
Comment was on unchanged code.
2. src/utils/import-declaration.ts:1
- Draft comment:
Update the TSESTree import to use '@typescript-eslint/types' instead of '@typescript-eslint/utils' to avoid pulling in unused production code. - Reason this comment was not posted:
Comment was on unchanged code.
3. test/utils/apply-default.spec.ts:13
- Draft comment:
Consider rephrasing the test description 'returns applies a deepMerge to each element in the array' for clarity (e.g. 'applies deepMerge to each element in the array'). - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
4. src/rules/no-unused-modules.ts:10
- Draft comment:
There's a minor spacing issue in the comment: consider removing the extra space before the comma, changing "incorrect types , commonjs actually" to "incorrect types, commonjs actually". - Reason this comment was not posted:
Comment was on unchanged code.
5. test/utils.ts:113
- Draft comment:
Typo: The comment on line 113 mentions "both valid and invalid test case". Since the intent is referring to multiple cases, consider using the plural form "test cases". - Reason this comment was not posted:
Comment was on unchanged code.
6. test/utils/apply-default.spec.ts:13
- Draft comment:
Typo in test description: "returns applies a deepMerge to each element in the array". Consider revising it for clarity, e.g., "applies a deep merge to each element in the array". - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% While the grammar is technically incorrect, test descriptions are internal documentation and minor grammatical issues don't impact code functionality. The meaning is still clear despite the error. The rules state we should not make purely informative comments that don't require code changes. Grammar fixes in test descriptions fall into this category of being too minor to warrant a comment. The grammar error could make the code slightly less professional and harder to maintain in the future. Clear test descriptions are important for documentation. While clear documentation is valuable, this is a very minor grammatical issue that doesn't impact understanding. The rules explicitly state we should not make purely informative comments or obvious/unimportant ones. This comment should be deleted as it's purely about grammar in a test description and doesn't impact functionality or maintainability enough to warrant a comment.
Workflow ID: wflow_BlDATZPQh1TRrJ0J
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
test/utils/create-rule.spec.ts (1)
14-45: Consider adding edge case tests.While the current test coverage is good, consider adding tests for edge cases such as:
- Rules without docs property
- Rules with existing URL in docs (to verify it gets overwritten)
- Different types of rule metadata configurations
Example additional test case:
+ it('should handle rule without docs property', () => { + const rule = createRule({ + create() { + return {} + }, + defaultOptions: [], + meta: { + messages: { + foo: 'some message', + }, + schema: [], + type: 'problem', + }, + name: 'test-no-docs', + }) + expect(rule.meta.docs?.url).toBe('test/test-no-docs') + })test/utils/deep-merge.spec.ts (1)
41-41: Consider testing array handling explicitly.While the current tests include arrays in the test data, consider adding a specific test case for array merging behavior to clearly document that arrays are replaced rather than merged element-wise.
Example additional test case:
+ it('replaces arrays rather than merging them', () => { + const a = { + arr: [1, 2, 3] + } + const b = { + arr: [4, 5] + } + + expect(deepMerge(a, b)).toStrictEqual({ arr: [4, 5] }) + })test/utils/apply-default.spec.ts (1)
13-13: Minor typo in test description.There's a small grammatical issue in the test description.
- it('returns applies a deepMerge to each element in the array', () => { + it('applies deepMerge to each element in the array', () => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (17)
.changeset/legal-squids-cover.md(1 hunks)package.json(2 hunks)src/rules/dynamic-import-chunkname.ts(2 hunks)src/rules/export.ts(1 hunks)src/rules/extensions.ts(6 hunks)src/rules/no-unused-modules.ts(1 hunks)src/utils/apply-default.ts(1 hunks)src/utils/create-rule.ts(1 hunks)src/utils/deep-merge.ts(1 hunks)src/utils/get-value.ts(1 hunks)src/utils/import-declaration.ts(1 hunks)src/utils/index.ts(1 hunks)test/rules/no-empty-named-blocks.spec.ts(2 hunks)test/utils.ts(4 hunks)test/utils/apply-default.spec.ts(1 hunks)test/utils/create-rule.spec.ts(1 hunks)test/utils/deep-merge.spec.ts(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- src/utils/get-value.ts
- src/utils/index.ts
- src/utils/deep-merge.ts
🚧 Files skipped from review as they are similar to previous changes (11)
- src/utils/import-declaration.ts
- .changeset/legal-squids-cover.md
- src/rules/export.ts
- src/rules/no-unused-modules.ts
- src/rules/dynamic-import-chunkname.ts
- test/rules/no-empty-named-blocks.spec.ts
- src/utils/apply-default.ts
- package.json
- test/utils.ts
- src/utils/create-rule.ts
- src/rules/extensions.ts
🧰 Additional context used
🧬 Code Graph Analysis (2)
test/utils/create-rule.spec.ts (1)
src/utils/create-rule.ts (2)
RuleCreator(12-38)createRule(70-70)
test/utils/deep-merge.spec.ts (1)
src/utils/deep-merge.ts (1)
deepMerge(20-50)
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 9 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 9 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 9 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 8.56 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8.56 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8.56 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 9 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 8.56 on macos-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on macos-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 9 on macos-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on macos-latest
🔇 Additional comments (3)
test/utils/create-rule.spec.ts (1)
1-46: Well-structured test suite with good coverage.The test suite effectively covers the core functionality of the
RuleCreatorfunction. The tests verify that:
- The returned function is indeed a function
- Rules are created correctly with proper metadata structure
- URL injection works as expected using the provided URL generator
The test structure follows good practices with clear descriptions and appropriate assertions.
test/utils/deep-merge.spec.ts (1)
1-58: Comprehensive test coverage for deepMerge functionality.The test suite effectively covers the key aspects of the
deepMergefunction:
- Object immutability - ensuring new objects are created
- Complex deep merging with nested objects and arrays
- Property overwriting behavior where second object takes precedence
The test data is well-structured with realistic scenarios including nested objects, arrays, and various primitive types.
test/utils/apply-default.spec.ts (1)
1-76: Excellent comprehensive test coverage.The test suite thoroughly covers the
applyDefaultfunction with well-designed test cases:
- Default cloning behavior when no user options are provided
- Deep merging of arrays with object elements
- Proper handling of primitive arrays
- Edge case with null defaults
- Immutability guarantees throughout
The test data is realistic and the assertions properly verify both functionality and immutability requirements.
3834c3f to
cfb000d
Compare
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed cfb000d in 2 minutes and 11 seconds. Click for details.
- Reviewed
659lines of code in17files - Skipped
1files when reviewing. - Skipped posting
12draft 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. package.json:66
- Draft comment:
Ensure that '@typescript-eslint/utils' is only included as an optional peer dependency while production code now uses '@typescript-eslint/types'. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
2. src/rules/dynamic-import-chunkname.ts:148
- Draft comment:
The fixer parameter type is updated to TSESLint.RuleFixer—verify this change is consistent across all rule files. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
3. src/rules/export.ts:1
- Draft comment:
Importing AST_NODE_TYPES from '@typescript-eslint/types' reduces bundle size. Confirm that similar adjustments are applied consistently. - Reason this comment was not posted:
Confidence changes required:20%<= threshold50%None
4. src/rules/extensions.ts:24
- Draft comment:
Using 'satisfies JSONSchema.JSONSchema4' for the schema and updating fixer types to TSESLint.RuleFixer improves type safety and consistency. - Reason this comment was not posted:
Confidence changes required:20%<= threshold50%None
5. src/rules/no-unused-modules.ts:8
- Draft comment:
Switching to import TSESTree from '@typescript-eslint/types' is aligned with the refactor; ensure that runtime code does not pull in unneeded utilities. - Reason this comment was not posted:
Confidence changes required:20%<= threshold50%None
6. src/utils/create-rule.ts:12
- Draft comment:
The new RuleCreator abstraction cleanly injects documentation URLs and applies default options. Note the TODO about adding type checks for context.report.data in the future. - Reason this comment was not posted:
Confidence changes required:10%<= threshold50%None
7. src/utils/apply-default.ts:19
- Draft comment:
Using structuredClone to clone defaults is acceptable given the Node version requirements; just ensure compatibility if supporting older Node versions. - Reason this comment was not posted:
Confidence changes required:10%<= threshold50%None
8. src/utils/get-value.ts:1
- Draft comment:
Importing TSESTree from '@typescript-eslint/types' avoids including extra utility code in production, which meets the refactor goal. - Reason this comment was not posted:
Confidence changes required:10%<= threshold50%None
9. src/utils/import-declaration.ts:1
- Draft comment:
The import now uses '@typescript-eslint/types' for AST_NODE_TYPES, ensuring consistency with the production refactor. - Reason this comment was not posted:
Confidence changes required:10%<= threshold50%None
10. test/utils/deep-merge.spec.ts:41
- Draft comment:
The deepMerge tests are solid; consider adding additional edge cases for deeply nested objects in future tests. - Reason this comment was not posted:
Confidence changes required:10%<= threshold50%None
11. src/utils/apply-default.ts:8
- Draft comment:
Typo: Consider replacing "user opts" with "user options" in the parameter documentation for clarity. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% This is an extremely minor stylistic suggestion about documentation. While technically about a changed line (since this is a new file), it doesn't impact code quality or functionality. The abbreviation "opts" is commonly used and perfectly understandable. This feels like unnecessary nitpicking. The suggestion would make the documentation more consistent since "options" is used elsewhere in the comments and code. It could improve readability slightly. While consistency is good, this change is too minor to warrant a PR comment. "opts" is a well-understood abbreviation in programming. Delete this comment as it's an extremely minor stylistic suggestion that doesn't meaningfully improve code quality.
12. test/utils/apply-default.spec.ts:13
- Draft comment:
Typo in test description: The phrase "returns applies a deepMerge to each element in the array" is confusing. Consider rewording it to something like "applies a deepMerge to each element in the array". - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% While the comment is technically correct about the grammar, test descriptions are primarily for developer readability and the meaning is still clear despite the typo. The rules state not to make purely informative comments or comments about obvious/unimportant issues. This seems like a very minor issue that doesn't impact code functionality or maintainability. The grammar error could make the test slightly harder to read for future developers. Poor test descriptions can sometimes lead to confusion about test intent. While clear test descriptions are important, this particular grammar issue is minor enough that most developers would still easily understand the test's purpose. The meaning remains clear despite the awkward wording. This comment should be deleted as it addresses a very minor grammatical issue that doesn't significantly impact code quality or understanding.
Workflow ID: wflow_Buj33ls6ObrhoOz0
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
close #361
Important
Refactor to drop
@typescript-eslint/utilson production, add utilities for deep merging and rule creation, and update tests and dependencies.@typescript-eslint/utilswith@typescript-eslint/typesindynamic-import-chunkname.ts,export.ts,extensions.ts,no-unused-modules.ts, andget-value.ts.package.json, making@typescript-eslint/utilsoptional.applyDefaultinapply-default.tsfor merging user options with defaults.deepMergeindeep-merge.tsfor deep merging objects.RuleCreatorincreate-rule.tsfor creating rules with default options and docs URLs.applyDefault,deepMerge, andRuleCreatorinapply-default.spec.ts,deep-merge.spec.ts, andcreate-rule.spec.ts.This description was created by
for cfb000d. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
New Features
Refactor
Tests
Chores