Skip to content

build: adopt Oxfmt and enforce TypeScript formatting in CI - #1825

Open
eitsupi wants to merge 3 commits into
mainfrom
build/oxfmt-formatting
Open

eitsupi wants to merge 3 commits into
mainfrom
build/oxfmt-formatting

Conversation

@eitsupi

@eitsupi eitsupi commented Oct 5, 2026 •

Copy link
Copy Markdown
Member

Summary

Follow up on #1818 and the toolchain review in #1732 by introducing a shared formatter and checking formatting in CI. After the ESLint migration, contributors had no formatting command or CI check; this PR provides both and formats the existing TypeScript sources consistently.

To keep the diff reviewable, formatting is limited to TypeScript files (.ts, .tsx, .mts, .cts) for now. Other file types can be enabled in follow-up PRs.

  • Pin Oxfmt to 0.73.0 and add pnpm run format / pnpm run format:check.
  • Share .oxfmtrc.jsonc between the CLI and the already recommended Oxc editor extension. Restrict the scope to TypeScript via ignorePatterns, use four-space indentation and single quotes, and disable import and package metadata sorting. Generated output (out/, dist/) is skipped through the existing Git ignore rules.
  • Format 173 TypeScript files (169 under src/, 4 under html/) in a dedicated mechanical commit. Move one no-control-regex suppression comment next to the regular expression after line wrapping moved it to another line.
  • Add an independent format CI job with Node.js 24, frozen-lockfile installation, and pnpm run format:check. Enable format on save for TypeScript and document the contributor commands.

@eitsupi
eitsupi force-pushed the build/oxfmt-formatting branch from 7eaa161 to 1c7d01d Compare October 18, 2026 15:23
@eitsupi eitsupi changed the title build: adopt Oxfmt and enforce repository formatting in CI build: adopt Oxfmt and enforce TypeScript formatting in CI Oct 18, 2026
@eitsupi
eitsupi marked this pull request as ready for review October 18, 2026 15:23
@eitsupi
eitsupi requested a review from renkun-ken October 18, 2026 15:23

@renkun-ken renkun-ken left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Reviewed 1c7d01d7865b2ca97e7c1971e400cbdd4a7409cd. No blocking correctness issues found; approving with one minor documentation clarification below.

Validation:

  • Verified all 179 changed file blobs against GitHub and reproduced the 173 TypeScript changes with Oxfmt 0.73.0, apart from the documented lint-suppression relocation. Compared normalized emitted JavaScript and inspected the remaining redundant-parenthesis difference; no behavior changes found.
  • Frozen-lockfile dependency installation (--ignore-scripts), pnpm run format:check, pnpm run lint, and pnpm run typecheck passed locally.
  • CLI smoke tests covered root/nested .ts, .tsx, .mts, and .cts files, excluded non-TypeScript files, and ignored generated/dependency directories. Direct language-server checks confirmed TypeScript formatting and non-TypeScript exclusion, with the .gitignore caveat below.
  • All eight current GitHub checks pass, including tests on macOS, Linux, and Windows. I did not rerun the full extension-host/R integration suites locally.

Comment thread CONTRIBUTING.md
Comment on lines +39 to +40
`package.json` sorting are disabled. Oxfmt respects the repository's `.gitignore`
files, so generated bundles, compiled tests, and dependencies are excluded.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Non-blocking documentation clarification: please qualify this as CLI behavior. With Oxfmt 0.73.0 and this PR's .oxfmtrc.jsonc/.gitignore in a fresh Git fixture, oxfmt --check . skips out/generated.ts and dist/generated.ts, but oxfmt --lsp returns formatting edits for textDocument/formatting requests on those same files. Sharing the config therefore does not give editor formatting the same generated-file exclusion. Saying “The CLI also respects the repository's .gitignore…” would match the tested behavior; if exclusion in the editor is intended too, add explicit generated-directory exclusions after the TypeScript re-inclusion patterns.

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.

2 participants