feat: add option `followTsOrganizeImports` for `order` rule
closes #286
Summary by CodeRabbit
- New Features
- Introduced a new option
followTsOrganizeImportsfor the import ordering rule, enhancing control over import statement organization. - Added a new usable group
privatefor import classification. - Expanded the import classification system to recognize 'private' as a valid import type.
- Introduced a new option
- Tests
- Added new test cases to validate the import ordering behavior, ensuring compliance with TypeScript's expectations.
- Documentation
- Updated documentation to include the new
followTsOrganizeImportsoption, detailing its behavior and implications.
- Updated documentation to include the new
[!IMPORTANT] Add
followTsOrganizeImportsoption andprivateimport type toorderrule, updating documentation and tests.
- New Features:
- Add
followTsOrganizeImportsoption toorderrule inorder.ts, aligning import order with TypeScript's LSP Organize Imports.- Introduce
privateimport type for imports starting with#inimport-type.ts.- Documentation:
- Update
order.mdto includefollowTsOrganizeImportsandprivateimport type.- Tests:
- Add test cases in
order.spec.tsto validatefollowTsOrganizeImportsandprivateimport type behavior.This description was created by
for 923ffb49d223be002b0c51f10534e74b9db7a31b. It will automatically update as commits are pushed.
🦋 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
"""
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 caseThis test case correctly verifies that when
followTsOrganizeImports: trueis set, imports are ordered according to TypeScript's Language Server Protocol (LSP) Organize Imports feature.The order being tested:
- Private imports (#)
- Scoped package imports (@)
- Regular external imports
- Side effect imports
- Node.js built-in modules
- Local imports
4832-4851: LGTM - Properly tests explicitfollowTsOrganizeImports: falsesettingThe 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 manualgroupstake precedence overfollowTsOrganizeImportsGood test case showing that when a custom groups configuration is provided, it takes precedence over the
followTsOrganizeImportsoption, even when set to true.
4873-4934: LGTM - Comprehensive test for TypeScript path mappings with both option valuesThese test cases properly cover:
- How the rule handles imports with TypeScript path mappings when
followTsOrganizeImports: true- The expected behavior when
followTsOrganizeImports: false- The correct configuration for TypeScript project path resolution
4935-4956: LGTM - Tests Node.js builtin module handling withnode:prefixGood test case showing that the rule correctly handles the special case of Node.js builtin modules with the
node:prefix whenfollowTsOrganizeImports: true.
5341-5363: Fixed test case aligns with TypeScript Organize Imports behaviorThis 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 addedThe 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
@coderabbitaiin 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
@coderabbitaiin 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 pauseto pause the reviews on a PR.@coderabbitai resumeto resume the paused reviews.@coderabbitai reviewto trigger an incremental review. This is useful when automatic reviews are disabled for the repository.@coderabbitai full reviewto do a full review from scratch and review all the files again.@coderabbitai summaryto regenerate the summary of the PR.@coderabbitai generate docstringsto generate docstrings for this PR.@coderabbitai resolveresolve all the CodeRabbit review comments.@coderabbitai configurationto show the current CodeRabbit configuration for the repository.@coderabbitai helpto get help.
Other keywords and placeholders
- Add
@coderabbitai ignoreanywhere in the PR description to prevent this PR from being reviewed. - Add
@coderabbitai summaryto generate the high-level summary at a specific location in the PR description. - Add
@coderabbitaianywhere in the PR title to generate the title automatically.
CodeRabbit Configuration File (.coderabbit.yaml)
- You can programmatically configure CodeRabbit by adding a
.coderabbit.yamlfile 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.
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.
@coderabbitai review
: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.
@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)
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.
Accidentally marked as ready for review. 😅
@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?
@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.
@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 havetsconfig.jsonwithpathsmapping"#/*": ["./xxx/*"], etc.We'll need to read the nearest
package.json, and check it'simportsfield 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?)
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?
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-importbutmaybe-private-import? Or it should be documented. Do you thinkprivatewould 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.
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?
Naming is hard...
@Shinigami92 Let's move to
privateand keepfollowTsOrganizeImports, also let's add some other test cases by using custom groups withprivateenabled andfollowTsOrganizeImportsenabled/disabled.
Okay, will do so 👍
Should we emit a warning if the users define their own
groupsandfollowTsOrganizeImports: truewhich does not take any effect actually?
Yes, I think this could make sense 👍
@coderabbitai review
: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.
@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. 🤷
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.
I simply do not use this rule in my projects anymore
