Skip to content

Comments

feat(oxfmt/sort-imports): support custom groups#17576

Merged
leaysgur merged 24 commits intooxc-project:mainfrom
nilptr:nilptr/feat/sort-imports-custom-groups
Jan 19, 2026
Merged

feat(oxfmt/sort-imports): support custom groups#17576
leaysgur merged 24 commits intooxc-project:mainfrom
nilptr:nilptr/feat/sort-imports-custom-groups

Conversation

@nilptr
Copy link
Contributor

@nilptr nilptr commented Jan 2, 2026

#17076

Authorship & Use of AI

All code changes in this pull request are completed by a human (myself).
ChatGPT is used only to assist with writing comments and this PR description.

Commit Structure

The changes are split into small, meaningful commits.
Each commit should be reviewable independently.

Current State of the Implementation

For easier comparison, I forked actual and made a few auxiliary changes on a separate branch nilptr/feat/oxfmt-custom-groups.

The auxiliary changes include:

  • Upgraded oxfmt to address the incompatible binding API.
  • Updated eslint.config.mjs to lint and fix .ts files.
  • Updated .oxfmtrc to enable experimentalSortImports, keeping it consistent with eslint.config.mjs.

How to Compare?

  1. Clone git@github.com:nilptr/actual.git and check out nilptr/feat/oxfmt-custom-groups branch.

  2. Run yarn install

  3. Run NAPI_RS_NATIVE_LIBRARY_PATH=/path/to/binding.node yarn fmt:oxfmt

    The native binding can be obtained in one of the following ways:

    • Download the oxfmt.darwin-arm64.node.zip built by me (if you trust it, or scan it with VirusTotal).
    • Build it by yourself: clone my repo, run pnpm build, and locate the .node binding file under apps/oxfmt/src-js.
  4. Inspect the results with git diff — the diff shows the differences between perfectionist and oxfmt

Observed Differences

There are 9 differences in total:

  • 4 differences are unrelevant to experimentalSortImports. (be the way, they are introduced after I rebased main branch)
  • 5 differences are relevant to experimentalSortImports, but are not caused by the group-matching logic, which is the focus of this PR.
    • 4 of them are blank lines after side-effect imports being removed
    • 1 of them is the wrong ImportLineMetadata.source
image image

I believe we can start review now.
While the review is ongoing, I will also try to see whether the observed differences can be fixed in this PR.

@nilptr nilptr requested review from Dunqing and camc314 as code owners January 2, 2026 01:02
@github-actions github-actions bot added A-linter Area - Linter A-formatter Area - Formatter C-enhancement Category - New feature or request labels Jan 2, 2026
@nilptr nilptr force-pushed the nilptr/feat/sort-imports-custom-groups branch from 00ea51b to c405a09 Compare January 2, 2026 01:05
@codspeed-hq
Copy link

codspeed-hq bot commented Jan 2, 2026

CodSpeed Performance Report

Merging this PR will not alter performance

Comparing nilptr:nilptr/feat/sort-imports-custom-groups (92e8eae) with main (a0b3721)

Summary

✅ 38 untouched benchmarks
⏩ 7 skipped benchmarks1

Footnotes

  1. 7 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@nilptr

This comment was marked as resolved.

@Dunqing Dunqing removed their assignment Jan 3, 2026
@Dunqing Dunqing requested review from leaysgur and removed request for Dunqing and camc314 January 3, 2026 15:32
@leaysgur
Copy link
Member

leaysgur commented Jan 5, 2026

Thank you~. Great work! 👏🏻
(And sorry for the late review..., btw Happy New Year!)

I'll review the changes and go through from now.

I will also try to see whether the observed differences can be fixed in this PR.

If possible, could you submit a separate PR for these?

@nilptr nilptr force-pushed the nilptr/feat/sort-imports-custom-groups branch 2 times, most recently from f74914b to 73ff4ab Compare January 7, 2026 01:11
@nilptr

This comment was marked as resolved.

Copy link
Member

@leaysgur leaysgur left a comment

Choose a reason for hiding this comment

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

Thanks again. ✨

Actually, index-type and sibling-type are invalid groups in perfectionist.

OK, let's remove it.

Regarding this, I'll leave it up to you whether to address it in this PR or create a separate one.

@leaysgur
Copy link
Member

leaysgur commented Jan 8, 2026

NOTE:

  • I'll take care of resolving CI/Lint errors, so you don't need to worry about it
  • For CI/Test, you need to
    • crates/oxc_formatter: cargo t
    • (root): cargo t -p website_formatter
    • Then, cargo insta accept
  • When you are ready for a review, just press the Re-request review button

graphite-app bot pushed a commit that referenced this pull request Jan 8, 2026
Preparation for #17576

```
error: large size difference between variants
  --> apps/oxfmt/src/core/config.rs:51:1
   |
51 | /   pub enum ResolvedOptions {
52 | |       /// For JS/TS files formatted by oxc_formatter.
53 | | /     OxcFormatter {
54 | | |         format_options: FormatOptions,
55 | | |         /// For embedded language formatting (e.g., CSS in template literals)
56 | | |         external_options: Value,
57 | | |         insert_final_newline: bool,
58 | | |     },
   | | |_____- the largest variant contains at least 281 bytes
...  |
66 | | /     ExternalFormatterPackageJson {
67 | | |         external_options: Value,
68 | | |         sort_package_json: Option<sort_package_json::SortOptions>,
69 | | |         insert_final_newline: bool,
70 | | |     },
   | | |_____- the second-largest variant contains at least 75 bytes
71 | |   }
   | |___^ the entire enum is at least 288 bytes
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/rust-1.92.0/index.html#large_enum_variant
   = note: `-D clippy::large-enum-variant` implied by `-D warnings`
   = help: to override `-D warnings` add `#[allow(clippy::large_enum_variant)]`
help: consider boxing the large fields or introducing indirection in some other way to reduce the total size of the enum
   |
54 -         format_options: FormatOptions,
54 +         format_options: Box<FormatOptions>,
   |

```
@nilptr nilptr force-pushed the nilptr/feat/sort-imports-custom-groups branch from 1a3d3de to 0f369cd Compare January 11, 2026 15:49
@github-actions github-actions bot added the A-cli Area - CLI label Jan 11, 2026
@nilptr nilptr requested a review from leaysgur January 12, 2026 03:42
@nilptr nilptr force-pushed the nilptr/feat/sort-imports-custom-groups branch from bb9fef7 to 8381619 Compare January 18, 2026 09:18
@nilptr
Copy link
Contributor Author

nilptr commented Jan 18, 2026

Many tests involving customGroups are primarily testing the sorting options (such as type, order, etc.) inside a custom group, rather than the grouping logic itself.
Those behaviors are not implemented yet and outside the scope of this PR, so I just added 2 of them.
Besides that, because type: "line-length" is not supported yet, I made some adjustments to the test cases.

@leaysgur
Copy link
Member

Thanks again!

About their tests, everything is as expected, and since we've been modifying and porting test cases all along. 😅 So, it should be fine for now.

I plan to review it again tomorrow morning and merge it. 👍🏻

Copy link
Member

@leaysgur leaysgur left a comment

Choose a reason for hiding this comment

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

🚀

@leaysgur leaysgur merged commit 984d5c1 into oxc-project:main Jan 19, 2026
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-cli Area - CLI A-formatter Area - Formatter A-linter Area - Linter C-enhancement Category - New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants