fix: consistent-type-specifier-style with prefer-inline and TS resolution-mode import attributes#443
Conversation
🦋 Changeset detectedLatest commit: 5df513b 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 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughSkip processing ImportDeclarations that include a TypeScript Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
|
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 126a59c in 31 seconds. Click for details.
- Reviewed
53lines of code in2files - Skipped
0files when reviewing. - Skipped posting
3draft 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/consistent-type-specifier-style.ts:47
- Draft comment:
The newly added function 'hasResolutionModeAttribute' cleanly checks for a resolution-mode attribute using Array.some. Optionally, you could use optional chaining (e.g. node.attributes?.some(...)) for brevity, but this is acceptable. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. src/rules/consistent-type-specifier-style.ts:95
- Draft comment:
The added early return in the prefer-inline branch (using hasResolutionModeAttribute) correctly bypasses fixing resolution-mode type imports, which is essential for TS compliance. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
3. test/rules/consistent-type-specifier-style.spec.ts:225
- Draft comment:
The test cases for resolution-mode attributes are placed in the TS_ONLY array, which is appropriate since this is a TypeScript-specific feature. This ensures the behavior is only tested when TS is in use. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
Workflow ID: wflow_kbOuSYtLnviCEPZW
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 (1)
src/rules/consistent-type-specifier-style.ts (1)
47-56: Consider handling Identifier keys in addition to Literal keys.The current implementation only checks for
Literalkeys, but ECMAScript import attributes allow both string literals and identifiers as keys. While TypeScript documentation examples use quoted'resolution-mode', a defensive check for both forms would be more robust.🔎 Suggested enhancement to handle both key types
function hasResolutionModeAttribute(node: TSESTree.ImportDeclaration) { return ( node.attributes && node.attributes.some( - attr => - attr.key.type === 'Literal' && attr.key.value === 'resolution-mode', + attr => { + if (attr.key.type === 'Literal') { + return attr.key.value === 'resolution-mode' + } + if (attr.key.type === 'Identifier') { + return attr.key.name === 'resolution-mode' + } + return false + } ) ) }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/rules/consistent-type-specifier-style.tstest/rules/consistent-type-specifier-style.spec.ts
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: JounQin
Repo: un-ts/eslint-plugin-import-x PR: 271
File: test/rules/no-unused-modules.spec.ts:1528-1532
Timestamp: 2025-03-30T09:06:59.006Z
Learning: The import from 'eslint8.56/use-at-your-own-risk' has incorrect TypeScript types but works correctly at runtime, which is properly handled with a `ts-expect-error` comment.
Learnt from: JounQin
Repo: un-ts/eslint-plugin-import-x PR: 378
File: src/rules/imports-first.ts:10-19
Timestamp: 2025-06-08T12:09:38.535Z
Learning: The current implementation in eslint-plugin-import-x uses the correct ESLint core DeprecatedInfo structure: deprecatedSince field and replacedBy array with objects containing rule properties that match the ExternalSpecifier type with name and url fields.
Learnt from: JounQin
Repo: un-ts/eslint-plugin-import-x PR: 378
File: src/rules/imports-first.ts:10-19
Timestamp: 2025-06-08T12:09:38.535Z
Learning: ESLint core ExternalSpecifier interface has optional name and url string properties. The current eslint-plugin-import-x implementation correctly uses this structure for the rule property within replacedBy arrays in DeprecatedInfo objects.
Learnt from: JounQin
Repo: un-ts/eslint-plugin-import-x PR: 386
File: src/rules/prefer-namespace-import.ts:41-46
Timestamp: 2025-06-18T15:22:38.532Z
Learning: In eslint-plugin-import-x, JounQin prefers to throw on invalid rule options rather than handling them gracefully with try/catch blocks and reporting configuration errors.
📚 Learning: 2025-05-31T03:11:08.864Z
Learnt from: JounQin
Repo: un-ts/eslint-plugin-import-x PR: 362
File: src/utils/import-declaration.ts:1-2
Timestamp: 2025-05-31T03:11:08.864Z
Learning: TypeScript type-only imports (using `import type`) are stripped during compilation and do not affect production dependencies or bundle size. Only runtime imports need to be considered when managing production dependencies in TypeScript projects.
Applied to files:
test/rules/consistent-type-specifier-style.spec.tssrc/rules/consistent-type-specifier-style.ts
📚 Learning: 2025-03-30T09:06:59.006Z
Learnt from: JounQin
Repo: un-ts/eslint-plugin-import-x PR: 271
File: test/rules/no-unused-modules.spec.ts:1528-1532
Timestamp: 2025-03-30T09:06:59.006Z
Learning: The import from 'eslint8.56/use-at-your-own-risk' has incorrect TypeScript types but works correctly at runtime, which is properly handled with a `ts-expect-error` comment.
Applied to files:
test/rules/consistent-type-specifier-style.spec.tssrc/rules/consistent-type-specifier-style.ts
📚 Learning: 2025-05-31T03:10:38.972Z
Learnt from: JounQin
Repo: un-ts/eslint-plugin-import-x PR: 362
File: src/utils/create-rule.ts:0-0
Timestamp: 2025-05-31T03:10:38.972Z
Learning: When `rewriteRelativeImportExtensions` is enabled in TypeScript configuration, using `.ts` extensions in import paths is correct and necessary for Node.js ESM compatibility. TypeScript will rewrite these to `.js` during compilation.
Applied to files:
test/rules/consistent-type-specifier-style.spec.tssrc/rules/consistent-type-specifier-style.ts
📚 Learning: 2025-06-08T12:09:38.535Z
Learnt from: JounQin
Repo: un-ts/eslint-plugin-import-x PR: 378
File: src/rules/imports-first.ts:10-19
Timestamp: 2025-06-08T12:09:38.535Z
Learning: ESLint core ExternalSpecifier interface has optional name and url string properties. The current eslint-plugin-import-x implementation correctly uses this structure for the rule property within replacedBy arrays in DeprecatedInfo objects.
Applied to files:
src/rules/consistent-type-specifier-style.ts
📚 Learning: 2025-06-08T12:09:38.535Z
Learnt from: JounQin
Repo: un-ts/eslint-plugin-import-x PR: 378
File: src/rules/imports-first.ts:10-19
Timestamp: 2025-06-08T12:09:38.535Z
Learning: The current implementation in eslint-plugin-import-x uses the correct ESLint core DeprecatedInfo structure: deprecatedSince field and replacedBy array with objects containing rule properties that match the ExternalSpecifier type with name and url fields.
Applied to files:
src/rules/consistent-type-specifier-style.ts
📚 Learning: 2025-03-30T14:44:11.779Z
Learnt from: SukkaW
Repo: un-ts/eslint-plugin-import-x PR: 272
File: src/utils/resolve.ts:0-0
Timestamp: 2025-03-30T14:44:11.779Z
Learning: In eslint-plugin-import-x's node resolver, the `modules` parameter of `createNodeResolver` function accepts both string and string array types, making it flexible when passing values like `moduleDirectory`.
Applied to files:
src/rules/consistent-type-specifier-style.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (2)
test/rules/consistent-type-specifier-style.spec.ts (1)
225-233: LGTM! Test coverage for resolution-mode exemption is appropriate.The test cases correctly validate that imports with
resolution-modeattributes are exempted under bothprefer-inlineandprefer-top-levelconfigurations. Placement in theTS_ONLYarray is correct since resolution-mode is TypeScript-specific syntax (introduced in TS 5.3).src/rules/consistent-type-specifier-style.ts (1)
98-100: LGTM! Guard correctly prevents invalid transformation.The early return when
resolution-modeis present prevents the rule from transformingimport type {...} with {'resolution-mode': ...}(valid) intoimport {type ...} with {...}(invalid TypeScript syntax). The placement after theimportKindcheck and before specifier validation is appropriate.Note: No equivalent guard is needed in the
prefer-top-levelbranch since it already returns early for top-level type imports at line 140.
commit: |
There was a problem hiding this comment.
Pull request overview
This PR fixes the consistent-type-specifier-style rule to properly handle TypeScript's resolution-mode import attributes introduced in TypeScript 5.3. The fix prevents the rule from incorrectly attempting to transform imports that include resolution-mode attributes, which are necessary for controlling module resolution behavior.
- Adds a helper function to detect
resolution-modeattributes in import declarations - Exempts imports with
resolution-modeattributes from theprefer-inlineenforcement - Adds test coverage for both
prefer-inlineandprefer-top-leveloptions with resolution-mode
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/rules/consistent-type-specifier-style.ts | Adds hasResolutionModeAttribute() helper function and integrates check into prefer-inline branch to skip transformation for imports with resolution-mode |
| test/rules/consistent-type-specifier-style.spec.ts | Adds test cases validating that top-level type imports with resolution-mode attributes are preserved under both configuration options |
| .changeset/salty-brooms-nail.md | Documents the patch-level change for the resolution-mode exception |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…S resolution-mode attribute
Signed-off-by: JounQin <admin@1stg.me>
Signed-off-by: JounQin <admin@1stg.me>
89e4366 to
5009ec6
Compare
consistent-type-specifier-style with prefer-inline and TS resolution-mode import attributesconsistent-type-specifier-style with prefer-inline and TS resolution-mode import attributes
fixes #233. not sure is it right way to place tests in TS_ONLY array as its TS only feature or in COMMON_TESTS?
Important
Add handling for TypeScript
resolution-modeattribute inconsistent-type-specifier-stylerule to prevent incorrect linting.hasResolutionModeAttribute()inconsistent-type-specifier-style.tsto check forresolution-modeattribute in import declarations.resolution-modeattribute is present.resolution-modeattribute inconsistent-type-specifier-style.spec.tsunderTS_ONLY.prefer-inlineandprefer-top-leveloptions are tested withresolution-mode.This description was created by
for 126a59c. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
Bug Fixes
Tests
Chores