Skip to content

feat: add option followTsOrganizeImports for order rule #287

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

Draft
wants to merge 22 commits into
base: master
Choose a base branch
from

Conversation

Shinigami92
Copy link
Member

@Shinigami92 Shinigami92 commented Apr 10, 2025

closes #286

Summary by CodeRabbit

  • New Features
    • Introduced a new option followTsOrganizeImports for the import ordering rule, enhancing control over import statement organization.
    • Added a new usable group private for import classification.
    • Expanded the import classification system to recognize 'private' as a valid import type.
  • Tests
    • Added new test cases to validate the import ordering behavior, ensuring compliance with TypeScript's expectations.
  • Documentation
    • Updated documentation to include the new followTsOrganizeImports option, detailing its behavior and implications.

Important

Add followTsOrganizeImports option and private import type to order rule, updating documentation and tests.

  • New Features:
    • Add followTsOrganizeImports option to order rule in order.ts, aligning import order with TypeScript's LSP Organize Imports.
    • Introduce private import type for imports starting with # in import-type.ts.
  • Documentation:
    • Update order.md to include followTsOrganizeImports and private import type.
  • Tests:
    • Add test cases in order.spec.ts to validate followTsOrganizeImports and private import type behavior.

This description was created by Ellipsis for 923ffb4. It will automatically update as commits are pushed.

Copy link

changeset-bot bot commented Apr 10, 2025

🦋 Changeset detected

Latest commit: ab4ec1d

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
eslint-plugin-import-x Minor

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

Copy link

coderabbitai bot commented Apr 10, 2025

"""

Walkthrough

This pull request introduces enhancements to the import ordering system by adding a new option called followTsOrganizeImports. This option allows for conditional handling of import types, specifically recognizing new types such as 'private'. The changes include updates to the Options interface, modifications to the typeTest function to identify private imports, and new test cases to validate the expected behavior according to TypeScript's import ordering standards. Documentation and changelog entries were also updated to reflect the new feature.

Changes

Files Change Summary
src/rules/order.ts, src/utils/import-type.ts Introduced followTsOrganizeImports option, updated types to include 'private', and added isPrivate function to enhance import type detection.
test/rules/order.spec.ts Added valid and invalid test cases to verify the import ordering behavior with the new followTsOrganizeImports option.
.changeset/clear-bikes-jog.md, .changeset/proud-comics-trade.md Added changelog entries indicating the new feature and the addition of the private group for the order rule.
docs/rules/order.md Updated documentation to include the new followTsOrganizeImports option and its implications for import ordering.
test/fixtures/typescript-order-custom-paths-mapping/tsconfig-with-path-mapping.json Added a TypeScript config file with custom path mappings including a #private alias for testing import resolution.

Assessment against linked issues

Objective Addressed Explanation
Support '#' prefixed imports as a 'private' type in import ordering (#286)

Poem

Hop, hop, I scurry through the code,
A new import type in every node,
With options to guide my playful trail,
Ensuring the order will never fail,
In the land of imports, I dance and play! 🐇✨
"""

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

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

test/rules/order.spec.ts

Oops! Something went wrong! :(

ESLint: 9.24.0

Error [ERR_MODULE_NOT_FOUND]: Cannot find module '/lib/index.js' imported from /eslint.config.js
at finalizeResolution (node:internal/modules/esm/resolve:257:11)
at moduleResolve (node:internal/modules/esm/resolve:914:10)
at defaultResolve (node:internal/modules/esm/resolve:1038:11)
at ModuleLoader.defaultResolve (node:internal/modules/esm/loader:557:12)
at ModuleLoader.resolve (node:internal/modules/esm/loader:525:25)
at ModuleLoader.getModuleJob (node:internal/modules/esm/loader:246:38)
at ModuleJob._link (node:internal/modules/esm/module_job:126:49)
(node:18067) ExperimentalWarning: Importing JSON modules is an experimental feature and might change at any time
(Use node --trace-warnings ... to show where the warning was created)

Tip

⚡💬 Agentic Chat (Pro Plan, General Availability)
  • We're introducing multi-step agentic chat in review comments and issue comments, within and outside of PR's. This feature enhances review and issue discussions with the CodeRabbit agentic chat by enabling advanced interactions, including the ability to create pull requests directly from comments and add commits to existing pull requests.

📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e9d728b and ab4ec1d.

📒 Files selected for processing (1)
  • test/rules/order.spec.ts (2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
test/rules/order.spec.ts (1)
test/utils.ts (1)
  • testFilePath (39-41)
🪛 GitHub Actions: CI
test/rules/order.spec.ts

[error] 1-2: ESLint rule '@rule-tester/order' violations: 'node:fs import should occur after import of glob' at line 1, 'path import should occur after import of glob' at line 2.

⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (7)
test/rules/order.spec.ts (7)

4812-4831: Good implementation of the new followTsOrganizeImports test case

This test case correctly verifies that when followTsOrganizeImports: true is set, imports are ordered according to TypeScript's Language Server Protocol (LSP) Organize Imports feature.

The order being tested:

  1. Private imports (#)
  2. Scoped package imports (@)
  3. Regular external imports
  4. Side effect imports
  5. Node.js built-in modules
  6. Local imports

4832-4851: LGTM - Properly tests explicit followTsOrganizeImports: false setting

The test correctly validates that when the option is explicitly set to false, the rule follows the default ESLint import order instead of TypeScript's organize imports order.


4852-4872: LGTM - Validates that manual groups take precedence over followTsOrganizeImports

Good test case showing that when a custom groups configuration is provided, it takes precedence over the followTsOrganizeImports option, even when set to true.


4873-4934: LGTM - Comprehensive test for TypeScript path mappings with both option values

These test cases properly cover:

  1. How the rule handles imports with TypeScript path mappings when followTsOrganizeImports: true
  2. The expected behavior when followTsOrganizeImports: false
  3. The correct configuration for TypeScript project path resolution

4935-4956: LGTM - Tests Node.js builtin module handling with node: prefix

Good test case showing that the rule correctly handles the special case of Node.js builtin modules with the node: prefix when followTsOrganizeImports: true.


5341-5363: Fixed test case aligns with TypeScript Organize Imports behavior

This test case correctly validates that the rule flags an error when imports don't follow TypeScript's ordering, specifically when a private import (#) appears after a scoped import (@).

The expected output shows the correct order, with private imports first, followed by scoped imports, then builtin modules, and finally local imports.


4958-4958: Todo item addressed: invalid test cases added

The todo comment requesting invalid test cases has been addressed with the addition of test case starting at line 5341, which properly tests the error condition when imports don't follow the TypeScript ordering.

✨ Finishing Touches
  • 📝 Generate Docstrings

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

codesandbox-ci bot commented Apr 10, 2025

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.

@Shinigami92 Shinigami92 force-pushed the fix-ts-organize-imports-mismatch-for-hashtag-imports branch from 0c288ff to bf0887e Compare April 10, 2025 15:58
@Shinigami92
Copy link
Member Author

@coderabbitai review

Copy link

coderabbitai bot commented Apr 10, 2025

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@Shinigami92 Shinigami92 force-pushed the fix-ts-organize-imports-mismatch-for-hashtag-imports branch from bf0887e to e64136f Compare April 10, 2025 16:13
@Shinigami92
Copy link
Member Author

@JounQin could you please do a first human-review / sanity check and run the GitHub actions workflow?
I would like to get feedback if this is a breaking change, due to it could differ in formatting many files of a project.
Also I'm not sure if 'mapped' is a good term or if we should be more specific here.
And lastly, I'm not yet fully sure if the current implementation is already sufficient (as all tests seems to already pass 👀) or if I miss something and we should add more tests.

PS: the contribution PR flow is 🚀🤯 (coderabbitai and changeset-bot)

Copy link

pkg-pr-new bot commented Apr 11, 2025

Open in StackBlitz

npm i https://pkg.pr.new/eslint-plugin-import-x@287

commit: ab4ec1d

Copy link

codecov bot commented Apr 11, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.00%. Comparing base (95dd356) to head (e9d728b).

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #287   +/-   ##
=======================================
  Coverage   95.99%   96.00%           
=======================================
  Files          91       91           
  Lines        4744     4752    +8     
  Branches     1763     1767    +4     
=======================================
+ Hits         4554     4562    +8     
  Misses        189      189           
  Partials        1        1           

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@JounQin JounQin marked this pull request as ready for review April 11, 2025 08:10
@JounQin JounQin marked this pull request as draft April 11, 2025 08:14
@JounQin
Copy link
Member

JounQin commented Apr 11, 2025

Accidentally marked as ready for review. 😅

@Shinigami92 Shinigami92 changed the title fix: ts organize imports mismatch for hashtag imports feat: add option followTsOrganizeImports for order rule Apr 11, 2025
@Shinigami92 Shinigami92 force-pushed the fix-ts-organize-imports-mismatch-for-hashtag-imports branch from 53106d8 to 2790318 Compare April 14, 2025 06:22
@Shinigami92
Copy link
Member Author

@JounQin is there anything that is missing that needs to be done before we can merge, except of finding good names?
Should I add some further test cases?
Should I add some more documentation?
Or is everything fine and just wait?

@Shinigami92 Shinigami92 marked this pull request as ready for review April 14, 2025 06:24
Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Reviewed everything up to 2790318 in 1 minute and 34 seconds

More details
  • Looked at 200 lines of code in 5 files
  • Skipped 0 files when reviewing.
  • Skipped posting 11 drafted comments based on config settings.
1. test/rules/order.spec.ts:5212
  • Draft comment:
    The new option followTsOrganizeImports is being tested and appears to work as intended. The test case here clearly contrasts the expected order when this option is enabled.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
    This comment is purely informative, as it only states that a new option is being tested and works as intended. It doesn't provide any actionable feedback or suggestions for improvement.
2. test/rules/order.spec.ts:5410
  • Draft comment:
    The test suite is very comprehensive. Consider adding inline documentation to explain the intent behind some of the more complex grouping and ordering test cases to aid future maintainers.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. test/rules/order.spec.ts:5200
  • Draft comment:
    The error messages defined with createOrderError are consistent. Ensure that documentation (if not already updated) reflects the behavior of followTsOrganizeImports and any implications on manual group definitions.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. test/rules/order.spec.ts:1
  • Draft comment:
    This test file is extremely comprehensive, covering a wide range of ordering scenarios (import vs require, newlines, comments, whitespace, and even invalid configurations). The extensive coverage is impressive but its size may impact maintainability.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None
5. test/rules/order.spec.ts:4812
  • Draft comment:
    New test cases for the followTsOrganizeImports option correctly simulate TypeScript’s LSP organize imports. Ensure that future updates continue to conform with TS LSP behavior.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None
6. test/rules/order.spec.ts:1426
  • Draft comment:
    The tests that handle multiline comments and Windows-style newlines (e.g. around lines 1426–1436) are very valuable. Adding inline comments explaining the intent in such complex blocks would improve maintainability.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None
7. test/rules/order.spec.ts:1588
  • Draft comment:
    Several tests provide the expected output as an array (supporting multiple valid fix outputs). Consider documenting the precedence rules if multiple correct outputs are possible to ensure deterministic behavior across environments.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None
8. test/rules/order.spec.ts:32
  • Draft comment:
    The use of helper functions like createOrderError and createGenericError is consistent and aids clarity. Make sure that any future changes to error message text remain synchronized with these tests.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None
9. test/rules/order.spec.ts:500
  • Draft comment:
    Given the substantial size and complexity of this test file, consider refactoring or splitting the tests into multiple modules (e.g. separating valid and invalid cases) for improved readability and future maintenance.
  • Reason this comment was not posted:
    Confidence changes required: 50% <= threshold 50%
    None
10. test/rules/order.spec.ts:5243
  • Draft comment:
    The tests that run under different parser configurations (including TypeScript and Flow) confirm robust integration. This is particularly important for validating the new followTsOrganizeImports option. Consistency across these setups is well maintained.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None
11. src/utils/import-type.ts:57
  • Draft comment:
    Typo Alert: In the isExternalModuleMain function (lines 57-61), the error message mentions 'name, path, and context' as required parameters, but the parameter is actually named 'modulePath'. Consider updating the error string to 'name, modulePath, and context are all required' for clarity.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_qiWlP2CypJt5qQzq


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link

@coderabbitai coderabbitai bot left a 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

♻️ Duplicate comments (2)
src/rules/order.ts (2)

836-836: 🛠️ Refactor suggestion

Update type name to be consistent with other type names.

The type name 'private' should be renamed to 'private-import' to maintain consistency with other singular type names in the array.

-  'private',
+  'private-import',

1407-1414: 🛠️ Refactor suggestion

Use ESLint reporting instead of debug logs for configuration warnings.

Currently, you're using the debug logger to warn about incompatible configurations, which most users won't see. A more user-friendly approach would be to use ESLint's reporting mechanism.

Based on a previous discussion, consider using the existing log function for now, but the message could be more explicit:

      if (options.followTsOrganizeImports && options.groups) {
        // TODO: remove warning when default of followTsOrganizeImports switched to true
        // because then the user potentially knows what they are doing
        // and the warning is not needed anymore
        log(
-         '`followTsOrganizeImports: true` has no effect when custom `groups` are defined.',
+         'When `followTsOrganizeImports: true` is used with custom `groups`, the TypeScript ordering is ignored in favor of your custom groups.',
        )
      }
🧹 Nitpick comments (1)
src/rules/order.ts (1)

56-63: The new import order differs from existing defaults.

The new defaultGroupsTsOrganizeImports constant defines an order that's significantly different from the default groups. This reordering appears to be intentional to match TypeScript's LSP Organize Imports behavior, but could use a code comment explaining the specific ordering rationale.

const defaultGroupsTsOrganizeImports = [
  'private',
  'external',
  'builtin',
  'parent',
  'index',
  'sibling',
+  // Order follows TypeScript's LSP Organize Imports behavior:
+  // 1. Private imports (starting with #)
+  // 2. External dependencies
+  // 3. Built-in Node.js modules
+  // 4. Parent/ancestor imports
+  // 5. Index imports
+  // 6. Sibling imports
] as const
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 708b5cc and 6a1397f.

📒 Files selected for processing (2)
  • .changeset/proud-comics-trade.md (1 hunks)
  • src/rules/order.ts (5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (21)
  • GitHub Check: Lint and Test with Node.js 22 and ESLint 9 on windows-latest
  • GitHub Check: Lint and Test with Node.js 20 and ESLint 9 on windows-latest
  • GitHub Check: Lint and Test with Node.js 22 and ESLint 8 on windows-latest
  • GitHub Check: Lint and Test with Node.js 18 and ESLint 9 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 22 and ESLint 8.56 on windows-latest
  • GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on windows-latest
  • GitHub Check: Lint and Test with Node.js 20 and ESLint 8 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 18 and ESLint 8 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 22 and ESLint 9 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 18 and ESLint 8.56 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 20 and ESLint 8 on macos-latest
  • GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on macos-latest
  • GitHub Check: Lint and Test with Node.js 20 and ESLint 8.56 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 22 and ESLint 8.56 on macos-latest
  • GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (4)
.changeset/proud-comics-trade.md (1)

1-6: LGTM: Clear and concise changelog entry for the feature.

The entry correctly documents the new usable group private for the order rule in a clear and concise manner.

src/rules/order.ts (3)

1162-1162: LGTM: Adding new option for TypeScript import organization.

The new followTsOrganizeImports option is well-named and properly added to the Options interface, with a good default value and a TODO comment about future default changes.


1285-1289: Clear schema definition for the new option.

The schema definition for followTsOrganizeImports is well-structured and includes appropriate type information and default value. The comment about changing the default in the next major version is helpful.


1415-1420: LGTM: Good conditional selection of import groups.

The code properly selects between the default groups and the new TypeScript-compatible groups based on the followTsOrganizeImports option. This implementation ensures backward compatibility while providing the new functionality.

@Shinigami92 Shinigami92 requested a review from JounQin April 14, 2025 10:18
@Shinigami92 Shinigami92 force-pushed the fix-ts-organize-imports-mismatch-for-hashtag-imports branch from bf9db5a to 213eb21 Compare April 14, 2025 11:00
@Shinigami92 Shinigami92 marked this pull request as draft April 14, 2025 12:05
@Shinigami92 Shinigami92 marked this pull request as ready for review April 14, 2025 12:50
@Shinigami92 Shinigami92 requested a review from JounQin April 14, 2025 12:50
Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Reviewed everything up to 923ffb4 in 3 minutes and 16 seconds

More details
  • Looked at 293 lines of code in 6 files
  • Skipped 0 files when reviewing.
  • Skipped posting 11 drafted comments based on config settings.
1. test/rules/order.spec.ts:4813
  • Draft comment:
    Excellent test coverage for the new followTsOrganizeImports option. The tests correctly verify both the enabled and disabled behavior, as well as that manual groups override the followTsOrganizeImports setting.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None
2. test/rules/order.spec.ts:4831
  • Draft comment:
    Good: The explicit test for followTsOrganizeImports=false confirms expected ordering without TS LSP semantics.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None
3. test/rules/order.spec.ts:4850
  • Draft comment:
    Nice: The manual groups test shows that custom group settings override followTsOrganizeImports as required.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None
4. test/rules/order.spec.ts:1
  • Draft comment:
    The test file is extensive and covers many scenarios; consider using a dedent helper for multi-line template literals to improve readability.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None
5. test/rules/order.spec.ts:4813
  • Draft comment:
    The new valid test case using { followTsOrganizeImports: true } clearly illustrates that '#'‐prefixed imports are shifted to the top per TS LSP organize imports. This test is clear and covers the expected behavior.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
    This comment is purely informative, as it only praises the addition of a test case without providing any actionable feedback or suggestions for improvement. It does not align with the rules for good comments, which should offer specific suggestions or highlight potential issues.
6. test/rules/order.spec.ts:4831
  • Draft comment:
    The test with explicit followTsOrganizeImports set to false correctly confirms that the default ordering is preserved when this option is disabled.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
    This comment is purely informative, as it simply states that a test is working as expected. It does not provide any actionable feedback or suggestions for improvement.
7. test/rules/order.spec.ts:4850
  • Draft comment:
    Good test coverage: the manual groups test confirms that explicitly set groups take precedence over the followTsOrganizeImports option. This reinforces correct prioritization.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
    This comment is purely informative, as it praises the test coverage without providing any actionable feedback or suggestions for improvement. It does not align with the rules for acceptable comments.
8. test/rules/order.spec.ts:5254
  • Draft comment:
    The invalid test case demonstrating a reordering error under followTsOrganizeImports: true is well constructed. It shows that '#'‐prefixed imports must be ordered before other groups as expected by TS’s organize imports behavior.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
    This comment is purely informative, praising the construction of a test case. It does not provide any actionable feedback or suggestions for improvement.
9. test/rules/order.spec.ts:5270
  • Draft comment:
    Overall, the test suite now covers multiple combinations for the followTsOrganizeImports option. Consider adding a brief test to check the default behavior when the option is omitted.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
    While having test coverage for omitted options can be valuable, in this case:
  1. The default behavior is already implicitly tested by the false case
  2. The comment is asking for verification rather than pointing out a clear issue
  3. The existing tests cover both true and false cases which effectively tests the behavior boundaries
    The comment does identify a gap in explicit test coverage. Having a test for the omitted case could make the behavior more obvious to readers.
    However, this is a "nice to have" rather than a critical issue. The behavior is already well tested through the existing cases. The comment is more of a suggestion than identifying a real problem.
    The comment should be deleted as it's asking for optional additional test coverage rather than identifying a clear issue that needs to be fixed.
10. docs/rules/order.md:169
  • Draft comment:
    Typo: Consider changing "If the import's specifier is starting with #" to "If the import's specifier starts with #" for improved clarity and grammatical correctness.
  • 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%
    Looking at the context, this is a documentation file explaining import ordering rules. The suggested change would improve readability and maintain consistency with other similar statements in the numbered list. However, the current wording is still understandable and the change is very minor. The rules say not to make purely informative comments or obvious changes.
    The change would improve the documentation quality, but it's a very minor grammatical fix that doesn't affect functionality. The current wording is still clear enough to understand.
    While the suggestion would improve the text, it's too minor of an issue to warrant a PR comment. Documentation polish can be handled in dedicated documentation review passes.
    Delete this comment as it suggests a very minor grammatical improvement that doesn't materially affect understanding of the documentation.
11. docs/rules/order.md:67
  • Draft comment:
    Typographical error: There is an extraneous diacritical mark (̀) before babel-register. Please remove it so it reads simply as "Allowed as babel-register is not assigned."
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_fe00QHHiUAX9pGFq


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link

@coderabbitai coderabbitai bot left a 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

♻️ Duplicate comments (1)
src/rules/order.ts (1)

836-836: 🛠️ Refactor suggestion

Update type name to be consistent with other type names

The type name 'private' should be renamed to 'private-import' to maintain consistency with other singular type names in the array.

-  'private',
+  'private-import',

This change would align with the consensus from previous discussions in PR comments.

🧹 Nitpick comments (1)
src/rules/order.ts (1)

1408-1411: Conditional selection of import groups is implemented correctly.

The logic to conditionally use defaultGroupsTsOrganizeImports instead of defaultGroups when followTsOrganizeImports is true is well implemented.

However, based on the PR comments, there was a discussion about warning users when they define their own groups along with followTsOrganizeImports: true. Consider implementing this warning mechanism to improve user experience.

const { pathGroups, maxPosition } = convertPathGroupsForRanks(
  options.pathGroups || [],
)
+if (options.followTsOrganizeImports && options.groups) {
+  log(
+    'If you have defined your own `groups` in `order`, `followTsOrganizeImports: true` has no effect.',
+  )
+}
const { groups, omittedTypes } = convertGroupsToRanks(
  options.groups ||
    (options.followTsOrganizeImports
      ? defaultGroupsTsOrganizeImports
      : defaultGroups),
)
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 213eb21 and 923ffb4.

📒 Files selected for processing (1)
  • src/rules/order.ts (5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (3)
src/rules/order.ts (3)

56-63: The default TypeScript organize imports order looks good.

This constant defines the order of imports to match TypeScript's LSP Organize Imports behavior, correctly positioning 'private' imports first, followed by external, builtin, and internal imports in the expected order.


1162-1162: Interface addition looks good.

The new followTsOrganizeImports option is correctly added to the Options interface as an optional boolean property.


1285-1289: Schema definition is well structured.

The schema validation for the new option is correctly added with appropriate type and default value. The TODO comment about changing the default in the next major version is a good practice for maintaining backward compatibility.

@Shinigami92 Shinigami92 requested a review from JounQin April 15, 2025 09:30
@Shinigami92 Shinigami92 requested a review from JounQin April 16, 2025 06:53
Comment on lines +56 to +63
const defaultGroupsTsOrganizeImports = [
'private',
'external',
'builtin',
'parent',
'index',
'sibling',
] as const
Copy link

Choose a reason for hiding this comment

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

The order in defaultGroupsTsOrganizeImports appears to be inconsistent with TypeScript's LSP Organize Imports behavior. Based on the tests and documentation, TypeScript organizes imports with 'builtin' modules before 'external' modules, but this implementation has 'external' before 'builtin'. Consider swapping these two entries to match TypeScript's actual ordering behavior.

Suggested change
const defaultGroupsTsOrganizeImports = [
'private',
'external',
'builtin',
'parent',
'index',
'sibling',
] as const
const defaultGroupsTsOrganizeImports = [
'private',
'builtin',
'external',
'parent',
'index',
'sibling',
] as const

Spotted by Diamond

Is this helpful? React 👍 or 👎 to let us know.

Copy link
Member

Choose a reason for hiding this comment

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

@Shinigami92 Can you double check?

Copy link
Member

Choose a reason for hiding this comment

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

I also think builtin should be front of external.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll check, but my gut feeling tells me either we missed a import-statement case, or it works correctly right now already and the AI is hallucinating

Copy link
Member Author

Choose a reason for hiding this comment

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

AH! so an external import is something like e.g. glob 👍
Will add to test

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you want to get a test case for separated node:* with followTsOrganizeImports: true?

Copy link
Member

Choose a reason for hiding this comment

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

I mean, you can still have all your node:* imports at the top if you but it in a separate group (split them with a newline from the other imports)

Wouldn't the group order be different?

Do you want to get a test case for separated node:* with followTsOrganizeImports: true?

Thta's would be appreciated.

Copy link
Member Author

Choose a reason for hiding this comment

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

Wouldn't the group order be different?

No, why should it?

That's would be appreciated.

Added ab4ec1d (#287)

You should take a case like I added here, and copy-paste it into your VSCode and then hit CTRL+SHIFT+P and run organize imports, then you can experiment yourself a bit with it 🙂

Copy link
Member

Choose a reason for hiding this comment

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

Finally we got failing test cases, I don't know whether it's expected to you.

Copy link
Member

@JounQin JounQin Apr 16, 2025

Choose a reason for hiding this comment

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

No, why should it?

Didn't the default order external should be listed in front of builtin as you set? Did I miss anything?

You should take a case like I added here, and copy-paste it into your VSCode and then hit CTRL+SHIFT+P and run organize imports, then you can experiment yourself a bit with it 🙂

I also use this command a lot when I develop projects not using this plugin nor simple-sort-imports, etc, for example: https://github.com/backstage/backstage.

Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
7.5% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

Comment on lines +4935 to +4956
// test followTsOrganizeImports: true and splitted node:* group
tValid({
code: `import fs from 'node:fs';
import path from "path";

import { internA } from "#a";
import { privateA } from "#private/a";
import { scopeA } from "@a/a";
import a from 'a';
import 'format.css';
import { glob } from 'glob';
import index from './';
import { localA } from "./a";
import sibling from './foo';
`,
...parserConfig,
options: [
{
followTsOrganizeImports: true,
},
],
}),
Copy link
Member Author

Choose a reason for hiding this comment

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

todo: test case is failing 👀
I will investigate

Copy link
Member

Choose a reason for hiding this comment

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

Finally we got the failing test case. 🤣

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, it looks like the code is not prepared to allow groups separated from each other without affecting each other. Due to I spend now a lot of time into this, I think I need to first dig deeper into the code and e.g. prepare myself with the other mentioned PR to add JSDocs, because right now it is really hard to understand whats going on and you need to often switch between docs and code instead of just reading from the code.

My personal case would be fixed/satisfied by the current implementation, because I never separate imports into groups and just trust the TS LSP by doing its thing. But I came in conflict with the 'import-x/order': 'error' rule, because by default the #-imports conflicts with the LSP in the default case. So I come constantly in conflict in a project with a coworker right now, because he wants to use the eslint rule default and I want to use the TS LSP default..., and IMO at least the both in combination should not at least conflict by default.

But yes, you are right in the sense of if now someone else would group their imports into 'node:*' having at the top, the LSP would not trigger, but the eslint rule would now trigger. 🫠 But was it like this also before?

If I overlook something right now, please hint me in the right direction what I need to touch to allow separate groups without affecting each other.

Otherwise, see you after 28th April 🚗-🇮🇸-😎

Copy link
Member

@JounQin JounQin Apr 16, 2025

Choose a reason for hiding this comment

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

But was it like this also before?

It could be, node: is only supported in a short time at import-js#3173 which we haven't adopted into this fork yet, so testing for it is lacking right now.

In my experience, TS LSP only sort imports in same group, what means if there is no lines between imports then they will be sorted, when there are new lines between imports, they're considered different groups by TS LSP and will not be sorted, so I think private is not a group now, it should be ranked if only in same group which is here

return function importsSorter(nodeA: ImportEntry, nodeB: ImportEntry) {
const importA = getNormalizedValue(
nodeA,
alphabetizeOptions.caseInsensitive,
)
const importB = getNormalizedValue(
nodeB,
alphabetizeOptions.caseInsensitive,
)
let result = 0
if (!importA.includes('/') && !importB.includes('/')) {
result = compareString(importA, importB)
} else {
const A = importA.split('/')
const B = importB.split('/')
const a = A.length
const b = B.length
for (let i = 0; i < Math.min(a, b); i++) {
// Skip comparing the first path segment, if they are relative segments for both imports
const x = A[i]
const y = B[i]
if (i === 0 && RELATIVE_DOTS.has(x) && RELATIVE_DOTS.has(y)) {
// If one is sibling and the other parent import, no need to compare at all, since the paths belong in different groups
if (x !== y) {
break
}
continue
}
result = compareString(x, y)
if (result) {
break
}
}
if (!result && a !== b) {
result = a < b ? -1 : 1
}
}
result = result * multiplier
// In case the paths are equal (result === 0), sort them by importKind
if (!result && multiplierImportKind) {
result =
multiplierImportKind *
compareString(
nodeA.node.importKind || DEFAULT_IMPORT_KIND,
nodeB.node.importKind || DEFAULT_IMPORT_KIND,
)
}
return result
}
}

Otherwise, see you after 28th April 🚗-🇮🇸-😎

Feel free to take a break, have a good time!

@Shinigami92 Shinigami92 marked this pull request as draft April 16, 2025 10:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

import-x/order mismatches typescript language server organize imports for # prefixed imports
3 participants