feat: migrate ts formatting from prettier to oxfmt#7973
Conversation
|
Claude finished @paulbalaji's task in 1m 1s —— View job Review: Prettier to oxfmt Migration
Overall Assessment: ✅ LGTM This is a well-executed tooling migration from Prettier to oxfmt for TypeScript/JavaScript formatting. The PR correctly preserves Prettier for Solidity files where oxfmt lacks support. Key Changes Reviewed
CodeRabbit Issues - Addressed ✅
"exports": {
".": {
"types": "./dist/index.d.ts",
"default": "./dist/index.js"
}
}Security ReviewNo security concerns - this PR only touches:
No Solidity contract logic was modified, so Trail of Bits security analysis is not applicable. Minor Observations (non-blocking)
CI Status
|
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThis PR migrates formatting from Prettier to oxfmt across the monorepo, updates CI and editor settings, adds an .oxfmtrc.json, adjusts many package.json manifests and scripts, and applies widespread TypeScript formatting changes (mostly generic/implements/extends line-wrapping) and minor doc/changelog cleanups. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7973 +/- ##
=======================================
Coverage 77.02% 77.02%
=======================================
Files 117 117
Lines 2651 2651
Branches 244 244
=======================================
Hits 2042 2042
Misses 593 593
Partials 16 16
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
fd1c012 to
2b1267e
Compare
|
Filed upstream issue for the line-breaking differences: oxc-project/oxc#18737 Update: likely just prettier differences and not oxfmt related |
2b1267e to
89af23a
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@typescript/ccip-server/package.json`:
- Line 72: Remove the stray "moduleResolution": "nodenext" entry from
package.json and add (or ensure it exists) under the TypeScript compilerOptions
in tsconfig.json instead; look for the literal "moduleResolution" key in
package.json and move it into the "compilerOptions" object in tsconfig.json (or
delete the package.json key if the parent tsconfig already defines it), so the
TypeScript compiler options live in tsconfig.json rather than package.json.
In `@typescript/sdk/package.json`:
- Around line 18-21: The package.json exports map currently sets "." to
"./dist/index.js" while the top-level "types": "./dist/index.d.ts" remains but
modern TypeScript won't find it when "exports" exists; update the "exports"
entry so "." is an object that includes a "types" condition pointing to
"./dist/index.d.ts" and an "import" (or "default") condition pointing to
"./dist/index.js" (e.g., "." : { "types": "./dist/index.d.ts", "import":
"./dist/index.js" }), and keep the top-level "types" as a fallback for older
tools.
🧹 Nitpick comments (6)
typescript/sdk/src/zksync/ZKSyncDeployer.ts (1)
51-54: Consider stronger typing for constructor arguments.The
any[]type forconstructorArgumentsbypasses type safety. Since this is pre-existing code and not the focus of this formatting PR, consider addressing this in a follow-up.Typing alternatives
Options to explore:
- Use a generic type parameter if constructor arg types vary by contract
- Define a union type for known constructor signatures
- Use
unknown[]with runtime validation if types can't be known staticallytypescript/relayer/src/metadata/multisig.ts (1)
543-543: Consider replacinganywith a proper union type.Right now we're using
anyfor the prefix variable. Since it can be one of two shapes fromdecodeProofPrefixordecodeSimplePrefix, a union type would be cleaner and give us better type safety. As per coding guidelines, we should prefer proper typing overany.♻️ Proposed refactor using union type
+type DecodedPrefix = + | ReturnType<typeof MultisigMetadataBuilder.decodeProofPrefix> + | ReturnType<typeof MultisigMetadataBuilder.decodeSimplePrefix>; + - const prefix: any = + const prefix: DecodedPrefix = type === IsmType.MERKLE_ROOT_MULTISIG || type === IsmType.STORAGE_MERKLE_ROOT_MULTISIG ? this.decodeProofPrefix(metadata).vscode/settings.json (1)
6-27: Good formatter configuration for the editor.The per-language settings properly route JS/TS/JSON to oxc-vscode and Solidity to Prettier.
formatOnSavewill keep things tidy without manual intervention.One thing though - the
.lintstagedrcapparently handles.mdfiles with oxfmt, but there's no[markdown]formatter specified here. Might want to add that for consistency when editing docs, or leave it if manual formatting is preferred for markdown.🔧 Optional: Add markdown formatter config
"[jsonc]": { "editor.defaultFormatter": "oxc.oxc-vscode" }, + "[markdown]": { + "editor.defaultFormatter": "oxc.oxc-vscode" + }, "[solidity]": { "editor.defaultFormatter": "esbenp.prettier-vscode" }starknet/package.json (1)
30-30: Minor inconsistency: format script includes./package.json.This package formats both
./srcand./package.json, while most other packages in this PR only format./src. Not a problem per se – oxfmt can handle JSON – but might be worth aligning for consistency across the swamp. Or leave it be, no ogre's gonna lose sleep over it.🔧 Optional: align with other packages
- "format": "oxfmt --write ./src ./package.json" + "format": "oxfmt --write ./src"typescript/aleo-sdk/package.json (1)
52-58: eslint-config-prettier is actively used but may be worth reconciling with oxfmt.The root eslint config extends the 'prettier' configuration (via eslint-config-prettier) which gets inherited by aleo-sdk. However, since aleo-sdk uses oxfmt for formatting rather than Prettier, there's a mismatch—eslint-config-prettier's rules disable ESLint checks that conflict with Prettier's formatting, which is irrelevant when oxfmt is doing the actual formatting work.
If oxfmt and Prettier have different formatting behaviors, this could lead to formatting conflicts. You might consider whether aleo-sdk needs its own eslint config override that doesn't extend the prettier rules, or whether the root config should be updated to account for projects using oxfmt instead.
typescript/cosmos-types/package.json (1)
19-22: Minor inconsistency in exports format.The exports here use the shorthand style
".": "./dist/index.js"while other packages in this PR (likecosmos-sdk) use the full conditional exports with explicit types:".": { "types": "./dist/index.d.ts", "default": "./dist/index.js" }It still works because the top-level
typesfield exists, but for consistency across the swamp... er, monorepo, you might want to align the format.🔧 Suggested change for consistency
- "exports": { - ".": "./dist/index.js" - }, + "exports": { + ".": { + "types": "./dist/index.d.ts", + "default": "./dist/index.js" + } + },
Replaced Prettier with oxfmt for TypeScript/JavaScript formatting. oxfmt is ~100x faster than Prettier. Solidity files still use Prettier. Changes: - Added oxfmt to catalog and root package.json - Created .oxfmtrc.json config - Updated format scripts in all packages - Updated CI workflow to use oxfmt - Added VS Code oxc extension recommendation Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
1d9aa96 to
9697bd8
Compare
- Remove stray moduleResolution from ccip-server/package.json (belongs in tsconfig) - Add types condition to sdk exports for node16/nodenext/bundler moduleResolution
9a686d4 to
944cdf4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@typescript/infra/package.json`:
- Line 26: The package.json still defines a "prettier" npm script and lists
"prettier" in devDependencies; replace that with the intended formatter: update
the "prettier" script (scripts.prettier) to use the oxfmt CLI (e.g., "oxfmt fmt
./src ./config ./scripts ./test" or your project’s oxfmt invocation) and remove
"prettier" from devDependencies, adding the oxfmt package to devDependencies
instead; ensure you update any README or CI steps that reference the old
"prettier" script to call the new scripts/oxfmt binary (referencing the
scripts.prettier entry and the devDependencies keys to locate the changes).
🧹 Nitpick comments (3)
typescript/helloworld/.prettierrc (1)
1-19: Looks good, though the top-level settings are now just swamp decoration.Since Prettier only formats
.solfiles now (which have their ownoverridesblock), the top-leveltabWidth: 2,singleQuote: true, andtrailingComma: "all"on lines 2–4 are effectively dead config. Not harmful, but you could slim this down to just theoverridesandpluginsif you fancy a tidy swamp..vscode/settings.json (1)
6-27: Markdown formatting gap between lint-staged and VSCode settings.The
.lintstagedrcrunsoxfmt --writeon*.mdfiles, but there's no[markdown]formatter configured here. Developers might get surprised by formatting changes on commit that they don't see in their editor. Consider adding a[markdown]block withoxc.oxc-vscodefor consistency, or this might be intentional if MD format-on-save is undesirable.typescript/radix-sdk/package.json (1)
32-32: Format script misses test files.The
formatscript only targets./src, but this package hastest:unitandtest:e2escripts, suggesting test files exist. For comparison,typescript/http-registry-serverincludes both./src ./testin its format script.Suggested fix
- "format": "oxfmt --write ./src", + "format": "oxfmt --write ./src ./test",
Renamed 'prettier' scripts to 'format' using oxfmt in 11 packages: - typescript/infra, sdk, utils, ccip-server, widgets, rebalancer-sim, github-proxy, tron-sdk, starknet - solidity: renamed to 'format' but kept prettier for .sol files - typescript/helloworld: oxfmt for src, prettier for contracts (.sol) Removed prettier devDependency from packages that no longer need it. Kept prettier only where Solidity formatting is needed (root, solidity, helloworld).
⚙️ Node Service Docker Images Built Successfully
Full image paths |
Summary
Changes
pnpm-workspace.yamloxfmt: ^0.27.0to catalogpackage.json.oxfmtrc.json.lintstagedrc.github/workflows/test.yml.vscode/extensions.jsontypescript/*/package.jsonTest plan
pnpm exec oxfmt --check .passespnpm run lintpassespnpm buildsucceedsSummary by CodeRabbit
Chores
CI
Style
Docs