eslint-plugin-import-x icon indicating copy to clipboard operation
eslint-plugin-import-x copied to clipboard

feat: add option `followTsOrganizeImports` for `order` rule

Open Shinigami92 opened this issue 8 months ago • 21 comments

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 923ffb49d223be002b0c51f10534e74b9db7a31b. It will automatically update as commits are pushed.

Shinigami92 avatar Apr 10 '25 14:04 Shinigami92

🦋 Changeset detected

Latest commit: ab4ec1de9979da6568c2c025dcd393123597e5cd

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

changeset-bot[bot] avatar Apr 10 '25 14:04 changeset-bot[bot]

"""

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 e9d728b3aa95c29321f2ffe5b9a395550499dece and ab4ec1de9979da6568c2c025dcd393123597e5cd.

📒 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.

coderabbitai[bot] avatar Apr 10 '25 14:04 coderabbitai[bot]

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.

codesandbox-ci[bot] avatar Apr 10 '25 14:04 codesandbox-ci[bot]

@coderabbitai review

Shinigami92 avatar Apr 10 '25 16:04 Shinigami92

:white_check_mark: 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.

coderabbitai[bot] avatar Apr 10 '25 16:04 coderabbitai[bot]

@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)

Shinigami92 avatar Apr 10 '25 16:04 Shinigami92

Open in StackBlitz

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

commit: ab4ec1d

pkg-pr-new[bot] avatar Apr 11 '25 03:04 pkg-pr-new[bot]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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           

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

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

codecov[bot] avatar Apr 11 '25 03:04 codecov[bot]

Accidentally marked as ready for review. 😅

JounQin avatar Apr 11 '25 08:04 JounQin

@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 avatar Apr 14 '25 06:04 Shinigami92

@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 I believe you missed this part:

We need to confirm whether it's a real internal-imports or just path aliasing via paths or another options.

Only checking starts with # is not enough, we could have tsconfig.json with paths mapping "#/*": ["./xxx/*"], etc.

We'll need to read the nearest package.json, and check it's imports field instead.

JounQin avatar Apr 14 '25 06:04 JounQin

@Shinigami92 I believe you missed this part:

We need to confirm whether it's a real internal-imports or just path aliasing via paths or another options.

Only checking starts with # is not enough, we could have tsconfig.json with paths mapping "#/*": ["./xxx/*"], etc.

We'll need to read the nearest package.json, and check it's imports field instead.

Actually I think we don't need to check for something like this, because either way I use package.json imports, tsconfig.json paths or non of these: the TypeScript Organize Imports behaves the same, always. 👀

Could you bring up an example where you think it can conflict somehow? (I mean: when is it important to read a tsconfig.json or package.json?)

Shinigami92 avatar Apr 14 '25 07:04 Shinigami92

Actually I think we don't need to check for something like this, because either way I use package.json imports, tsconfig.json paths or non of these: the TypeScript Organize Imports behaves the same, always. 👀

Then the group name is wrong? 🤣 It's not true private-import but maybe-private-import? Or it should be documented. Do you think private would be enough?

JounQin avatar Apr 14 '25 07:04 JounQin

Actually I think we don't need to check for something like this, because either way I use package.json imports, tsconfig.json paths or non of these: the TypeScript Organize Imports behaves the same, always. 👀

Then the group name is wrong? 🤣 It's not true private-import but maybe-private-import? Or it should be documented. Do you think private would be enough?

Yeah, seems we are back to the naming 😆 Maybe this was one of the things why I tempted to mapped at the start. To me this is at the end just a string in the codebase which could potentially used by 0.001% of users if they for any reason want to configure manually groupings. Otherwise it is just a separator of different groups by rank and thats it. The algorithm does not care how it is named.

Shinigami92 avatar Apr 14 '25 07:04 Shinigami92

Naming is hard...

@Shinigami92 Let's move to private and keep followTsOrganizeImports, also let's add some other test cases by using custom groups with private enabled and followTsOrganizeImports enabled/disabled.

Should we emit a warning if the users define their own groups and followTsOrganizeImports: true which does not take any effect actually?

JounQin avatar Apr 14 '25 07:04 JounQin

Naming is hard...

@Shinigami92 Let's move to private and keep followTsOrganizeImports, also let's add some other test cases by using custom groups with private enabled and followTsOrganizeImports enabled/disabled.

Okay, will do so 👍

Should we emit a warning if the users define their own groups and followTsOrganizeImports: true which does not take any effect actually?

Yes, I think this could make sense 👍

Shinigami92 avatar Apr 14 '25 08:04 Shinigami92

@coderabbitai review

Shinigami92 avatar Apr 14 '25 09:04 Shinigami92

:white_check_mark: 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.

coderabbitai[bot] avatar Apr 14 '25 09:04 coderabbitai[bot]

@JounQin performed the requested actions (rename, added tests and warning) let me know if anything further is missing and if the warning is correctly implemented or if there is another way of reporting warnings to the user.

Feedback for the PR process: the sonarqubecloud's Quality Gate failed are currently a bit more confusing then helping, and I just assume that the report is so low/bad, because of "duplicated" code in the tests. But TBH this is a wanted thing and not really duplicated code in terms of DRY. Maybe treat the quality gate for the test folder differently. 🤷

Shinigami92 avatar Apr 14 '25 09:04 Shinigami92

Feedback for the PR process: the sonarqubecloud's Quality Gate failed are currently a bit more confusing then helping, and I just assume that the report is so low/bad, because of "duplicated" code in the tests. But TBH this is a wanted thing and not really duplicated code in terms of DRY.

Nevermind, tools are just tools.

JounQin avatar Apr 14 '25 09:04 JounQin

Quality Gate Failed Quality Gate failed

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

See analysis details on SonarQube Cloud

sonarqubecloud[bot] avatar Apr 16 '25 10:04 sonarqubecloud[bot]

I simply do not use this rule in my projects anymore

Shinigami92 avatar Jun 09 '25 10:06 Shinigami92