Replace ESLint + Prettier with Biome#310
Conversation
Migrated from ESLint + Prettier using `biome migrate` commands. Rules turned off to match current ESLint config. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 34 minutes and 31 seconds. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThe pull request migrates the repository's code quality tooling from separate Prettier and ESLint configurations to Biome, a unified Rust-based linter and formatter. This includes removing old configuration files, adding Biome configuration, updating CI workflows, updating documentation, and reformatting the codebase to conform to Biome's style conventions. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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 |
Performance comparison: ESLint + Prettier vs BiomeBenchmark (local, clean tree after worktree cleanup)
~27x faster. Rule coverage
Auto-fixable improvements Biome finds (not caught by ESLint)
CI impactThe |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 993ea2d30c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "vcs": { "enabled": true, "clientKind": "git", "useIgnoreFile": true }, | ||
| "files": { | ||
| "ignoreUnknown": true, | ||
| "includes": ["packages/**", "builds/**", "global.d.ts"] |
There was a problem hiding this comment.
files.includes currently limits Biome to packages/**, builds/**, and global.d.ts, which drops tracked TS files like tools/build.ts, tools/template/index.ts, and vitest.config.ts from linting/formatting. In the current setup, ESLint only ignores JS and builds/**/* (eslint.config.js), so these tooling files are still checked today; after migrating scripts to biome ci ., this config silently reduces lint coverage and can let regressions in build/test tooling slip through.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Introduces Biome configuration and a migration plan intended to replace the repo’s current ESLint + Prettier tooling with a single lint/format tool.
Changes:
- Add a Biome configuration (
biome.json) for formatting, linting rules, and import organization. - Add a step-by-step migration plan document (
plans/biome.md) describing the intended rollout and parity goals.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
plans/biome.md |
Documents the intended migration steps and parity targets for switching to Biome. |
biome.json |
Defines Biome formatter/linter settings and rule overrides for the repo. |
| "files": { | ||
| "ignoreUnknown": true, | ||
| "includes": ["packages/**", "builds/**", "global.d.ts"] | ||
| }, |
There was a problem hiding this comment.
files.includes restricts Biome to only packages/**, builds/**, and global.d.ts, which is a narrower scope than the current prettier . / eslint . scripts (e.g., it would exclude tools/build.ts and root config .ts files). If the goal is a full ESLint+Prettier replacement, expand this include list (or remove it) so biome format/biome ci covers the same files as today.
| "rules": { | ||
| "recommended": true, | ||
| "complexity": { | ||
| "noUselessEscapeInRegex": "off" | ||
| }, | ||
| "correctness": { |
There was a problem hiding this comment.
The plan/PR description calls out turning off several unfixable rules for parity (e.g. noArguments, noImplicitAnyLet, noUnusedFunctionParameters, noGlobalEval, noNonNullAssertion, noThisInStatic), but they are not disabled in this linter.rules config. If those rules are enabled by recommended: true, biome ci will still fail/noise; please add explicit "off" entries (or update the plan if it’s outdated).
| ## What gets deleted | ||
|
|
||
| - `eslint.config.js` | ||
| - `.prettierrc` | ||
| - `.prettierignore` | ||
| - `eslint`, `@eslint/js`, `typescript-eslint`, `prettier` from devDependencies |
There was a problem hiding this comment.
This plan (and the PR description) says ESLint/Prettier configs and deps will be deleted/removed, but in the current branch those files still exist at repo root (eslint.config.js, .prettierrc, .prettierignore) and scripts still reference Prettier/ESLint. Either include those removals/updates in this PR (as described), or adjust the plan/PR description to match what’s actually being delivered.
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
|
@brianmhunt: Have we lost the Firefox test run? (Update: Found, perfect) |
| try { | ||
| applyBindings({}, testNode) | ||
| } catch (ex) { | ||
| } catch (_ex) { |
There was a problem hiding this comment.
Which rule is it? Any-type starts with underscore? It seems strange.
There was a problem hiding this comment.
@brianmhunt There are a lot of changes of this kind. I can understand that with class variables. But here, or with local const variables not. Can you explain it?
There was a problem hiding this comment.
This is Biome's noUnusedFunctionParameters rule. When a function parameter isn't used, Biome's auto-fix prefixes it with _ to signal it's intentionally ignored (same convention as TypeScript's noUnusedParameters).
On second thought, these _ prefixes add noise on a legacy codebase. We've changed the approach: noUnusedFunctionParameters is now set to warn (not auto-fixed), so these won't be modified. The current diff should only contain pure formatting changes (matching the existing Prettier config) plus 5 targeted fixes (explicit getter returns, annotated switch fallthroughs).
Format all files with Biome (matches Prettier settings). Safe auto-fixes applied by `biome check --fix`: unused imports removed, unused variables prefixed, getter returns made explicit, switch fallthroughs annotated. Skipped unsafe fixes (useLiteralKeys, useOptionalChain, useTemplate) as they surface latent type errors when converting bracket to dot notation. Disabled organizeImports (reorders break TS inference in some files). All 2679 tests pass, tsc 0 errors. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Single Rust-native tool for linting + formatting (27x faster). Migrated config matches Prettier style + ESLint rules. New rules enabled: noUnusedImports, noUnusedVariables, noUnusedFunctionParameters (warn), noRedeclare, noFallthroughSwitchClause, useGetterReturn (error). Removed: eslint, @eslint/js, typescript-eslint, prettier, globals Added: @biomejs/biome CI: single `biome ci .` replaces Prettier + ESLint steps Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
brianmhunt
force-pushed
the
modern-tooling/phase-4-biome
branch
from
904395c to
f68ef93
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@builds/knockout/spec/bindingDependencyBehaviors.js`:
- Around line 568-569: Fix the typo in the inline comment above the vm call:
change "ch ange view model to new object" to "change view model to new object"
(the comment immediately preceding the vm({ obj1: { prop1: 'Second view ' },
prop2: 'model' }) invocation).
In `@builds/knockout/spec/components/defaultLoaderBehaviors.js`:
- Around line 14-15: Remove the unused second argument from the two unregister
calls: locate the ko.components.unregister calls referencing testComponentName
and 'nonexistent-component' and change them to the single-argument form (call
ko.components.unregister with only the component name) so they match the rest of
the file.
In `@builds/knockout/spec/defaultBindings/eventBehaviors.js`:
- Line 38: The test fixture assigns HTML to testNode.innerHTML with a mismatched
closing tag (opens <a> but closes </button>); update the string in the
assignment to use a matching closing tag (change the closing tag to </a>) so the
element markup is consistent and the test becomes deterministic. Refer to the
testNode.innerHTML assignment in eventBehaviors.js to locate and correct the
closing tag.
In `@builds/knockout/spec/defaultBindings/ifnotBehaviors.js`:
- Line 4: Update the test title strings to fix spelling and improve clarity:
change "truey" to "truthy" in the it() description "Should remove descendant
nodes from the document (and not bind them) if the value is truey" and replace
"bindedness" with a correct term such as "binding" or "boundness" in the other
it() description that uses "bindedness" (around line 42) so both test names read
clearly and correctly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fde9a392-9b04-4aa7-8921-ba3c728e58e3
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (144)
.github/workflows/lint-and-typecheck.yml.prettierignore.prettierrcAGENTS.mdbiome.jsonbuilds/knockout/helpers/mocha-test-helpers.jsbuilds/knockout/spec/arrayEditDetectionBehaviors.jsbuilds/knockout/spec/arrayToDomEditDetectionBehaviors.jsbuilds/knockout/spec/asyncBehaviors.jsbuilds/knockout/spec/asyncBindingBehaviors.jsbuilds/knockout/spec/bindingAttributeBehaviors.jsbuilds/knockout/spec/bindingDependencyBehaviors.jsbuilds/knockout/spec/bindingGlobalsBehaviors.jsbuilds/knockout/spec/bindingPreprocessingBehaviors.jsbuilds/knockout/spec/components/componentBindingBehaviors.jsbuilds/knockout/spec/components/customElementBehaviors.jsbuilds/knockout/spec/components/defaultLoaderBehaviors.jsbuilds/knockout/spec/components/loaderRegistryBehaviors.jsbuilds/knockout/spec/crossWindowBehaviors.jsbuilds/knockout/spec/defaultBindings/attrBehaviors.jsbuilds/knockout/spec/defaultBindings/checkedBehaviors.jsbuilds/knockout/spec/defaultBindings/clickBehaviors.jsbuilds/knockout/spec/defaultBindings/cssBehaviors.jsbuilds/knockout/spec/defaultBindings/enableDisableBehaviors.jsbuilds/knockout/spec/defaultBindings/eventBehaviors.jsbuilds/knockout/spec/defaultBindings/foreachBehaviors.jsbuilds/knockout/spec/defaultBindings/hasfocusBehaviors.jsbuilds/knockout/spec/defaultBindings/htmlBehaviors.jsbuilds/knockout/spec/defaultBindings/ifBehaviors.jsbuilds/knockout/spec/defaultBindings/ifnotBehaviors.jsbuilds/knockout/spec/defaultBindings/letBehaviors.jsbuilds/knockout/spec/defaultBindings/optionsBehaviors.jsbuilds/knockout/spec/defaultBindings/selectedOptionsBehaviors.jsbuilds/knockout/spec/defaultBindings/styleBehaviors.jsbuilds/knockout/spec/defaultBindings/submitBehaviors.jsbuilds/knockout/spec/defaultBindings/textBehaviors.jsbuilds/knockout/spec/defaultBindings/textInputBehaviors.jsbuilds/knockout/spec/defaultBindings/uniqueNameBehaviors.jsbuilds/knockout/spec/defaultBindings/usingBehaviors.jsbuilds/knockout/spec/defaultBindings/valueBehaviors.jsbuilds/knockout/spec/defaultBindings/visibleHiddenBehaviors.jsbuilds/knockout/spec/defaultBindings/withBehaviors.jsbuilds/knockout/spec/dependentObservableBehaviors.jsbuilds/knockout/spec/dependentObservableDomBehaviors.jsbuilds/knockout/spec/domNodeDisposalBehaviors.jsbuilds/knockout/spec/expressionRewritingBehaviors.jsbuilds/knockout/spec/extenderBehaviors.jsbuilds/knockout/spec/mappingHelperBehaviors.jsbuilds/knockout/spec/memoizationBehaviors.jsbuilds/knockout/spec/nativeTemplateEngineBehaviors.jsbuilds/knockout/spec/nodePreprocessingBehaviors.jsbuilds/knockout/spec/observableArrayBehaviors.jsbuilds/knockout/spec/observableArrayChangeTrackingBehaviors.jsbuilds/knockout/spec/observableBehaviors.jsbuilds/knockout/spec/observableUtilsBehaviors.jsbuilds/knockout/spec/onErrorBehaviors.jsbuilds/knockout/spec/parseHtmlFragment.jsbuilds/knockout/spec/pureComputedBehaviors.jsbuilds/knockout/spec/subscribableBehaviors.jsbuilds/knockout/spec/taskBehaviors.jsbuilds/knockout/spec/templatingBehaviors.jsbuilds/knockout/spec/utilsBehaviors.jsbuilds/knockout/spec/utilsDomBehaviors.jsbuilds/knockout/src/index.tsbuilds/reference/index.tsbuilds/reference/spec/bindingGlobalsBehavior.jsbuilds/reference/spec/iifeBehavior.jsbuilds/reference/src/index.tseslint.config.jspackage.jsonpackages/bind/spec/bindingAttributeBehaviors.tspackages/bind/spec/bindingCompletionPromiseBehavior.tspackages/bind/src/applyBindings.tspackages/bind/src/bindingContext.tspackages/bind/verified-behaviors.jsonpackages/binding.component/spec/componentBindingBehaviors.tspackages/binding.component/src/slotBinding.tspackages/binding.component/verified-behaviors.jsonpackages/binding.core/spec/attrBehaviors.tspackages/binding.core/spec/checkedBehaviors.tspackages/binding.core/spec/descendantsCompleteBehaviors.tspackages/binding.core/spec/textInputBehaviors.tspackages/binding.core/spec/usingBehaviors.tspackages/binding.core/spec/valueBehaviors.tspackages/binding.core/src/options.tspackages/binding.core/src/value.tspackages/binding.core/verified-behaviors.jsonpackages/binding.foreach/spec/eachBehavior.tspackages/binding.foreach/src/foreach.tspackages/binding.foreach/verified-behaviors.jsonpackages/binding.if/spec/elseBehaviors.tspackages/binding.if/spec/withBehaviors.tspackages/binding.if/verified-behaviors.jsonpackages/binding.template/spec/foreachBehaviors.tspackages/binding.template/spec/nativeTemplateEngineBehaviors.tspackages/binding.template/spec/templatingBehaviors.tspackages/binding.template/src/nativeTemplateEngine.tspackages/binding.template/src/templateSources.tspackages/binding.template/src/templating.tspackages/binding.template/verified-behaviors.jsonpackages/builder/verified-behaviors.jsonpackages/computed/src/computed.tspackages/computed/src/throttleExtender.tspackages/computed/verified-behaviors.jsonpackages/filter.punches/verified-behaviors.jsonpackages/lifecycle/spec/LifeCycleBehaviors.tspackages/lifecycle/src/LifeCycle.tspackages/lifecycle/verified-behaviors.jsonpackages/observable/spec/mappingHelperBehaviors.tspackages/observable/src/mappingHelpers.tspackages/observable/src/observable.tspackages/observable/src/observableArray.changeTracking.tspackages/observable/verified-behaviors.jsonpackages/provider.attr/verified-behaviors.jsonpackages/provider.bindingstring/verified-behaviors.jsonpackages/provider.component/spec/customElementBehaviors.tspackages/provider.component/verified-behaviors.jsonpackages/provider.databind/spec/dataBindProviderBehaviors.tspackages/provider.databind/verified-behaviors.jsonpackages/provider.multi/verified-behaviors.jsonpackages/provider.mustache/spec/attributeInterpolationSpec.tspackages/provider.mustache/verified-behaviors.jsonpackages/provider.native/verified-behaviors.jsonpackages/provider.virtual/verified-behaviors.jsonpackages/provider/spec/providerBehaviors.tspackages/provider/verified-behaviors.jsonpackages/utils.component/src/ComponentABC.tspackages/utils.component/verified-behaviors.jsonpackages/utils.functionrewrite/verified-behaviors.jsonpackages/utils.jsx/verified-behaviors.jsonpackages/utils.parser/spec/parserBehaviors.tspackages/utils.parser/spec/preparserBehavior.tspackages/utils.parser/src/Identifier.tspackages/utils.parser/src/Node.tspackages/utils.parser/src/Parser.tspackages/utils.parser/verified-behaviors.jsonpackages/utils/spec/memoizationBehaviors.tspackages/utils/src/dom/fixes.tspackages/utils/src/dom/virtualElements.tspackages/utils/src/options.tspackages/utils/src/tasks.tspackages/utils/verified-behaviors.jsonplans/biome.mdplans/modern-tooling.md
💤 Files with no reviewable changes (2)
- .prettierignore
- .prettierrc
| ko.applyBindings({ someItem: null, condition: true }, testNode); | ||
| expect(testNode.childNodes[0].childNodes.length).to.deep.equal(0); | ||
| }); | ||
| it('Should remove descendant nodes from the document (and not bind them) if the value is truey', function () { |
There was a problem hiding this comment.
Fix typos in test names for clarity.
Line 4 uses truey and Line 42 uses bindedness; these read as spelling errors and make test output noisier to scan.
Also applies to: 42-42
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@builds/knockout/spec/defaultBindings/ifnotBehaviors.js` at line 4, Update the
test title strings to fix spelling and improve clarity: change "truey" to
"truthy" in the it() description "Should remove descendant nodes from the
document (and not bind them) if the value is truey" and replace "bindedness"
with a correct term such as "binding" or "boundness" in the other it()
description that uses "bindedness" (around line 42) so both test names read
clearly and correctly.
- Expand files.includes to cover tools/ and vitest.config.ts - Fix mismatched HTML tag in eventBehaviors (<a>...</button> → </a>) - Fix typo "ch ange" → "change" in bindingDependencyBehaviors - Fix typo "truey" → "truthy" in ifnotBehaviors - Remove unused requireCallbacks variable - Remove unused second arg in unregister calls Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Document what was skipped and why (unsafe fixes break tsc, organizeImports breaks TS inference, useArrowFunction breaks KO this-binding). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Safety reviewThree independent adversarial reviews were run against this PR. Summary: Formatting changes (safe)The bulk of the 135-file diff is pure formatting matching the existing Prettier config:
Semantic changes (all verified safe)Beyond the 5 originally disclosed fixes, the adversarial review found additional semantic changes applied by
Config parityAll Prettier formatting options are replicated. All ESLint rules that were enforced remain enforced. Three intentionally stricter additions: File scope is equal or wider (now includes Verification
|
Summary
useArrowFunction,useTemplate,useLiteralKeys,useOptionalChain,useImportType)Plan
See
plans/biome.mdfor full execution plan.Test plan
bunx @biomejs/biome ci .passes (0 errors)bun run buildsucceeds (all 27 packages)bunx vitest runpasses (2679 tests)bunx tscpasses (0 type errors)useArrowFunctionauto-fix didn't break KOthis-binding callbacks🤖 Generated with Claude Code
Summary by CodeRabbit
Chores
Style
Documentation