feat(oxfmt): support boolean shorthand for sortImports#20672
feat(oxfmt): support boolean shorthand for sortImports#20672Mister-Hope wants to merge 1 commit intooxc-project:mainfrom
sortImports#20672Conversation
github-actions
bot
added
A-cli
There was a problem hiding this comment.
Pull request overview
Updates Oxfmt’s configuration surface to accept a boolean shorthand for sortImports, and refreshes the generated schemas/docs and tests accordingly.
Changes:
- Extend
sortImportsconfig to acceptboolean | objectvia a newSortImportsUserConfigunion. - Update generated JSON/markdown schemas (and npm-distributed schema) to reflect the new type.
- Add CLI + API tests/fixtures for the boolean form.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tasks/website_formatter/src/snapshots/schema_markdown.snap | Updates rendered markdown docs to show sortImports as `object |
| tasks/website_formatter/src/snapshots/schema_json.snap | Updates schema snapshot to reference SortImportsUserConfig and adds its definition. |
| npm/oxfmt/configuration_schema.json | Updates shipped schema to reference SortImportsUserConfig and adds its definition. |
| apps/oxfmt/test/cli/sort_imports/sort_imports.test.ts | Adds a CLI test case for boolean true. |
| apps/oxfmt/test/cli/sort_imports/fixtures/boolean_true/input.ts | Adds a new CLI fixture input for the boolean config case. |
| apps/oxfmt/test/cli/sort_imports/fixtures/boolean_true/.oxfmtrc.json | Adds a new CLI fixture config enabling sort imports with true. |
| apps/oxfmt/test/api/sort_imports.test.ts | Adds an API test case for boolean true. |
| apps/oxfmt/src/core/oxfmtrc/to_oxfmt_options.rs | Adds parsing/mapping logic for SortImportsUserConfig into formatter options. |
| apps/oxfmt/src/core/oxfmtrc/format_config.rs | Changes sort_imports type to Option<SortImportsUserConfig> and introduces the untagged enum. |
| apps/oxfmt/src-js/config.generated.ts | Updates generated TS types so sortImports accepts `boolean |
| it("should sort with `true` (boolean)", async () => { | ||
| const input = `import { b } from "b"; | ||
| import { a } from "a"; | ||
| `; | ||
| const result = await format("a.ts", input, { | ||
| experimentalSortImports: true, | ||
| }); | ||
|
|
There was a problem hiding this comment.
This new API test only exercises the deprecated alias key (experimentalSortImports) with a boolean. Since the PR’s goal is boolean shorthand for sortImports, add a test that passes sortImports: true to format() so the primary (non-experimental) option name is covered and can’t regress.
| const snapshot = await runWriteModeAndSnapshot(cwd, ["input.ts"]); | ||
| expect(snapshot).toMatchSnapshot(); | ||
| }); | ||
|
|
There was a problem hiding this comment.
The new CLI test validates boolean enablement, but the fixture config uses the deprecated alias key experimentalSortImports. Since this PR advertises sortImports: true support, add a fixture/test that uses sortImports: true in .oxfmtrc.json so the documented key is covered at the CLI/config-file level as well.
| it("should sort imports when enabled via config with `sortImports: true`", async () => { | |
| const cwd = join(fixturesDir, "boolean_true_sort_imports_config"); | |
| const snapshot = await runWriteModeAndSnapshot(cwd, ["input.ts"]); | |
| expect(snapshot).toMatchSnapshot(); | |
| }); |
| SortImportsUserConfig::Bool(true) => { | ||
| format_options.sort_imports = Some(SortImportsOptions::default()); | ||
| None | ||
| } |
There was a problem hiding this comment.
In the boolean shorthand handling for sort_imports, the true path sets format_options.sort_imports directly and skips the shared object-processing path (including validate()). Consider mapping Bool(true) to an empty/default SortImportsConfig and running the same conversion+validation logic as {} so {} and true can’t diverge over time and there’s only one place to maintain defaults/validation.
| SortImportsUserConfig::Bool(true) => { | |
| format_options.sort_imports = Some(SortImportsOptions::default()); | |
| None | |
| } | |
| SortImportsUserConfig::Bool(true) => Some(Default::default()), |
|
Thank you for your time. However, I would like to close this because I believe the drawback of the increased amount of code to maintain outweighs the benefits gained from this change. Also, unlike other options, |
|
I understand that you don't like it. Just say you don't need it is enough.
The change only adds less than 30 lines in actual codes. So I am a little surprised about this expression. Basically, a Is this reason strong enough for you? |
This PR supports a boolean option for
sortImportssimilarly likesortPackageJson, allowing people to enable the feature with boolean flags:import { defineConfig } from "oxfmt"; export const config = defineConfig({ sortPackageJson: true, - sortImports: {}, + sortImports: true, });Note: currently all tests are still using
experimentalprefix, so I followed them. I am expecting some warnings from the copilot review.I think the changes could be done in a seperate PR, I would like to create one if any maintainer requests this.