feat: no-jsx-props-bind #7410
feat: no-jsx-props-bind #7410
Conversation
🦋 Changeset detectedLatest commit: 52fd7cb The changes in this PR will be included in the next version bump. This PR includes changesets to release 13 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughAdds a new nursery lint rule Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 |
github-actions
bot
added
A-Project
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (11)
crates/biome_rule_options/src/no_jsx_props_bind.rs (1)
6-12: Add brief field docs for schema consumersTiny win: add rustdoc comments per field to improve generated schema/help text.
crates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/validAllowBind.jsx (1)
1-3: Good coverage for allowBind=trueConsider adding a case with partial application to ensure we only relax .bind when configured:
- onClick={this._handleClick.bind(this, 1)}
crates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/valid.jsx (1)
1-3: Add cases to exercise “stable by reference” resolutionTo cover the open “declared elsewhere” gap, please add:
- Valid: function handle() {} …
- Invalid: function Component(){ const handle = () => {}; return }
This will guide the implementation that resolves identifiers to their defining scope.crates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/validAllowArrowFunctions.jsx (1)
1-3: Nice. Add a couple more arrow samples for breadth
- Inside map: items.map(i => <Btn onClick={() => doThing(i)} />)
- TS params: <Btn onClick={(e: MouseEvent) => doThing(e)} />
crates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/validAllowFunctions.jsx (2)
3-12: Split fixtures to isolate behavioursThis mixes two behaviours: inline function expressions and referencing a named function. Consider separate files (e.g. validAllowFunctions.inline.jsx and valid.functionReference.jsx) so failures pinpoint the precise allowance.
9-12: Add a case for imported/stable referencesSince you’re exploring stability of referenced functions, add a case like
import { onClick } from "./handler"; <Foo onClick={onClick} />to lock in expected behaviour for external declarations.Happy to draft the extra fixture if you confirm desired behaviour (always valid like ESLint, or only when proven stable?).
crates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/validIgnoreRefs.jsx (1)
3-10: Good coverage for callback refs; consider adding object ref tooAdd a case like
const r = React.createRef(); <Foo ref={r} />(oruseRef) to ensure ignoreRefs also covers object refs, not only callback refs.crates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/invalid.jsx (1)
1-12: Add guard cases to pin expected behaviour
- Add a valid case
onClick={onClick}(declared elsewhere) to capture the “reference is fine” scenario.- Add a valid case
onClick={foo()}to ensure we do NOT misreport generic call expressions as “.bind()”.I can draft these fixtures if you’d like.
crates/biome_js_analyze/src/lint/nursery/no_jsx_props_bind.rs (3)
10-11: Rule summary should mention arrow functions explicitlyTitle says “.bind() or function declaration” but the rule also forbids inline arrow functions.
Apply:
- /// Disallow .bind() or function declaration in JSX props + /// Disallow .bind(), inline arrow functions, and function expressions in JSX props
54-78: Minor: be tolerant of non-expression attribute valuesYou already short-circuit on expression values; consider returning early if the attribute value isn’t an expression container to avoid extra work.
48-53: Future enhancement: flag unstable identifiers (optional, behind a flag)To cover the “passed-by-reference but unstable” gap:
- If the value is an identifier or
this.method, resolve its binding viactx.model().- If the binding is declared inside the same function component body (and not wrapped in
useCallback) or is a function-valued prop created in render, treat it as unstable.- Keep this off by default for ESLint parity; gate behind an option (e.g.
checkIdentifiers).I can sketch the symbol-resolution logic if you want to pursue this next.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (12)
crates/biome_configuration/src/analyzer/linter/rules.rsis excluded by!**/rules.rsand included by**crates/biome_diagnostics_categories/src/categories.rsis excluded by!**/categories.rsand included by**crates/biome_js_analyze/src/lint/nursery.rsis excluded by!**/nursery.rsand included by**crates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/invalid.jsx.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/valid.jsx.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/validAllowArrowFunctions.jsx.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/validAllowBind.jsx.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/validAllowFunctions.jsx.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/validIgnoreDomComponents.jsx.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/validIgnoreRefs.jsx.snapis excluded by!**/*.snapand included by**packages/@biomejs/backend-jsonrpc/src/workspace.tsis excluded by!**/backend-jsonrpc/src/workspace.tsand included by**packages/@biomejs/biome/configuration_schema.jsonis excluded by!**/configuration_schema.jsonand included by**
📒 Files selected for processing (15)
crates/biome_js_analyze/src/lint/nursery/no_jsx_props_bind.rs(1 hunks)crates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/invalid.jsx(1 hunks)crates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/valid.jsx(1 hunks)crates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/validAllowArrowFunctions.jsx(1 hunks)crates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/validAllowArrowFunctions.options.json(1 hunks)crates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/validAllowBind.jsx(1 hunks)crates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/validAllowBind.options.json(1 hunks)crates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/validAllowFunctions.jsx(1 hunks)crates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/validAllowFunctions.options.json(1 hunks)crates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/validIgnoreDomComponents.jsx(1 hunks)crates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/validIgnoreDomComponents.options.json(1 hunks)crates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/validIgnoreRefs.jsx(1 hunks)crates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/validIgnoreRefs.options.json(1 hunks)crates/biome_rule_options/src/lib.rs(1 hunks)crates/biome_rule_options/src/no_jsx_props_bind.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{rs,toml}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Format Rust and TOML files before committing (use
just f/just format).
Files:
crates/biome_rule_options/src/no_jsx_props_bind.rscrates/biome_rule_options/src/lib.rscrates/biome_js_analyze/src/lint/nursery/no_jsx_props_bind.rs
crates/biome_*/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place core crates under /crates/biome_*/
Files:
crates/biome_rule_options/src/no_jsx_props_bind.rscrates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/validIgnoreDomComponents.options.jsoncrates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/validAllowFunctions.options.jsoncrates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/validAllowFunctions.jsxcrates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/validAllowArrowFunctions.options.jsoncrates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/validIgnoreDomComponents.jsxcrates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/validAllowBind.options.jsoncrates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/validAllowBind.jsxcrates/biome_rule_options/src/lib.rscrates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/validAllowArrowFunctions.jsxcrates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/valid.jsxcrates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/validIgnoreRefs.options.jsoncrates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/validIgnoreRefs.jsxcrates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/invalid.jsxcrates/biome_js_analyze/src/lint/nursery/no_jsx_props_bind.rs
crates/biome_*_{syntax,parser,formatter,analyze,factory,semantic}/**
📄 CodeRabbit inference engine (CLAUDE.md)
Maintain the per-language crate structure: biome_{lang}_{syntax,parser,formatter,analyze,factory,semantic}
Files:
crates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/validIgnoreDomComponents.options.jsoncrates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/validAllowFunctions.options.jsoncrates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/validAllowFunctions.jsxcrates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/validAllowArrowFunctions.options.jsoncrates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/validIgnoreDomComponents.jsxcrates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/validAllowBind.options.jsoncrates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/validAllowBind.jsxcrates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/validAllowArrowFunctions.jsxcrates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/valid.jsxcrates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/validIgnoreRefs.options.jsoncrates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/validIgnoreRefs.jsxcrates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/invalid.jsxcrates/biome_js_analyze/src/lint/nursery/no_jsx_props_bind.rs
**/tests/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place test files under a tests/ directory in each crate
Files:
crates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/validIgnoreDomComponents.options.jsoncrates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/validAllowFunctions.options.jsoncrates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/validAllowFunctions.jsxcrates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/validAllowArrowFunctions.options.jsoncrates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/validIgnoreDomComponents.jsxcrates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/validAllowBind.options.jsoncrates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/validAllowBind.jsxcrates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/validAllowArrowFunctions.jsxcrates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/valid.jsxcrates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/validIgnoreRefs.options.jsoncrates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/validIgnoreRefs.jsxcrates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/invalid.jsx
🧠 Learnings (13)
📚 Learning: 2025-09-05T09:13:58.901Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-09-05T09:13:58.901Z
Learning: Applies to crates/biome_analyze/crates/biome_rule_options/lib/**/*.rs : Derive Serialize, Deserialize, Deserializable (and schemars JsonSchema behind feature) on options types; use serde attributes rename_all="camelCase", deny_unknown_fields, default
Applied to files:
crates/biome_rule_options/src/no_jsx_props_bind.rs
📚 Learning: 2025-09-05T09:13:58.901Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-09-05T09:13:58.901Z
Learning: Applies to crates/biome_analyze/crates/biome_js_analyze/tests/specs/nursery/** : Place snapshot test cases for new rules under tests/specs/nursery/<ruleName>/ with files prefixed by invalid/valid
Applied to files:
crates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/validIgnoreDomComponents.options.jsoncrates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/validAllowFunctions.options.jsoncrates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/validAllowArrowFunctions.options.jsoncrates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/validAllowBind.options.jsoncrates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/validAllowBind.jsxcrates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/valid.jsxcrates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/validIgnoreRefs.options.jsoncrates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/validIgnoreRefs.jsxcrates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/invalid.jsx
📚 Learning: 2025-08-05T14:43:29.581Z
Learnt from: dyc3
PR: biomejs/biome#7081
File: packages/@biomejs/biome/configuration_schema.json:7765-7781
Timestamp: 2025-08-05T14:43:29.581Z
Learning: The file `packages/biomejs/biome/configuration_schema.json` is auto-generated and should not be manually edited or reviewed for schema issues; any changes should be made at the code generation source.
Applied to files:
crates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/validIgnoreDomComponents.options.jsoncrates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/validAllowFunctions.options.jsoncrates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/validAllowArrowFunctions.options.jsoncrates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/validAllowBind.options.jsoncrates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/validIgnoreRefs.options.json
📚 Learning: 2025-08-11T11:48:27.774Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_formatter/CONTRIBUTING.md:0-0
Timestamp: 2025-08-11T11:48:27.774Z
Learning: Applies to crates/biome_formatter/biome_html_formatter/tests/specs/html/**/options.json : When non-default formatting options are needed for a test group, place an options.json (biome.json format) alongside the .html files in that folder
Applied to files:
crates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/validIgnoreDomComponents.options.jsoncrates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/validAllowFunctions.options.jsoncrates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/validIgnoreRefs.options.json
📚 Learning: 2025-09-05T09:13:58.901Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-09-05T09:13:58.901Z
Learning: Applies to crates/biome_analyze/crates/biome_js_analyze/tests/specs/**/*.jsonc : Use .jsonc files containing arrays of code snippets for snapshot tests that require multiple script snippets
Applied to files:
crates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/validIgnoreDomComponents.options.jsoncrates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/validAllowFunctions.options.jsoncrates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/validAllowArrowFunctions.options.jsoncrates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/validAllowBind.options.jsoncrates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/validIgnoreRefs.options.json
📚 Learning: 2025-09-05T09:13:58.901Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-09-05T09:13:58.901Z
Learning: Applies to crates/biome_analyze/crates/biome_js_analyze/lib/src/lint/nursery/**/*.rs : Place all new rules in the nursery group under biome_js_analyze/lib/src/lint/nursery
Applied to files:
crates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/validIgnoreDomComponents.options.jsoncrates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/validAllowFunctions.options.jsoncrates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/validAllowArrowFunctions.options.jsoncrates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/validAllowBind.options.jsoncrates/biome_rule_options/src/lib.rscrates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/validIgnoreRefs.options.jsoncrates/biome_js_analyze/src/lint/nursery/no_jsx_props_bind.rs
📚 Learning: 2025-09-05T09:13:58.901Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-09-05T09:13:58.901Z
Learning: Applies to crates/biome_analyze/crates/biome_rule_options/lib/**/*.rs : Define per‑rule options in the biome_rule_options crate (one file per rule, e.g., use_my_rule.rs)
Applied to files:
crates/biome_rule_options/src/lib.rs
📚 Learning: 2025-08-11T11:48:52.001Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_js_formatter/CONTRIBUTING.md:0-0
Timestamp: 2025-08-11T11:48:52.001Z
Learning: Applies to crates/biome_js_formatter/**/Cargo.toml : Add biome_js_formatter as a path dependency in Cargo.toml: biome_js_formatter = { version = "0.0.1", path = "../biome_js_formatter" }
Applied to files:
crates/biome_rule_options/src/lib.rs
📚 Learning: 2025-09-05T09:13:58.901Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-09-05T09:13:58.901Z
Learning: Applies to crates/biome_analyze/crates/biome_js_analyze/**/src/**/*.rs : In declare_lint_rule! metadata, set version: "next"
Applied to files:
crates/biome_js_analyze/src/lint/nursery/no_jsx_props_bind.rs
📚 Learning: 2025-09-05T09:13:58.901Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-09-05T09:13:58.901Z
Learning: Applies to crates/biome_analyze/crates/biome_js_analyze/**/src/**/*.rs : When deprecating a rule, add deprecated: "<reason>" to declare_lint_rule!
Applied to files:
crates/biome_js_analyze/src/lint/nursery/no_jsx_props_bind.rs
📚 Learning: 2025-09-05T09:13:58.901Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-09-05T09:13:58.901Z
Learning: Applies to crates/biome_analyze/crates/biome_js_analyze/**/src/**/*.rs : Set the language field in declare_lint_rule! to the most specific applicable language (js/jsx/ts/tsx)
Applied to files:
crates/biome_js_analyze/src/lint/nursery/no_jsx_props_bind.rs
📚 Learning: 2025-09-05T09:13:58.901Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-09-05T09:13:58.901Z
Learning: Applies to crates/biome_analyze/crates/biome_js_analyze/**/src/**/*.rs : When porting a lint rule, set sources in declare_lint_rule! (RuleSource::Eslint(...).same() or .inspired())
Applied to files:
crates/biome_js_analyze/src/lint/nursery/no_jsx_props_bind.rs
📚 Learning: 2025-09-05T09:13:58.901Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-09-05T09:13:58.901Z
Learning: Applies to crates/biome_analyze/crates/biome_js_analyze/**/src/**/*.rs : If a rule provides a code action, declare fix_kind in declare_lint_rule!
Applied to files:
crates/biome_js_analyze/src/lint/nursery/no_jsx_props_bind.rs
🧬 Code graph analysis (1)
crates/biome_js_analyze/src/lint/nursery/no_jsx_props_bind.rs (1)
crates/biome_analyze/src/rule.rs (4)
recommended(538-541)sources(553-556)inspired(241-246)domains(568-571)
🔇 Additional comments (9)
crates/biome_rule_options/src/no_jsx_props_bind.rs (2)
1-12: Options shape, derives, and serde config look solidMatches our conventions (Line 5), public and schema-friendly. Nice one.
3-11: No custom Default required: all options default to false upstream
DerivingDefaultalready mirrors eslint-plugin-react’s jsx-no-bind defaults (all five are false), so no change needed.crates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/validIgnoreRefs.options.json (1)
1-16: Config file LGTMSchema path, rule path, and option key casing all look correct.
crates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/validIgnoreDomComponents.options.json (1)
1-16: Config looks soundMatches the new rule and option; pathing to the schema also looks correct for this folder depth.
crates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/validAllowBind.options.json (1)
1-16: LGTMOption wiring for allowBind is correct and consistent with the other options files.
crates/biome_rule_options/src/lib.rs (1)
114-114: Module exposure is correct
pub mod no_jsx_props_bind;is in the expected position and naming matches the file. Remember this file is generated; ensurextask/codegenwas run so it doesn’t get clobbered later.crates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/validAllowArrowFunctions.options.json (1)
1-16: Config looks correctCamel-cased option matches schema expectations. No issues.
crates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/validAllowFunctions.options.json (1)
1-16: Config looks correctOption name aligns with the rule options. All good.
crates/biome_js_analyze/src/lint/nursery/no_jsx_props_bind.rs (1)
38-45: Confirm TSX coverageWith
language: "jsx", does this fire for TSX as well? If not, we should either widen language or add a TSX variant and tests.Would you add a TSX test fixture mirroring the current JSX ones to confirm?
| if options.ignore_dom_components && element.is_element() { | ||
| return None; | ||
| } |
There was a problem hiding this comment.
ignoreDomComponents currently disables the rule for (almost) all elements
element.is_element() checks “is JSX element vs fragment”, not “is DOM (lowercase) vs custom (capitalised)”. This will skip both DOM and custom components whenever the option is true.
Use the tag name to detect lowercase DOM components:
- if options.ignore_dom_components && element.is_element() {
- return None;
- }
+ if options.ignore_dom_components {
+ // Ignore only built-in DOM components (lowercase tag names)
+ let is_dom_component = if let Ok(token) = element.name_value_token() {
+ let text = token.text();
+ matches!(text.chars().next(), Some(c) if c.is_ascii_lowercase())
+ } else {
+ false
+ };
+ if is_dom_component {
+ return None;
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if options.ignore_dom_components && element.is_element() { | |
| return None; | |
| } | |
| if options.ignore_dom_components { | |
| // Ignore only built-in DOM components (lowercase tag names) | |
| let is_dom_component = if let Ok(token) = element.name_value_token() { | |
| let text = token.text(); | |
| matches!(text.chars().next(), Some(c) if c.is_ascii_lowercase()) | |
| } else { | |
| false | |
| }; | |
| if is_dom_component { | |
| return None; | |
| } | |
| } |
🤖 Prompt for AI Agents
In crates/biome_js_analyze/src/lint/nursery/no_jsx_props_bind.rs around lines
61-63, the current check uses element.is_element() which only distinguishes
element vs fragment and wrongly disables the rule for custom components too;
instead, inspect the JSX tag name (from the element's opening element/name) and
determine if it is a DOM element by checking the identifier's first character is
lowercase (or otherwise treating lowercase tag names as DOM), and only return
None when the tag name indicates a lowercase DOM element; update the conditional
to extract the tag identifier safely and test its first character rather than
using is_element().
| AnyJsExpression::JsCallExpression(_) => { | ||
| // TODO: Check this | ||
| if options.allow_bind { | ||
| None | ||
| } else { | ||
| Some("JSX props should not use .bind()") | ||
| } | ||
| } |
There was a problem hiding this comment.
Call-expression check mislabels any call as “.bind()”
This will flag onClick={foo()} as if it were .bind(). We must specifically detect a member call where the property is named “bind”.
Narrow the check to .bind:
- AnyJsExpression::JsCallExpression(_) => {
- // TODO: Check this
- if options.allow_bind {
- None
- } else {
- Some("JSX props should not use .bind()")
- }
- }
+ AnyJsExpression::JsCallExpression(call) => {
+ if options.allow_bind {
+ return None;
+ }
+ let is_bind = call
+ .callee()
+ .ok()
+ .and_then(|c| c.as_js_member_expression().cloned())
+ .and_then(|m| m.member_name().ok())
+ .and_then(|n| n.value_token().ok())
+ .map(|t| t.text() == "bind")
+ .unwrap_or(false);
+ if is_bind {
+ Some("JSX props should not use .bind()")
+ } else {
+ None
+ }
+ }Also add a test that onClick={foo()} is not reported.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| AnyJsExpression::JsCallExpression(_) => { | |
| // TODO: Check this | |
| if options.allow_bind { | |
| None | |
| } else { | |
| Some("JSX props should not use .bind()") | |
| } | |
| } | |
| AnyJsExpression::JsCallExpression(call) => { | |
| if options.allow_bind { | |
| return None; | |
| } | |
| let is_bind = call | |
| .callee() | |
| .ok() | |
| .and_then(|c| c.as_js_member_expression().cloned()) | |
| .and_then(|m| m.member_name().ok()) | |
| .and_then(|n| n.value_token().ok()) | |
| .map(|t| t.text() == "bind") | |
| .unwrap_or(false); | |
| if is_bind { | |
| Some("JSX props should not use .bind()") | |
| } else { | |
| None | |
| } | |
| } |
🤖 Prompt for AI Agents
In crates/biome_js_analyze/src/lint/nursery/no_jsx_props_bind.rs around lines
95-102, the current match arm treats any call expression as a .bind() usage and
reports it incorrectly; change the detection to only match a MemberCall (or call
where callee is a member expression) whose property/identifier is named "bind"
so plain calls like onClick={foo()} are not flagged, and add a unit test
asserting that onClick={foo()} produces no diagnostic while
onClick={foo.bind(this)} does.
| <span onClick={() => console.log("Hello!")} />; ( | ||
| <button | ||
| type="button" | ||
| onClick={function () { | ||
| alert("1337"); | ||
| }} | ||
| /> | ||
| </>; |
There was a problem hiding this comment.
Syntax error: stray ; ( breaks parsing
There’s an extra ; ( after the <span /> which makes the file invalid JSX, so the test will fail before the rule runs.
Apply this fix:
- <span onClick={() => console.log("Hello!")} />; (
+ <span onClick={() => console.log("Hello!")} />📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <span onClick={() => console.log("Hello!")} />; ( | |
| <button | |
| type="button" | |
| onClick={function () { | |
| alert("1337"); | |
| }} | |
| /> | |
| </>; | |
| <span onClick={() => console.log("Hello!")} /> | |
| <button | |
| type="button" | |
| onClick={function () { | |
| alert("1337"); | |
| }} | |
| /> | |
| </>; |
🤖 Prompt for AI Agents
In
crates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/validIgnoreDomComponents.jsx
around lines 9–16, the JSX is broken by an extra "; (" immediately after the
self-closing <span />, which produces a syntax error; remove the stray semicolon
and opening parenthesis so the fragment contains only valid JSX nodes (e.g.,
<span onClick={...} /> <button ... /> inside the fragment) and ensure the
fragment is properly opened/closed with no stray punctuation.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/invalidIgnoreDomComponents.jsx (1)
1-5: Broaden coverage with a couple of quick companions
- Add a self-closing variant to avoid any oddities with children.
- Ensure there’s a valid DOM case under the same options (e.g.
<div onClick={() => {}} />) to prove the ignore actually works.- When you tackle identifier references, add:
- Stable: module-scope
function onClick(){}then<Foo onClick={onClick} />(valid).- Unstable: inside component
const onClick = () => {}; return <Foo onClick={onClick} />;(invalid, once supported).Happy to sketch these fixtures if helpful.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (4)
crates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/invalid.jsx.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/invalidIgnoreDomComponents.jsx.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/valid.jsx.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/validIgnoreDomComponents.jsx.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (6)
crates/biome_js_analyze/src/lint/nursery/no_jsx_props_bind.rs(1 hunks)crates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/invalid.jsx(1 hunks)crates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/invalidIgnoreDomComponents.jsx(1 hunks)crates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/invalidIgnoreDomComponents.options.json(1 hunks)crates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/valid.jsx(1 hunks)crates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/validIgnoreDomComponents.jsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- crates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/invalidIgnoreDomComponents.options.json
🚧 Files skipped from review as they are similar to previous changes (4)
- crates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/validIgnoreDomComponents.jsx
- crates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/invalid.jsx
- crates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/valid.jsx
- crates/biome_js_analyze/src/lint/nursery/no_jsx_props_bind.rs
🧰 Additional context used
📓 Path-based instructions (3)
crates/biome_*_{syntax,parser,formatter,analyze,factory,semantic}/**
📄 CodeRabbit inference engine (CLAUDE.md)
Maintain the per-language crate structure: biome_{lang}_{syntax,parser,formatter,analyze,factory,semantic}
Files:
crates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/invalidIgnoreDomComponents.jsx
crates/biome_*/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place core crates under /crates/biome_*/
Files:
crates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/invalidIgnoreDomComponents.jsx
**/tests/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place test files under a tests/ directory in each crate
Files:
crates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/invalidIgnoreDomComponents.jsx
🔇 Additional comments (1)
crates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/invalidIgnoreDomComponents.jsx (1)
1-5: Good invalid fixture for custom components with ignoreDomComponents enabledThis correctly exercises an inline arrow on a non-DOM component; should still be flagged. All good.
dyc3
left a comment
dyc3
left a comment
There was a problem hiding this comment.
For that case, you can use the Semantic model to look up where the binding was defined.
Also, to fix the merge conflicts:
- rebase on main (you'll get conflicts)
- run
just gen-all - continue the rebase
git rebase --continue
| } | ||
|
|
||
| impl Rule for NoJsxPropsBind { | ||
| type Query = Ast<JsxAttribute>; |
There was a problem hiding this comment.
Here's how you get access to the semantic model. It'll be available in ctx.model()
| type Query = Ast<JsxAttribute>; | |
| type Query = Semantic<JsxAttribute>; |
There was a problem hiding this comment.
Thanks for the hint, although I'm still struggling with this to be honest. Let me try for a couple more hours.
| pub ignore_dom_components: bool, | ||
| pub ignore_refs: bool, | ||
| pub allow_arrow_functions: bool, | ||
| pub allow_functions: bool, | ||
| pub allow_bind: bool, |
There was a problem hiding this comment.
We generally don't add options on the first iteration of a new rule because we want to have concrete use cases for them.
Most of these options seem to defeat the purpose of the rule too?
There was a problem hiding this comment.
I just went off the ESLint rule. Anyway, I've removed them all as requested.
ignoreDOMComponents at least makes sense to me, ignoreRefs probably as well.
|
|
||
| impl Rule for NoJsxPropsBind { | ||
| type Query = Ast<JsxAttribute>; | ||
| type State = &'static str; |
There was a problem hiding this comment.
This is a rather unusual way to emit varying diagnostic messages. Typically, you pass along the information needed to write the diagnostic message instead of passing the message itself.
There was a problem hiding this comment.
Okay, I adapted how this was done in other rules.
| // | ||
| // Read our guidelines to write great diagnostics: | ||
| // https://docs.rs/biome_analyze/latest/biome_analyze/#what-a-rule-should-say-to-the-user | ||
| // | ||
| let node = ctx.query(); | ||
| Some(RuleDiagnostic::new( | ||
| rule_category!(), | ||
| node.range(), | ||
| markup! { | ||
| {state} | ||
| }, |
There was a problem hiding this comment.
Please read the guide and make sure to follow the rule pillars. The diagnostic does not contain enough info.
| /// Using `.bind()` on a function or declaring a function directly in props | ||
| /// creates a new function on every render, which is treated as a completely different function. |
There was a problem hiding this comment.
It would be good to elaborate and explain the performance impact.
There was a problem hiding this comment.
I slightly rephrased it from here. Maybe we can just copy/paste the entire opening paragraph?
https://github.com/jsx-eslint/eslint-plugin-react/blob/master/docs/rules/jsx-no-bind.md
sgarcialaguna
force-pushed
the
nojsxpropsbind
branch
from
3ab2037 to
c73fcdc
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
crates/biome_js_analyze/src/lint/nursery/no_jsx_props_bind.rs (4)
9-13: Docs: call out arrow functions explicitly and tighten the rationale.The summary says “function declaration”, but we’re flagging arrow/function expressions. Also, briefly explain the render-time allocation cost.
- /// Disallow .bind() or function declaration in JSX props + /// Disallow .bind() and inline function expressions (including arrow functions) in JSX props @@ - /// Using `.bind()` on a function or declaring a function directly in props - /// creates a new function on every render, which is treated as a completely different function. - /// This may cause unnecessary rerenders. + /// Using `.bind()` or creating a function inline in props allocates a new + /// function every render, changing identity and potentially triggering unnecessary rerenders.Also applies to: 15-27, 29-34
58-63: Switch to Semantic query to unlock identifier resolution (needed for the “referenced function” case).This gives you
ctx.model()to resolveonClick={onClick}to its declaration and assess stability.-use biome_analyze::{ - Ast, Rule, RuleDiagnostic, RuleDomain, RuleSource, context::RuleContext, declare_lint_rule, -}; +use biome_analyze::{ + Ast, Rule, RuleDiagnostic, RuleDomain, RuleSource, context::RuleContext, declare_lint_rule, Semantic, +}; @@ - type Query = Ast<JsxAttribute>; + type Query = Semantic<JsxAttribute>;Skeleton for later (where you match the prop expression):
if let AnyJsExpression::JsIdentifierExpression(id_expr) = &expression { let model = ctx.model(); if let Ok(name_ref) = id_expr.name() { if let Some(binding) = model.binding(&name_ref) { // TODO: inspect `binding.tree()` or `binding.syntax()`: // - Is it a class method (this.handleClick)? Consider stable. // - Is it a top-level function? Consider stable. // - Is it a variable assigned to an arrow/function inside a function component? // If not wrapped in useCallback, consider unstable. } } }Happy to flesh this out with concrete AST checks against Biome’s semantic model API.
111-126: Diagnostic: include the prop name and a concrete reason.Small UX win and clearer action.
- fn diagnostic(_ctx: &RuleContext<Self>, state: &Self::State) -> Option<RuleDiagnostic> { + fn diagnostic(ctx: &RuleContext<Self>, state: &Self::State) -> Option<RuleDiagnostic> { + let prop = ctx + .query() + .name() + .ok() + .and_then(|n| n.value_token().ok()) + .map(|t| t.text().to_string()) + .unwrap_or_else(|| "prop".to_string()); let note = match state.invalid_kind { InvalidKind::ArrowFunction => "JSX props should not use arrow functions", InvalidKind::Bind => "JSX props should not use .bind()", InvalidKind::Function => "JSX props should not use functions", }; Some( RuleDiagnostic::new( rule_category!(), state.attribute_range, - "Pass stable functions as props to avoid unnecessary rerenders.", + format!("Avoid creating functions in '{prop}'; it allocates a new function each render."), ) .note(note) .note("Consider extracting the function or wrapping it with React.useCallback"), ) }
64-109: Heuristic for referenced identifiers (what’s “stable”).If you want to go beyond eslint parity, a pragmatic heuristic is:
- this.handleClick: point to a class method/property in the enclosing class → allow.
- Top-level function: allow.
- Variable in a function component initialised to an arrow/function:
- If initialiser is inside React.useCallback(...) → allow.
- Otherwise, consider unstable (warn).
I can wire this up using
ctx.model()with conservative fallbacks.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (8)
crates/biome_cli/src/execute/migrate/eslint_any_rule_to_biome.rsis excluded by!**/migrate/eslint_any_rule_to_biome.rsand included by**crates/biome_configuration/src/analyzer/linter/rules.rsis excluded by!**/rules.rsand included by**crates/biome_diagnostics_categories/src/categories.rsis excluded by!**/categories.rsand included by**crates/biome_js_analyze/src/lint/nursery.rsis excluded by!**/nursery.rsand included by**crates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/invalid.jsx.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/valid.jsx.snapis excluded by!**/*.snapand included by**packages/@biomejs/backend-jsonrpc/src/workspace.tsis excluded by!**/backend-jsonrpc/src/workspace.tsand included by**packages/@biomejs/biome/configuration_schema.jsonis excluded by!**/configuration_schema.jsonand included by**
📒 Files selected for processing (5)
crates/biome_js_analyze/src/lint/nursery/no_jsx_props_bind.rs(1 hunks)crates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/invalid.jsx(1 hunks)crates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/valid.jsx(1 hunks)crates/biome_rule_options/src/lib.rs(1 hunks)crates/biome_rule_options/src/no_jsx_props_bind.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- crates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/invalid.jsx
- crates/biome_rule_options/src/no_jsx_props_bind.rs
- crates/biome_rule_options/src/lib.rs
- crates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/valid.jsx
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{rs,toml}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Format Rust and TOML files before committing (use
just f/just format).
Files:
crates/biome_js_analyze/src/lint/nursery/no_jsx_props_bind.rs
crates/biome_*_{syntax,parser,formatter,analyze,factory,semantic}/**
📄 CodeRabbit inference engine (CLAUDE.md)
Maintain the per-language crate structure: biome_{lang}_{syntax,parser,formatter,analyze,factory,semantic}
Files:
crates/biome_js_analyze/src/lint/nursery/no_jsx_props_bind.rs
crates/biome_*/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place core crates under /crates/biome_*/
Files:
crates/biome_js_analyze/src/lint/nursery/no_jsx_props_bind.rs
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-09-05T09:13:58.901Z
Learning: Applies to crates/biome_analyze/crates/biome_js_analyze/lib/src/lint/nursery/**/*.rs : Place all new rules in the nursery group under biome_js_analyze/lib/src/lint/nursery
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-09-05T09:13:58.901Z
Learning: Applies to crates/biome_analyze/crates/biome_js_analyze/**/src/**/*.rs : Set the language field in declare_lint_rule! to the most specific applicable language (js/jsx/ts/tsx)
📚 Learning: 2025-09-05T09:13:58.901Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-09-05T09:13:58.901Z
Learning: Applies to crates/biome_analyze/crates/biome_js_analyze/**/src/**/*.rs : When deprecating a rule, add deprecated: "<reason>" to declare_lint_rule!
Applied to files:
crates/biome_js_analyze/src/lint/nursery/no_jsx_props_bind.rs
🧬 Code graph analysis (1)
crates/biome_js_analyze/src/lint/nursery/no_jsx_props_bind.rs (1)
crates/biome_analyze/src/rule.rs (4)
recommended(554-557)sources(569-572)inspired(249-254)domains(584-587)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (26)
- GitHub Check: Test Node.js API
- GitHub Check: Documentation
- GitHub Check: Bench (biome_package)
- GitHub Check: Bench (biome_configuration)
- GitHub Check: Test (depot-windows-2022-16)
- GitHub Check: Test (depot-ubuntu-24.04-arm-16)
- GitHub Check: Bench (biome_html_parser)
- GitHub Check: Bench (biome_css_analyze)
- GitHub Check: Bench (biome_graphql_parser)
- GitHub Check: Check JS Files
- GitHub Check: Bench (biome_module_graph)
- GitHub Check: Lint project (depot-windows-2022)
- GitHub Check: End-to-end tests
- GitHub Check: Lint project (depot-ubuntu-24.04-arm-16)
- GitHub Check: Check Dependencies
- GitHub Check: Bench (biome_html_formatter)
- GitHub Check: Bench (biome_graphql_formatter)
- GitHub Check: Bench (biome_json_formatter)
- GitHub Check: Bench (biome_js_analyze)
- GitHub Check: Bench (biome_json_analyze)
- GitHub Check: Bench (biome_json_parser)
- GitHub Check: Bench (biome_css_formatter)
- GitHub Check: Bench (biome_js_parser)
- GitHub Check: Bench (biome_css_parser)
- GitHub Check: Bench (biome_js_formatter)
- GitHub Check: autofix
🔇 Additional comments (3)
crates/biome_js_analyze/src/lint/nursery/no_jsx_props_bind.rs (3)
37-44: Rule metadata LGTM.Name, language ("jsx"), source and domains look correct for a nursery rule.
75-86: Arrow/function expression detection LGTM.Covers the common inline cases.
29-34: Add a “Valid” example for identifier and call cases.To lock behaviour:
- onClick={onClick} (referenced function)
- onClick={foo()} (plain call; should be allowed)
I can add fixtures mirroring eslint-plugin-react’s examples and the extra cases above.
| AnyJsExpression::JsCallExpression(call) => { | ||
| // This will still throw a false positive on e.g. window.bind() | ||
| let is_bind = call | ||
| .callee() | ||
| .ok() | ||
| .and_then(|c| c.as_js_static_member_expression().cloned()) | ||
| .and_then(|m| m.member().ok()) | ||
| .and_then(|n| n.value_token().ok()) | ||
| .map(|t| t.text() == "bind") | ||
| .unwrap_or(false); | ||
| if is_bind { | ||
| Some(NoJsxPropsBindState { | ||
| invalid_kind: InvalidKind::Bind, | ||
| attribute_range: expression.range(), | ||
| }) | ||
| // Some("JSX props should not use .bind()") | ||
| } else { | ||
| None | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
.bind() detection: support computed/optional members and avoid gaps.
Current check only recognises static members. Cover obj["bind"](…) and optional chaining obj.bind?.(…).
- AnyJsExpression::JsCallExpression(call) => {
- // This will still throw a false positive on e.g. window.bind()
- let is_bind = call
- .callee()
- .ok()
- .and_then(|c| c.as_js_static_member_expression().cloned())
- .and_then(|m| m.member().ok())
- .and_then(|n| n.value_token().ok())
- .map(|t| t.text() == "bind")
- .unwrap_or(false);
+ AnyJsExpression::JsCallExpression(call) => {
+ use biome_js_syntax::AnyJsMemberExpression;
+
+ // Unwrap optional chaining: (obj.bind)?.(...)
+ let callee = call.callee().ok().and_then(|c| {
+ if let Some(chain) = c.as_js_chain_expression() {
+ chain.expression().ok()
+ } else {
+ Some(c)
+ }
+ });
+
+ let member = callee.and_then(|c| c.as_js_member_expression().cloned());
+ let is_bind = member
+ .and_then(|m: AnyJsMemberExpression| m.member_name().ok())
+ .and_then(|n| n.value_token().ok())
+ .map(|t| t.text() == "bind")
+ .unwrap_or(false);
if is_bind {
- Some(NoJsxPropsBindState {
- invalid_kind: InvalidKind::Bind,
- attribute_range: expression.range(),
- })
+ Some(NoJsxPropsBindState {
+ invalid_kind: InvalidKind::Bind,
+ // Highlight just the callee if we can
+ attribute_range: callee.map(|c| c.range()).unwrap_or_else(|| expression.range()),
+ })
} else {
None
}
}Add tests for:
- onClick={foo()} (not reported)
- onClick={obj"bind"}
- onClick={obj.bind?.(this)}
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| AnyJsExpression::JsCallExpression(call) => { | |
| // This will still throw a false positive on e.g. window.bind() | |
| let is_bind = call | |
| .callee() | |
| .ok() | |
| .and_then(|c| c.as_js_static_member_expression().cloned()) | |
| .and_then(|m| m.member().ok()) | |
| .and_then(|n| n.value_token().ok()) | |
| .map(|t| t.text() == "bind") | |
| .unwrap_or(false); | |
| if is_bind { | |
| Some(NoJsxPropsBindState { | |
| invalid_kind: InvalidKind::Bind, | |
| attribute_range: expression.range(), | |
| }) | |
| // Some("JSX props should not use .bind()") | |
| } else { | |
| None | |
| } | |
| } | |
| AnyJsExpression::JsCallExpression(call) => { | |
| use biome_js_syntax::AnyJsMemberExpression; | |
| // Unwrap optional chaining: (obj.bind)?.(...) | |
| let callee = call.callee().ok().and_then(|c| { | |
| if let Some(chain) = c.as_js_chain_expression() { | |
| chain.expression().ok() | |
| } else { | |
| Some(c) | |
| } | |
| }); | |
| let member = callee.and_then(|c| c.as_js_member_expression().cloned()); | |
| let is_bind = member | |
| .and_then(|m: AnyJsMemberExpression| m.member_name().ok()) | |
| .and_then(|n| n.value_token().ok()) | |
| .map(|t| t.text() == "bind") | |
| .unwrap_or(false); | |
| if is_bind { | |
| Some(NoJsxPropsBindState { | |
| invalid_kind: InvalidKind::Bind, | |
| // Highlight just the callee if we can | |
| attribute_range: callee.map(|c| c.range()).unwrap_or_else(|| expression.range()), | |
| }) | |
| } else { | |
| None | |
| } | |
| } |
🤖 Prompt for AI Agents
In crates/biome_js_analyze/src/lint/nursery/no_jsx_props_bind.rs around lines 87
to 106, the current .bind() detection only matches static member expressions and
misses computed member calls (obj["bind"](...)) and optional chaining/member
calls (obj.bind?.(...)) and may produce gaps; update the callee analysis to also
handle JsComputedMemberExpression and
JsOptionalCallExpression/JsOptionalMemberExpression (or the AST variants in this
crate) by extracting the member name from computed string literal keys and by
unwrapping optional call/member nodes to check the final member token text ==
"bind"; ensure you preserve the negative case for plain calls (onClick={foo()})
and add/update tests for onClick={foo()} (no report),
onClick={obj["bind"](this)} (report), and onClick={obj.bind?.(this)} (report).
CodSpeed Performance ReportMerging #7410 will not alter performanceComparing Summary
Footnotes
|
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
crates/biome_js_analyze/src/lint/nursery/no_jsx_props_bind.rs (1)
58-63: Use the semantic query to enable referenced-function analysisSwitch to
Semantic<JsxAttribute>so you can resolve identifiers viactx.model()and handle the “referenced function declared elsewhere” case.-use biome_analyze::{ - Ast, Rule, RuleDiagnostic, RuleDomain, RuleSource, context::RuleContext, declare_lint_rule, -}; +use biome_analyze::{ + Ast, Rule, RuleDiagnostic, RuleDomain, RuleSource, context::RuleContext, declare_lint_rule, + Semantic, +}; @@ - type Query = Ast<JsxAttribute>; + type Query = Semantic<JsxAttribute>;Would you like me to follow up with a targeted implementation to resolve
JsReferenceIdentifierto its binding and classify stability by scope (module/global => stable; inside component body withoutuseCallback=> unstable)?
🧹 Nitpick comments (3)
crates/biome_js_analyze/src/lint/nursery/no_jsx_props_bind.rs (3)
9-13: Docs: say “arrow/function expressions”, not “function declaration”, and briefly mention the perf costDeclarations can’t appear in JSX expressions; what you mean are function expressions. Also add one line on why new identities hurt memoisation.
- /// Disallow .bind() or function declaration in JSX props + /// Disallow .bind(), arrow functions, or function expressions in JSX props @@ - /// Using `.bind()` on a function or declaring a function directly in props - /// creates a new function on every render, which is treated as a completely different function. - /// This may cause unnecessary rerenders. + /// Using `.bind()` or creating a function inline in props creates a new function + /// on every render, changing identity and defeating memoisation (e.g. `React.memo`, + /// `useMemo`, `PureComponent`), which may cause unnecessary rerenders.Also applies to: 15-27, 29-35
107-122: Diagnostic: include attribute name and precise kind; tweak wording“functions” is vague; say “function expressions”. Also mention the prop name to make fixes clearer.
-pub struct NoJsxPropsBindState { - invalid_kind: InvalidKind, - attribute_range: TextRange, -} +pub struct NoJsxPropsBindState { + invalid_kind: InvalidKind, + attribute_range: TextRange, + // Optional: surface prop name in the diagnostic + // attribute_name: Option<biome_rowan::TextSize>, // or store token range +} @@ - let note = match state.invalid_kind { - InvalidKind::ArrowFunction => "JSX props should not use arrow functions", - InvalidKind::Bind => "JSX props should not use .bind()", - InvalidKind::Function => "JSX props should not use functions", + let note = match state.invalid_kind { + InvalidKind::ArrowFunction => "JSX props should not use arrow functions", + InvalidKind::Bind => "JSX props should not use .bind()", + InvalidKind::Function => "JSX props should not use function expressions", }; Some( RuleDiagnostic::new( rule_category!(), state.attribute_range, - "Pass stable functions as props to avoid unnecessary rerenders.", + "Pass stable function references as props to avoid unnecessary rerenders.", ) .note(note) .note("Consider extracting the function or wrapping it in useCallback"), )
64-105: Support the “referenced function” case (heuristic via semantic model)Not a blocker, but here’s a minimal approach: if the expression is a reference, resolve its binding and classify stability by where it’s declared.
match &expression { + AnyJsExpression::JsReferenceIdentifier(id) => { + let model = ctx.model(); + // If we can’t resolve, don’t report + let Some(binding) = model.binding(id) else { return None; }; + // Heuristic: + // - Declared at module scope or as a class field initializer (arrow) => stable + // - Declared inside the current function component body and not produced by a `useCallback` call => unstable + // Wire up an `is_unstable_local_function(&binding, model)` helper as a follow‑up. + if is_unstable_local_function(&binding, &model) { + return Some(NoJsxPropsBindState { + invalid_kind: InvalidKind::Function, + attribute_range: expression.range(), + }); + } + None + }Happy to send a follow‑up patch with
is_unstable_local_function(scope walk + quickuseCallbackcallee check).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/biome_js_analyze/src/lint/nursery/no_jsx_props_bind.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{rs,toml}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Format Rust and TOML files before committing (use
just f/just format).
Files:
crates/biome_js_analyze/src/lint/nursery/no_jsx_props_bind.rs
crates/biome_*_{syntax,parser,formatter,analyze,factory,semantic}/**
📄 CodeRabbit inference engine (CLAUDE.md)
Maintain the per-language crate structure: biome_{lang}_{syntax,parser,formatter,analyze,factory,semantic}
Files:
crates/biome_js_analyze/src/lint/nursery/no_jsx_props_bind.rs
crates/biome_*/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place core crates under /crates/biome_*/
Files:
crates/biome_js_analyze/src/lint/nursery/no_jsx_props_bind.rs
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-09-05T09:13:58.901Z
Learning: Applies to crates/biome_analyze/crates/biome_js_analyze/lib/src/lint/nursery/**/*.rs : Place all new rules in the nursery group under biome_js_analyze/lib/src/lint/nursery
📚 Learning: 2025-09-05T09:13:58.901Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-09-05T09:13:58.901Z
Learning: Applies to crates/biome_analyze/crates/biome_js_analyze/**/src/**/*.rs : When deprecating a rule, add deprecated: "<reason>" to declare_lint_rule!
Applied to files:
crates/biome_js_analyze/src/lint/nursery/no_jsx_props_bind.rs
📚 Learning: 2025-09-05T09:13:58.901Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-09-05T09:13:58.901Z
Learning: Applies to crates/biome_analyze/crates/biome_js_analyze/**/src/**/*.rs : If a rule provides a code action, declare fix_kind in declare_lint_rule!
Applied to files:
crates/biome_js_analyze/src/lint/nursery/no_jsx_props_bind.rs
🧬 Code graph analysis (1)
crates/biome_js_analyze/src/lint/nursery/no_jsx_props_bind.rs (1)
crates/biome_analyze/src/rule.rs (4)
recommended(554-557)sources(569-572)inspired(249-254)domains(584-587)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (23)
- GitHub Check: Check JS Files
- GitHub Check: Test Node.js API
- GitHub Check: Documentation
- GitHub Check: Test (depot-windows-2022-16)
- GitHub Check: Test (depot-ubuntu-24.04-arm-16)
- GitHub Check: Check Dependencies
- GitHub Check: Bench (biome_graphql_parser)
- GitHub Check: Bench (biome_package)
- GitHub Check: Bench (biome_css_parser)
- GitHub Check: Bench (biome_js_parser)
- GitHub Check: Bench (biome_json_parser)
- GitHub Check: Bench (biome_js_analyze)
- GitHub Check: Bench (biome_html_parser)
- GitHub Check: Bench (biome_module_graph)
- GitHub Check: Bench (biome_configuration)
- GitHub Check: Bench (biome_graphql_formatter)
- GitHub Check: Bench (biome_json_analyze)
- GitHub Check: Bench (biome_css_formatter)
- GitHub Check: Bench (biome_json_formatter)
- GitHub Check: Bench (biome_html_formatter)
- GitHub Check: Bench (biome_js_formatter)
- GitHub Check: Bench (biome_css_analyze)
- GitHub Check: autofix
🔇 Additional comments (2)
crates/biome_js_analyze/src/lint/nursery/no_jsx_props_bind.rs (2)
37-44: Rule metadata looks goodName, source mapping, and nursery placement align with project conventions.
37-44: No change required
Thelanguage: "jsx"setting is consistent with numerous existing JSX rules (e.g.no_suspicious_semicolon_in_jsx,no_array_index_key,no_duplicate_jsx_props, etc.) and follows the crate’s convention.
| match &expression { | ||
| AnyJsExpression::JsArrowFunctionExpression(_) => Some(NoJsxPropsBindState { | ||
| invalid_kind: InvalidKind::ArrowFunction, | ||
| attribute_range: expression.range(), | ||
| }), | ||
|
|
||
| AnyJsExpression::JsFunctionExpression(_) => Some(NoJsxPropsBindState { | ||
| invalid_kind: InvalidKind::Function, | ||
| attribute_range: expression.range(), | ||
| }), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Gate arrow/function matches behind options
Allow users to opt‑in/out via allow_arrow_functions and allow_functions.
- match &expression {
- AnyJsExpression::JsArrowFunctionExpression(_) => Some(NoJsxPropsBindState {
+ match &expression {
+ AnyJsExpression::JsArrowFunctionExpression(_) if !options.allow_arrow_functions => Some(NoJsxPropsBindState {
invalid_kind: InvalidKind::ArrowFunction,
attribute_range: expression.range(),
}),
- AnyJsExpression::JsFunctionExpression(_) => Some(NoJsxPropsBindState {
+ AnyJsExpression::JsFunctionExpression(_) if !options.allow_functions => Some(NoJsxPropsBindState {
invalid_kind: InvalidKind::Function,
attribute_range: expression.range(),
}),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| match &expression { | |
| AnyJsExpression::JsArrowFunctionExpression(_) => Some(NoJsxPropsBindState { | |
| invalid_kind: InvalidKind::ArrowFunction, | |
| attribute_range: expression.range(), | |
| }), | |
| AnyJsExpression::JsFunctionExpression(_) => Some(NoJsxPropsBindState { | |
| invalid_kind: InvalidKind::Function, | |
| attribute_range: expression.range(), | |
| }), | |
| match &expression { | |
| AnyJsExpression::JsArrowFunctionExpression(_) if !options.allow_arrow_functions => Some(NoJsxPropsBindState { | |
| invalid_kind: InvalidKind::ArrowFunction, | |
| attribute_range: expression.range(), | |
| }), | |
| AnyJsExpression::JsFunctionExpression(_) if !options.allow_functions => Some(NoJsxPropsBindState { | |
| invalid_kind: InvalidKind::Function, | |
| attribute_range: expression.range(), | |
| }), |
🤖 Prompt for AI Agents
In crates/biome_js_analyze/src/lint/nursery/no_jsx_props_bind.rs around lines 74
to 83, the current match always flags arrow and function expressions; change
each arm to respect the linter options by returning Some(NoJsxPropsBindState {
... }) only when the corresponding option disallows that kind (i.e., for
AnyJsExpression::JsArrowFunctionExpression, only create the state if
!options.allow_arrow_functions; for AnyJsExpression::JsFunctionExpression, only
create the state if !options.allow_functions). Ensure you have access to the
options/config in this scope (add it as a parameter or capture it) and otherwise
fall through to None when the option allows the construct.
| AnyJsExpression::JsCallExpression(call) => { | ||
| // This will still throw a false positive on e.g. window.bind() | ||
| let is_bind = call | ||
| .callee() | ||
| .ok() | ||
| .and_then(|c| c.as_js_static_member_expression().cloned()) | ||
| .and_then(|m| m.member().ok()) | ||
| .and_then(|n| n.value_token().ok()) | ||
| .map(|t| t.text() == "bind") | ||
| .unwrap_or(false); | ||
| if is_bind { | ||
| Some(NoJsxPropsBindState { | ||
| invalid_kind: InvalidKind::Bind, | ||
| attribute_range: expression.range(), | ||
| }) | ||
| } else { | ||
| None | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
.bind() detection: handle chains/computed/optional and honour option
Covers only static members and ignores allow_bind. Also onClick={foo()} must not be flagged.
- AnyJsExpression::JsCallExpression(call) => {
- // This will still throw a false positive on e.g. window.bind()
- let is_bind = call
- .callee()
- .ok()
- .and_then(|c| c.as_js_static_member_expression().cloned())
- .and_then(|m| m.member().ok())
- .and_then(|n| n.value_token().ok())
- .map(|t| t.text() == "bind")
- .unwrap_or(false);
- if is_bind {
+ AnyJsExpression::JsCallExpression(call) => {
+ if options.allow_bind {
+ return None;
+ }
+ use biome_js_syntax::AnyJsMemberExpression;
+
+ // Unwrap optional chaining: (obj.bind)?.(...)
+ let callee = call.callee().ok().and_then(|c| {
+ if let Some(chain) = c.as_js_chain_expression() {
+ chain.expression().ok()
+ } else {
+ Some(c)
+ }
+ });
+
+ // Support static, computed, and optional members
+ let member = callee.as_ref().and_then(|c| c.as_js_member_expression().cloned());
+ let is_bind = member
+ .and_then(|m: AnyJsMemberExpression| m.member_name().ok())
+ .and_then(|n| n.value_token().ok())
+ .map(|t| t.text() == "bind")
+ .unwrap_or(false);
+
+ if is_bind {
Some(NoJsxPropsBindState {
invalid_kind: InvalidKind::Bind,
- attribute_range: expression.range(),
+ // Highlight the callee if present
+ attribute_range: callee.map(|c| c.range()).unwrap_or_else(|| expression.range()),
})
} else {
None
}
}Please also add tests:
- invalid:
<Foo onClick={obj["bind"](this)} />,<Foo onClick={obj.bind?.(this)} /> - valid:
<Foo onClick={foo()} />
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| AnyJsExpression::JsCallExpression(call) => { | |
| // This will still throw a false positive on e.g. window.bind() | |
| let is_bind = call | |
| .callee() | |
| .ok() | |
| .and_then(|c| c.as_js_static_member_expression().cloned()) | |
| .and_then(|m| m.member().ok()) | |
| .and_then(|n| n.value_token().ok()) | |
| .map(|t| t.text() == "bind") | |
| .unwrap_or(false); | |
| if is_bind { | |
| Some(NoJsxPropsBindState { | |
| invalid_kind: InvalidKind::Bind, | |
| attribute_range: expression.range(), | |
| }) | |
| } else { | |
| None | |
| } | |
| } | |
| AnyJsExpression::JsCallExpression(call) => { | |
| // Honor the `allow_bind` option first | |
| if options.allow_bind { | |
| return None; | |
| } | |
| use biome_js_syntax::AnyJsMemberExpression; | |
| // Unwrap optional chaining: (obj.bind)?.(...) | |
| let callee = call.callee().ok().and_then(|c| { | |
| if let Some(chain) = c.as_js_chain_expression() { | |
| chain.expression().ok() | |
| } else { | |
| Some(c) | |
| } | |
| }); | |
| // Support static, computed, and optional members | |
| let member = callee.as_ref().and_then(|c| c.as_js_member_expression().cloned()); | |
| let is_bind = member | |
| .and_then(|m: AnyJsMemberExpression| m.member_name().ok()) | |
| .and_then(|n| n.value_token().ok()) | |
| .map(|t| t.text() == "bind") | |
| .unwrap_or(false); | |
| if is_bind { | |
| Some(NoJsxPropsBindState { | |
| invalid_kind: InvalidKind::Bind, | |
| // Highlight the specific callee if we have it | |
| attribute_range: callee.map(|c| c.range()).unwrap_or_else(|| expression.range()), | |
| }) | |
| } else { | |
| None | |
| } | |
| } |
🤖 Prompt for AI Agents
crates/biome_js_analyze/src/lint/nursery/no_jsx_props_bind.rs around lines
84-102: the current .bind() detection only handles static member expressions and
misses chained/optional/computed calls and doesn’t respect the allow_bind
option; update the callee analysis to accept MemberExpression or
OptionalMemberExpression (including chained members), detect both static
property name "bind" and computed property access where the property is a string
literal "bind", and skip when the call is not a member call (so plain foo()
won’t be flagged); also read and honor the allow_bind configuration flag to
bypass this check when enabled; finally add tests for invalid cases `<Foo
onClick={obj["bind"](this)} />`, `<Foo onClick={obj.bind?.(this)} />` and a
valid case `<Foo onClick={foo()} />`.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
crates/biome_js_analyze/src/lint/nursery/no_jsx_props_bind.rs (4)
58-60: Switch to Semantic query to enable symbol resolution (needed for referenced functions).To support “is this a stable referenced function?” you’ll need access to the semantic model via ctx.model(). Change the query type to Semantic and import Semantic.
- use biome_analyze::{ - Ast, Rule, RuleDiagnostic, RuleDomain, RuleSource, context::RuleContext, declare_lint_rule, - }; + use biome_analyze::{ + Ast, Semantic, Rule, RuleDiagnostic, RuleDomain, RuleSource, context::RuleContext, declare_lint_rule, + }; @@ - type Query = Ast<JsxAttribute>; + type Query = Semantic<JsxAttribute>;Happy to wire a follow-up that resolves identifiers and checks stability (e.g. function declarations vs. useCallback/useMemo).
Also applies to: 1-3
64-73: Respect rule options early (ignore refs and DOM elements).Currently options aren’t consulted; the rule runs on every attribute. Short‑circuit per options before extracting the expression.
fn run(ctx: &RuleContext<Self>) -> Self::Signals { - let expression = ctx + let options = ctx.options(); + + // ignore ref attributes if configured + if options.ignore_refs { + if let Ok(name) = ctx.query().name() { + if name.text() == "ref" { + return None; + } + } + } + + // ignore lowercase (built-in) DOM components if configured + if options.ignore_dom_components { + if let Some(opening) = ctx + .query() + .syntax() + .ancestors() + .find_map(biome_js_syntax::JsxOpeningElement::cast) + { + if let Ok(token) = opening.name().and_then(|n| n.value_token()) { + if token.text().chars().next().is_some_and(|c| c.is_ascii_lowercase()) { + return None; + } + } + } + } + + let expression = ctx .query() .initializer()? .value() .ok()? .as_jsx_expression_attribute_value()? .expression() .ok()?;Note: this assumes corresponding fields exist on NoJsxPropsBindOptions (see separate note).
74-84: Gate arrow/function reports behind options.Allow users to opt in/out for arrows and function expressions.
- match &expression { - AnyJsExpression::JsArrowFunctionExpression(_) => Some(NoJsxPropsBindState { + match &expression { + AnyJsExpression::JsArrowFunctionExpression(_) if !options.allow_arrow_functions => Some(NoJsxPropsBindState { invalid_kind: InvalidKind::ArrowFunction, attribute_range: expression.range(), }), - AnyJsExpression::JsFunctionExpression(_) => Some(NoJsxPropsBindState { + AnyJsExpression::JsFunctionExpression(_) if !options.allow_functions => Some(NoJsxPropsBindState { invalid_kind: InvalidKind::Function, attribute_range: expression.range(), }),
85-101: Tighten .bind() detection, honour allow_bind, and support optional/computed members.Handle obj"bind", obj.bind?.(...), and avoid flagging plain calls. Also, highlight just the callee when possible.
- AnyJsExpression::JsCallExpression(call) => { - // This will still throw a false positive on e.g. window.bind() - let is_bind = call - .callee() - .ok() - .and_then(|c| c.as_js_static_member_expression().cloned()) - .and_then(|m| m.member().ok()) - .and_then(|n| n.value_token().ok()) - .map_or(false, |t| t.text() == "bind"); - if is_bind { - Some(NoJsxPropsBindState { - invalid_kind: InvalidKind::Bind, - attribute_range: expression.range(), - }) - } else { - None - } - } + AnyJsExpression::JsCallExpression(call) => { + if options.allow_bind { + return None; + } + use biome_js_syntax::AnyJsMemberExpression; + + // Unwrap optional chaining: (obj.bind)?.(...) + let callee = call.callee().ok().and_then(|c| { + if let Some(chain) = c.as_js_chain_expression() { + chain.expression().ok() + } else { + Some(c) + } + }); + + // Accept static/computed/optional member expressions + let member = callee.as_ref().and_then(|c| c.as_js_member_expression().cloned()); + let is_bind = member + .and_then(|m: AnyJsMemberExpression| m.member_name().ok()) + .and_then(|n| n.value_token().ok()) + .map(|t| t.text() == "bind") + .unwrap_or(false); + + if is_bind { + Some(NoJsxPropsBindState { + invalid_kind: InvalidKind::Bind, + attribute_range: callee.as_ref().map(|c| c.range()).unwrap_or_else(|| expression.range()), + }) + } else { + None + } + }Please add tests for:
- invalid: <Foo onClick={obj"bind"} />,
- valid:
🧹 Nitpick comments (2)
crates/biome_js_analyze/src/lint/nursery/no_jsx_props_bind.rs (2)
106-121: Enrich diagnostic with attribute name; keep message concise.Naming the prop improves UX and aligns with Biome’s rule pillars.
- fn diagnostic(_ctx: &RuleContext<Self>, state: &Self::State) -> Option<RuleDiagnostic> { + fn diagnostic(ctx: &RuleContext<Self>, state: &Self::State) -> Option<RuleDiagnostic> { + let prop = ctx.query().name().ok().map(|n| n.text()).unwrap_or("prop".into()); let note = match state.invalid_kind { InvalidKind::ArrowFunction => "JSX props should not use arrow functions", InvalidKind::Bind => "JSX props should not use .bind()", InvalidKind::Function => "JSX props should not use function expressions", }; Some( RuleDiagnostic::new( rule_category!(), state.attribute_range, - "Pass stable function references as props to avoid unnecessary rerenders.", + format!("Avoid inline binding in JSX: `{prop}` should receive a stable function reference."), ) .note(note) .note("Consider extracting the function or wrapping it in useCallback"), ) }
64-104: Roadmap note: resolving referenced functions for “stability”.To answer the open TODO (“is onClick={onClick} stable?”), resolve the identifier and classify:
- Function declaration/expression defined outside render scope → stable
- useCallback(...) result → stable
- Class method reference → stable
- Inline/closure capturing render locals → unstable
Sketch (outside this diff): use ctx.model() to resolve AnyJsExpression::JsReferenceIdentifier to a symbol, get its declaration node, and walk upwards to see if it’s top-level/module/class or React hook. I can draft this if helpful.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/@biomejs/backend-jsonrpc/src/workspace.tsis excluded by!**/backend-jsonrpc/src/workspace.tsand included by**
📒 Files selected for processing (1)
crates/biome_js_analyze/src/lint/nursery/no_jsx_props_bind.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{rs,toml}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Format Rust and TOML files before committing (use
just f/just format).
Files:
crates/biome_js_analyze/src/lint/nursery/no_jsx_props_bind.rs
crates/biome_*_{syntax,parser,formatter,analyze,factory,semantic}/**
📄 CodeRabbit inference engine (CLAUDE.md)
Maintain the per-language crate structure: biome_{lang}_{syntax,parser,formatter,analyze,factory,semantic}
Files:
crates/biome_js_analyze/src/lint/nursery/no_jsx_props_bind.rs
crates/biome_*/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place core crates under /crates/biome_*/
Files:
crates/biome_js_analyze/src/lint/nursery/no_jsx_props_bind.rs
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-09-05T09:13:58.901Z
Learning: Applies to crates/biome_analyze/crates/biome_js_analyze/lib/src/lint/nursery/**/*.rs : Place all new rules in the nursery group under biome_js_analyze/lib/src/lint/nursery
📚 Learning: 2025-09-05T09:13:58.901Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-09-05T09:13:58.901Z
Learning: Applies to crates/biome_analyze/crates/biome_js_analyze/**/src/**/*.rs : When deprecating a rule, add deprecated: "<reason>" to declare_lint_rule!
Applied to files:
crates/biome_js_analyze/src/lint/nursery/no_jsx_props_bind.rs
📚 Learning: 2025-09-05T09:13:58.901Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-09-05T09:13:58.901Z
Learning: Applies to crates/biome_analyze/crates/biome_js_analyze/**/src/**/*.rs : Avoid unnecessary string allocations in rules; compare against &str or TokenText instead of calling to_string()
Applied to files:
crates/biome_js_analyze/src/lint/nursery/no_jsx_props_bind.rs
🧬 Code graph analysis (1)
crates/biome_js_analyze/src/lint/nursery/no_jsx_props_bind.rs (1)
crates/biome_analyze/src/rule.rs (4)
recommended(554-557)sources(569-572)inspired(249-254)domains(584-587)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
crates/biome_js_analyze/src/lint/nursery/no_jsx_props_bind.rs (3)
65-74: Respect options early: ignore refs and DOM componentsConsult options before extracting the expression, skipping attributes when configured (mirrors ESLint).
fn run(ctx: &RuleContext<Self>) -> Self::Signals { - let expression: AnyJsExpression = ctx + let options = ctx.options(); + + // Ignore `ref` attributes if configured + if options.ignore_refs { + if let Ok(name) = ctx.query().name() { + if name.text() == "ref" { + return None; + } + } + } + + // Ignore lowercase DOM components if configured + if options.ignore_dom_components { + if let Some(opening) = ctx + .query() + .syntax() + .ancestors() + .find_map(biome_js_syntax::JsxOpeningElement::cast) + { + if let Ok(token) = opening.name().and_then(|n| n.value_token()) { + if token.text().chars().next().is_some_and(|c| c.is_ascii_lowercase()) { + return None; + } + } + } + } + + let expression: AnyJsExpression = ctx .query() .initializer()? .value() .ok()? .as_jsx_expression_attribute_value()? .expression() .ok()?;
75-84: Gate arrow/function matches behind optionsAllow users to enable arrow/functions if desired.
- AnyJsExpression::JsArrowFunctionExpression(_) => Some(NoJsxPropsBindState { + AnyJsExpression::JsArrowFunctionExpression(_) if !ctx.options().allow_arrow_functions => Some(NoJsxPropsBindState { invalid_kind: InvalidKind::ArrowFunction, attribute_range: expression.range(), }), - AnyJsExpression::JsFunctionExpression(_) => Some(NoJsxPropsBindState { + AnyJsExpression::JsFunctionExpression(_) if !ctx.options().allow_functions => Some(NoJsxPropsBindState { invalid_kind: InvalidKind::Function, attribute_range: expression.range(), }),
85-102: .bind() detection: support computed/optional members and honour optionCovers only static members and ignores
allow_bind. Add optional/chain support and skip non-member calls.- AnyJsExpression::JsCallExpression(call) => { - // This will still throw a false positive on e.g. window.bind() - let is_bind = call - .callee() - .ok() - .and_then(|c| c.as_js_static_member_expression().cloned()) - .and_then(|m| m.member().ok()) - .and_then(|n| n.value_token().ok()) - .map_or(false, |t| t.text() == "bind"); - if is_bind { - Some(NoJsxPropsBindState { - invalid_kind: InvalidKind::Bind, - attribute_range: expression.range(), - }) - } else { - None - } - } + AnyJsExpression::JsCallExpression(call) => { + if ctx.options().allow_bind { + return None; + } + use biome_js_syntax::AnyJsMemberExpression; + + // Unwrap optional chaining: (obj.bind)?.(...) + let callee = call + .callee() + .ok() + .and_then(|c| if let Some(chain) = c.as_js_chain_expression() { chain.expression().ok() } else { Some(c) }); + + let member = callee.as_ref().and_then(|c| c.as_js_member_expression().cloned()); + let is_bind = member + .and_then(|m: AnyJsMemberExpression| m.member_name().ok()) + .and_then(|n| n.value_token().ok()) + .map(|t| t.text() == "bind") + .unwrap_or(false); + + if is_bind { + Some(NoJsxPropsBindState { + invalid_kind: InvalidKind::Bind, + // Highlight callee if available (e.g. `obj.bind`) + attribute_range: callee.map(|c| c.range()).unwrap_or_else(|| expression.range()), + }) + } else { + None + } + }Add tests:
- invalid:
onClick={obj["bind"](this)},onClick={obj.bind?.(this)}- valid:
onClick={foo()}
🧹 Nitpick comments (5)
crates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/invalid.jsx (3)
1-10: Add edge cases for .bind() and a control for plain callsPlease add:
- invalid:
<Foo onClick={obj["bind"](this)} />,<Foo onClick={obj.bind?.(this)} />- valid:
<Foo onClick={foo()} />(should not be flagged)These will guard against false positives and missed
.bind()variants.Happy to open a quick PR with these fixtures.
12-25: Cover arrow-function components (current logic likely misses them)Add cases like:
- invalid:
const Foo = () => { const onClick = () => {}; return <Bar onClick={onClick} /> }- invalid:
const Foo = () => { function onClick() {}; return <Bar onClick={onClick} /> }The implementation checks only for JS_FUNCTION_DECLARATION ancestors, so arrow/function-expression components are currently slipping through. Tests will expose that.
I can add these and wire snapshots.
12-25: Avoid reusing the same component name thriceMinor: redefining
function Foo()three times is a bit noisy for readers. Consider renaming toFooDecl,FooArrowVar,FooFuncVar.crates/biome_js_analyze/src/lint/nursery/no_jsx_props_bind.rs (2)
109-113: Remove leftover debug commentsThe
dbg!comments can go; they distract in production rule code.- // dbg!(binding.syntax().kind()); - // dbg!(identifier.syntax().kind()); - // dbg!(declaration.syntax().kind());
154-169: Diagnostic polish: note can suggest useCallback with depsMinor copy tweak: “Consider extracting the function or wrapping it in useCallback with an appropriate dependency list.” This mirrors React guidance.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
crates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/invalid.jsx.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/valid.jsx.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (3)
crates/biome_js_analyze/src/lint/nursery/no_jsx_props_bind.rs(1 hunks)crates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/invalid.jsx(1 hunks)crates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/valid.jsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/valid.jsx
🧰 Additional context used
📓 Path-based instructions (4)
crates/biome_*_{syntax,parser,formatter,analyze,factory,semantic}/**
📄 CodeRabbit inference engine (CLAUDE.md)
Maintain the per-language crate structure: biome_{lang}_{syntax,parser,formatter,analyze,factory,semantic}
Files:
crates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/invalid.jsxcrates/biome_js_analyze/src/lint/nursery/no_jsx_props_bind.rs
crates/biome_*/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place core crates under /crates/biome_*/
Files:
crates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/invalid.jsxcrates/biome_js_analyze/src/lint/nursery/no_jsx_props_bind.rs
**/tests/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place test files under a tests/ directory in each crate
Files:
crates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/invalid.jsx
**/*.{rs,toml}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Format Rust and TOML files before committing (use
just f/just format).
Files:
crates/biome_js_analyze/src/lint/nursery/no_jsx_props_bind.rs
🧠 Learnings (10)
📓 Common learnings
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-09-05T09:13:58.901Z
Learning: Applies to crates/biome_analyze/crates/biome_js_analyze/lib/src/lint/nursery/**/*.rs : Place all new rules in the nursery group under biome_js_analyze/lib/src/lint/nursery
📚 Learning: 2025-09-05T09:13:58.901Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-09-05T09:13:58.901Z
Learning: Applies to crates/biome_analyze/crates/biome_js_analyze/**/src/**/*.rs : When deprecating a rule, add deprecated: "<reason>" to declare_lint_rule!
Applied to files:
crates/biome_js_analyze/src/lint/nursery/no_jsx_props_bind.rs
📚 Learning: 2025-09-05T09:13:58.901Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-09-05T09:13:58.901Z
Learning: Applies to crates/biome_analyze/crates/biome_js_analyze/lib/src/lint/nursery/**/*.rs : Place all new rules in the nursery group under biome_js_analyze/lib/src/lint/nursery
Applied to files:
crates/biome_js_analyze/src/lint/nursery/no_jsx_props_bind.rs
📚 Learning: 2025-09-05T09:13:58.901Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-09-05T09:13:58.901Z
Learning: Applies to crates/biome_analyze/crates/biome_js_analyze/**/src/**/*.rs : Avoid unnecessary string allocations in rules; compare against &str or TokenText instead of calling to_string()
Applied to files:
crates/biome_js_analyze/src/lint/nursery/no_jsx_props_bind.rs
📚 Learning: 2025-09-05T09:13:58.901Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-09-05T09:13:58.901Z
Learning: Applies to crates/biome_analyze/crates/biome_js_analyze/**/src/**/*.rs : If a rule provides a code action, declare fix_kind in declare_lint_rule!
Applied to files:
crates/biome_js_analyze/src/lint/nursery/no_jsx_props_bind.rs
📚 Learning: 2025-09-05T09:13:58.901Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-09-05T09:13:58.901Z
Learning: Applies to crates/biome_analyze/crates/biome_js_analyze/**/src/**/*.rs : Set the language field in declare_lint_rule! to the most specific applicable language (js/jsx/ts/tsx)
Applied to files:
crates/biome_js_analyze/src/lint/nursery/no_jsx_props_bind.rs
📚 Learning: 2025-09-05T09:13:58.901Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-09-05T09:13:58.901Z
Learning: Applies to crates/biome_analyze/crates/biome_js_analyze/**/src/**/*.rs : When porting a lint rule, set sources in declare_lint_rule! (RuleSource::Eslint(...).same() or .inspired())
Applied to files:
crates/biome_js_analyze/src/lint/nursery/no_jsx_props_bind.rs
📚 Learning: 2025-09-05T09:13:58.901Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-09-05T09:13:58.901Z
Learning: Applies to crates/biome_analyze/crates/biome_rule_options/lib/**/*.rs : Define per‑rule options in the biome_rule_options crate (one file per rule, e.g., use_my_rule.rs)
Applied to files:
crates/biome_js_analyze/src/lint/nursery/no_jsx_props_bind.rs
📚 Learning: 2025-09-05T09:13:58.901Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-09-05T09:13:58.901Z
Learning: Applies to crates/biome_analyze/crates/biome_rule_options/lib/**/*.rs : Derive Serialize, Deserialize, Deserializable (and schemars JsonSchema behind feature) on options types; use serde attributes rename_all="camelCase", deny_unknown_fields, default
Applied to files:
crates/biome_js_analyze/src/lint/nursery/no_jsx_props_bind.rs
📚 Learning: 2025-09-05T09:13:58.901Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-09-05T09:13:58.901Z
Learning: Applies to crates/biome_analyze/crates/biome_js_analyze/**/src/**/*.rs : In declare_lint_rule! metadata, set version: "next"
Applied to files:
crates/biome_js_analyze/src/lint/nursery/no_jsx_props_bind.rs
| AnyJsExpression::JsIdentifierExpression(identifier) => { | ||
| let model = ctx.model(); | ||
| let binding: Binding = model.binding(&identifier.name().ok()?)?; | ||
|
|
||
| let declaration = binding.tree().declaration()?; | ||
|
|
||
| // dbg!(binding.syntax().kind()); | ||
| // dbg!(identifier.syntax().kind()); | ||
| // dbg!(declaration.syntax().kind()); | ||
|
|
||
| match &declaration { | ||
| AnyJsBindingDeclaration::JsFunctionDeclaration(_) => { | ||
| // Global functions are fine. | ||
| // This is probably overly simplistic | ||
| // Also I don't understand why I need to skip the first ancestor | ||
| // It seems like the first ancestor of a function declaration is itself a | ||
| // function declaration?? | ||
| if !declaration.syntax().ancestors().skip(1).any(|anc| { | ||
| anc.kind() == biome_js_syntax::JsSyntaxKind::JS_FUNCTION_DECLARATION | ||
| }) { | ||
| return None; | ||
| } | ||
| return Some(NoJsxPropsBindState { | ||
| invalid_kind: InvalidKind::Function, | ||
| attribute_range: expression.range(), | ||
| }); | ||
| } | ||
| AnyJsBindingDeclaration::JsVariableDeclarator(variable_declarator) => { | ||
| // dbg!("It's some kind of variable!"); | ||
| // dbg!(&variable_declarator); | ||
| match variable_declarator.initializer()?.expression().ok()? { | ||
| AnyJsExpression::JsFunctionExpression(_) | ||
| | AnyJsExpression::JsArrowFunctionExpression(_) => { | ||
| if !declaration.syntax().ancestors().any(|anc| anc.kind() == biome_js_syntax::JsSyntaxKind::JS_FUNCTION_DECLARATION) { | ||
| return None | ||
| } | ||
| return Some(NoJsxPropsBindState { | ||
| invalid_kind: InvalidKind::Function, | ||
| attribute_range: expression.range(), | ||
| }); | ||
| } | ||
| _ => None, | ||
| } | ||
| } | ||
| _ => None, | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Identifier path: misses arrow/function components; fix function-like ancestor check
The ancestor check looks only for JS_FUNCTION_DECLARATION; arrow/function-expression components won’t be flagged. Also the skip(1) hack is brittle. Check for any function-like ancestor.
- AnyJsBindingDeclaration::JsFunctionDeclaration(_) => {
- // Global functions are fine.
- // This is probably overly simplistic
- // Also I don't understand why I need to skip the first ancestor
- // It seems like the first ancestor of a function declaration is itself a
- // function declaration??
- if !declaration.syntax().ancestors().skip(1).any(|anc| {
- anc.kind() == biome_js_syntax::JsSyntaxKind::JS_FUNCTION_DECLARATION
- }) {
- return None;
- }
- return Some(NoJsxPropsBindState {
- invalid_kind: InvalidKind::Function,
- attribute_range: expression.range(),
- });
- }
+ AnyJsBindingDeclaration::JsFunctionDeclaration(_) => {
+ use biome_js_syntax::JsSyntaxKind::*;
+ // Valid only when declared at module scope (no function-like ancestor beyond itself).
+ let is_nested_in_fn_like = declaration
+ .syntax()
+ .ancestors()
+ .skip(1)
+ .any(|a| matches!(a.kind(), JS_FUNCTION_DECLARATION | JS_FUNCTION_EXPRESSION | JS_ARROW_FUNCTION_EXPRESSION));
+ if is_nested_in_fn_like {
+ Some(NoJsxPropsBindState { invalid_kind: InvalidKind::Function, attribute_range: expression.range() })
+ } else {
+ None
+ }
+ }
@@
- AnyJsBindingDeclaration::JsVariableDeclarator(variable_declarator) => {
+ AnyJsBindingDeclaration::JsVariableDeclarator(variable_declarator) => {
// dbg!("It's some kind of variable!");
// dbg!(&variable_declarator);
- match variable_declarator.initializer()?.expression().ok()? {
+ match variable_declarator.initializer()?.expression().ok()? {
AnyJsExpression::JsFunctionExpression(_)
| AnyJsExpression::JsArrowFunctionExpression(_) => {
- if !declaration.syntax().ancestors().any(|anc| anc.kind() == biome_js_syntax::JsSyntaxKind::JS_FUNCTION_DECLARATION) {
- return None
- }
- return Some(NoJsxPropsBindState {
- invalid_kind: InvalidKind::Function,
- attribute_range: expression.range(),
- });
+ use biome_js_syntax::JsSyntaxKind::*;
+ let is_inside_fn_like = declaration
+ .syntax()
+ .ancestors()
+ .any(|a| matches!(a.kind(), JS_FUNCTION_DECLARATION | JS_FUNCTION_EXPRESSION | JS_ARROW_FUNCTION_EXPRESSION));
+ if is_inside_fn_like {
+ Some(NoJsxPropsBindState { invalid_kind: InvalidKind::Function, attribute_range: expression.range() })
+ } else {
+ None
+ }
}
_ => None,
}
}Please also add tests for arrow-function components as noted in the fixture comment.
🤖 Prompt for AI Agents
In crates/biome_js_analyze/src/lint/nursery/no_jsx_props_bind.rs around lines
103 to 149, the current ancestor checks only detect JS_FUNCTION_DECLARATION (and
uses a brittle skip(1)) so arrow/function expression components are missed;
change the logic to look for any function-like ancestor
(JS_FUNCTION_DECLARATION, JS_FUNCTION_EXPRESSION, JS_ARROW_FUNCTION_EXPRESSION)
using a single ancestors().any(...) check (remove skip(1) usage) in both the
declaration and variable_declarator branches, and ensure the same
InvalidKind::Function path is returned when any of those ancestors are found;
additionally add unit/fixture tests covering arrow-function components as
indicated by the fixture comment.
|
Okay, made some progress. The code to check if a variable is global is almost certainly wrong, definitely hacky, I know that much. |
| // It seems like the first ancestor of a function declaration is itself a | ||
| // function declaration?? |
There was a problem hiding this comment.
ancestors() includes the node itself, so this is expected.
| if !declaration.syntax().ancestors().skip(1).any(|anc| { | ||
| anc.kind() == biome_js_syntax::JsSyntaxKind::JS_FUNCTION_DECLARATION | ||
| }) { | ||
| return None; | ||
| } |
There was a problem hiding this comment.
I'm assuming this is the globals check? You should be consulting the semantic model for this info. Global references have no bindings that they map to. Take a look at noConsoleLog for a good example.
There was a problem hiding this comment.
Hmm, maybe global isn't quite the correct term here, or I am still missing something. I don't mean to check where anything is defined on window, which it looks likes no_console is doing. I'm just looking if the function was declared outside a component
const onGlobal = function () {};
function Bar() {
const onLocal = function () {};
return <Foo onClick={onGlobal} onMouseDown={onLocal} />
}In this snippet, onGlobal is fine, but onLocal is not.
I tried adapting the code from no_console:
let (reference, name) = global_identifier(&expression)?;
let is_global = model.binding(&reference).is_none();
dbg!(name, is_global);But this returns false for both onGlobal and onLocal
There was a problem hiding this comment.
If you aren't looking for any globals, then you don't need the function global_identifier. Just use model.binding by passing the correct reference.
There was a problem hiding this comment.
let is_global = model
.binding(&expression.as_js_reference_identifier()?)
.is_none();
dbg!(is_global);This fits the types, but also returns false for both cases.
There was a problem hiding this comment.
Yeah, I fear I'm stuck on this one. Anything I can think of to pass to model.binding is never none. I would need some more help here.
There was a problem hiding this comment.
Then it's fine if we don't cover this use case for now. We can merge it now and come back later. A nursery rule doesn't have to be perfect at the first try. You can keep the original task open and leave a comment saying what's left after we merge this one. What do you think @sgarcialaguna ?
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (6)
crates/biome_js_analyze/src/lint/nursery/no_jsx_props_bind.rs (6)
38-45: Wire options schema in biome_rule_options (separate file)Your rule reads
NoJsxPropsBindOptionsbut doesn’t use fields unless they exist. Define them to match eslint flexibility.Outside this file, add in
crates/biome_rule_options/src/no_jsx_props_bind.rs:use biome_analyze::RuleOptions; use biome_deserialize_macros::Merge; use serde::{Deserialize, Serialize}; #[derive(Debug, Clone, Default, Serialize, Deserialize, Merge, RuleOptions)] #[serde(default, rename_all = "camelCase", deny_unknown_fields)] pub struct NoJsxPropsBindOptions { pub allow_arrow_functions: bool, pub allow_functions: bool, pub allow_bind: bool, pub ignore_refs: bool, pub ignore_dom_components: bool, }And expose the module in
crates/biome_rule_options/src/lib.rs:pub mod no_jsx_props_bind;Happy to open a tiny follow‑up PR wiring docs and snapshots.
75-85: Gate arrow/function reports behind optionsHonour
allow_arrow_functionsandallow_functions.- match &expression { - AnyJsExpression::JsArrowFunctionExpression(_) => Some(NoJsxPropsBindState { + match &expression { + AnyJsExpression::JsArrowFunctionExpression(_) if !options.allow_arrow_functions => Some(NoJsxPropsBindState { invalid_kind: InvalidKind::ArrowFunction, attribute_range: expression.range(), }), - AnyJsExpression::JsFunctionExpression(_) => Some(NoJsxPropsBindState { + AnyJsExpression::JsFunctionExpression(_) if !options.allow_functions => Some(NoJsxPropsBindState { invalid_kind: InvalidKind::Function, attribute_range: expression.range(), }),
133-145: Variable declarator path: include arrow/function components, mirror logicSymmetry with the function‑declaration branch; treat any function‑like ancestor as “local”.
- match variable_declarator.initializer()?.expression().ok()? { + match variable_declarator.initializer()?.expression().ok()? { AnyJsExpression::JsFunctionExpression(_) | AnyJsExpression::JsArrowFunctionExpression(_) => { - if !declaration.syntax().ancestors().any(|anc| anc.kind() == biome_js_syntax::JsSyntaxKind::JS_FUNCTION_DECLARATION) { - return None - } - Some(NoJsxPropsBindState { - invalid_kind: InvalidKind::Function, - attribute_range: expression.range(), - }) + use biome_js_syntax::JsSyntaxKind::*; + let inside_fn_like = declaration + .syntax() + .ancestors() + .any(|a| matches!(a.kind(), JS_FUNCTION_DECLARATION | JS_FUNCTION_EXPRESSION | JS_ARROW_FUNCTION_EXPRESSION)); + if inside_fn_like { + Some(NoJsxPropsBindState { + invalid_kind: InvalidKind::Function, + attribute_range: expression.range(), + }) + } else { + None + } } _ => None, }
65-75: Respect options early; skip refs and DOM componentsShort‑circuit per eslint parity: ignore
refattributes and built‑in DOM components when configured.- fn run(ctx: &RuleContext<Self>) -> Self::Signals { - let expression: AnyJsExpression = ctx + fn run(ctx: &RuleContext<Self>) -> Self::Signals { + let options = ctx.options(); + + // Ignore ref attributes if configured + if options.ignore_refs { + if let Ok(name) = ctx.query().name() { + if name.text() == "ref" { + return None; + } + } + } + + // Ignore lowercase (built-in) DOM components if configured + if options.ignore_dom_components { + if let Some(opening) = ctx + .query() + .syntax() + .ancestors() + .find_map(JsxOpeningElement::cast) + { + if let Ok(token) = opening.name().and_then(|n| n.value_token()) { + if token.text().chars().next().is_some_and(|c| c.is_ascii_lowercase()) { + return None; + } + } + } + } + + let expression: AnyJsExpression = ctx .query() .initializer()? .value() .ok()? .as_jsx_expression_attribute_value()? .expression() .ok()?;
113-129: Fix “global vs local” check; consider any function‑like ancestor (no skip hack)This currently only checks
JS_FUNCTION_DECLARATIONand uses a brittleskip(1). Include arrow/function expressions and drop the hack.- AnyJsBindingDeclaration::JsFunctionDeclaration(_) => { - // Global functions are fine. - // This is probably overly simplistic - // Also I don't understand why I need to skip the first ancestor - // It seems like the first ancestor of a function declaration is itself a - // function declaration?? - if !declaration.syntax().ancestors().skip(1).any(|anc| { - anc.kind() == biome_js_syntax::JsSyntaxKind::JS_FUNCTION_DECLARATION - }) { - return None; - } - return Some(NoJsxPropsBindState { - invalid_kind: InvalidKind::Function, - attribute_range: expression.range(), - }); - } + AnyJsBindingDeclaration::JsFunctionDeclaration(_) => { + use biome_js_syntax::JsSyntaxKind::*; + // Flag only when declared inside a function-like (component boundary). + let is_nested_in_fn_like = declaration + .syntax() + .ancestors() + .skip(1) // exclude the declaration itself + .any(|a| matches!(a.kind(), JS_FUNCTION_DECLARATION | JS_FUNCTION_EXPRESSION | JS_ARROW_FUNCTION_EXPRESSION)); + if is_nested_in_fn_like { + Some(NoJsxPropsBindState { + invalid_kind: InvalidKind::Function, + attribute_range: expression.range(), + }) + } else { + None + } + }
85-102: .bind() detection: handle optional/computed members and honour optionOnly flag member calls named “bind”; support
obj["bind"](…)and optional chaining.- AnyJsExpression::JsCallExpression(call) => { - // This will still throw a false positive on e.g. window.bind() - let is_bind = call - .callee() - .ok() - .and_then(|c| c.as_js_static_member_expression().cloned()) - .and_then(|m| m.member().ok()) - .and_then(|n| n.value_token().ok()) - .map_or(false, |t| t.text() == "bind"); - if is_bind { - Some(NoJsxPropsBindState { - invalid_kind: InvalidKind::Bind, - attribute_range: expression.range(), - }) - } else { - None - } - } + AnyJsExpression::JsCallExpression(call) => { + if options.allow_bind { + return None; + } + // Unwrap optional chain: (obj.bind)?.(...) + let callee = call + .callee() + .ok() + .and_then(|c| { + if let Some(chain) = c.as_js_chain_expression() { + chain.expression().ok() + } else { + Some(c) + } + }); + + let member = callee.as_ref().and_then(|c| c.as_js_member_expression().cloned()); + let is_bind = member + .and_then(|m: AnyJsMemberExpression| m.member_name().ok()) + .and_then(|n| n.value_token().ok()) + .map(|t| t.text() == "bind") + .unwrap_or(false); + + if is_bind { + Some(NoJsxPropsBindState { + invalid_kind: InvalidKind::Bind, + // Highlight just the callee when available + attribute_range: callee.map(|c| c.range()).unwrap_or_else(|| expression.range()), + }) + } else { + None + } + }Add tests:
- invalid:
<Foo onClick={obj["bind"](this)} />,<Foo onClick={obj.bind?.(this)} />- valid:
<Foo onClick={foo()} />
🧹 Nitpick comments (3)
crates/biome_js_analyze/src/lint/nursery/no_jsx_props_bind.rs (3)
5-5: Import the JSX/member helpers you use in the ruleYou reference JSX opening elements and member expressions in suggested logic; import them here.
-use biome_js_syntax::{AnyJsExpression, JsxAttribute, binding_ext::AnyJsBindingDeclaration}; +use biome_js_syntax::{ + AnyJsExpression, + JsxAttribute, + JsxOpeningElement, + AnyJsMemberExpression, + binding_ext::AnyJsBindingDeclaration, +};
154-169: Diagnostic polish: include the prop name for contextSmall DX win: show which prop triggered the diagnostic.
- fn diagnostic(_ctx: &RuleContext<Self>, state: &Self::State) -> Option<RuleDiagnostic> { + fn diagnostic(ctx: &RuleContext<Self>, state: &Self::State) -> Option<RuleDiagnostic> { + let prop = ctx.query().name().ok().map(|n| n.text()).unwrap_or("prop".into()); let note = match state.invalid_kind { InvalidKind::ArrowFunction => "JSX props should not use arrow functions", InvalidKind::Bind => "JSX props should not use .bind()", InvalidKind::Function => "JSX props should not use function expressions", }; Some( RuleDiagnostic::new( rule_category!(), state.attribute_range, - "Pass stable function references as props to avoid unnecessary rerenders.", + format!("Pass stable function references in `{prop}` to avoid unnecessary rerenders."), ) .note(note) .note("Consider extracting the function or wrapping it in useCallback"), ) }
85-95: Remove misleading comment“false positive on window.bind()” isn’t actionable here. Let’s drop it to avoid confusion.
- // This will still throw a false positive on e.g. window.bind()
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/biome_js_analyze/src/lint/nursery/no_jsx_props_bind.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
crates/biome_*_{syntax,parser,formatter,analyze,factory,semantic}/**
📄 CodeRabbit inference engine (CLAUDE.md)
Maintain the per-language crate structure: biome_{lang}_{syntax,parser,formatter,analyze,factory,semantic}
Files:
crates/biome_js_analyze/src/lint/nursery/no_jsx_props_bind.rs
crates/biome_*/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place core crates under /crates/biome_*/
Files:
crates/biome_js_analyze/src/lint/nursery/no_jsx_props_bind.rs
**/*.rs
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Format all Rust source files before committing (just f)
Files:
crates/biome_js_analyze/src/lint/nursery/no_jsx_props_bind.rs
🧠 Learnings (10)
📓 Common learnings
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-09-05T09:13:58.901Z
Learning: Applies to crates/biome_analyze/crates/biome_js_analyze/lib/src/lint/nursery/**/*.rs : Place all new rules in the nursery group under biome_js_analyze/lib/src/lint/nursery
📚 Learning: 2025-09-05T09:13:58.901Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-09-05T09:13:58.901Z
Learning: Applies to crates/biome_analyze/crates/biome_js_analyze/**/src/**/*.rs : In declare_lint_rule! metadata, set version: "next"
Applied to files:
crates/biome_js_analyze/src/lint/nursery/no_jsx_props_bind.rs
📚 Learning: 2025-09-05T09:13:58.901Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-09-05T09:13:58.901Z
Learning: Applies to crates/biome_analyze/crates/biome_js_analyze/**/src/**/*.rs : When deprecating a rule, add deprecated: "<reason>" to declare_lint_rule!
Applied to files:
crates/biome_js_analyze/src/lint/nursery/no_jsx_props_bind.rs
📚 Learning: 2025-09-05T09:13:58.901Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-09-05T09:13:58.901Z
Learning: Applies to crates/biome_analyze/crates/biome_js_analyze/**/src/**/*.rs : If a rule provides a code action, declare fix_kind in declare_lint_rule!
Applied to files:
crates/biome_js_analyze/src/lint/nursery/no_jsx_props_bind.rs
📚 Learning: 2025-09-05T09:13:58.901Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-09-05T09:13:58.901Z
Learning: Applies to crates/biome_analyze/crates/biome_js_analyze/lib/src/lint/nursery/**/*.rs : Place all new rules in the nursery group under biome_js_analyze/lib/src/lint/nursery
Applied to files:
crates/biome_js_analyze/src/lint/nursery/no_jsx_props_bind.rs
📚 Learning: 2025-09-05T09:13:58.901Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-09-05T09:13:58.901Z
Learning: Applies to crates/biome_analyze/crates/biome_js_analyze/**/src/**/*.rs : Set the language field in declare_lint_rule! to the most specific applicable language (js/jsx/ts/tsx)
Applied to files:
crates/biome_js_analyze/src/lint/nursery/no_jsx_props_bind.rs
📚 Learning: 2025-09-05T09:13:58.901Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-09-05T09:13:58.901Z
Learning: Applies to crates/biome_analyze/crates/biome_js_analyze/**/src/**/*.rs : Avoid unnecessary string allocations in rules; compare against &str or TokenText instead of calling to_string()
Applied to files:
crates/biome_js_analyze/src/lint/nursery/no_jsx_props_bind.rs
📚 Learning: 2025-09-05T09:13:58.901Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-09-05T09:13:58.901Z
Learning: Applies to crates/biome_analyze/crates/biome_js_analyze/**/src/**/*.rs : When porting a lint rule, set sources in declare_lint_rule! (RuleSource::Eslint(...).same() or .inspired())
Applied to files:
crates/biome_js_analyze/src/lint/nursery/no_jsx_props_bind.rs
📚 Learning: 2025-09-05T09:13:58.901Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-09-05T09:13:58.901Z
Learning: Applies to crates/biome_analyze/crates/biome_rule_options/lib/**/*.rs : Define per‑rule options in the biome_rule_options crate (one file per rule, e.g., use_my_rule.rs)
Applied to files:
crates/biome_js_analyze/src/lint/nursery/no_jsx_props_bind.rs
📚 Learning: 2025-09-05T09:13:58.901Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-09-05T09:13:58.901Z
Learning: Applies to crates/biome_analyze/crates/biome_rule_options/lib/**/*.rs : Derive Serialize, Deserialize, Deserializable (and schemars JsonSchema behind feature) on options types; use serde attributes rename_all="camelCase", deny_unknown_fields, default
Applied to files:
crates/biome_js_analyze/src/lint/nursery/no_jsx_props_bind.rs
| let note = match state.invalid_kind { | ||
| InvalidKind::ArrowFunction => "JSX props should not use arrow functions", | ||
| InvalidKind::Bind => "JSX props should not use .bind()", | ||
| InvalidKind::Function => "JSX props should not use function expressions", | ||
| }; | ||
| Some( | ||
| RuleDiagnostic::new( | ||
| rule_category!(), | ||
| state.attribute_range, | ||
| "Pass stable function references as props to avoid unnecessary rerenders.", | ||
| ) | ||
| .note(note) | ||
| .note("Consider extracting the function or wrapping it in useCallback"), | ||
| ) |
There was a problem hiding this comment.
The message should explain that the flagged code creates a new function every render.
dyc3
left a comment
dyc3
left a comment
There was a problem hiding this comment.
Sorry it's been so long, this one slipped through the cracks. But, this needs a changeset, and a rebase to fix the merge conflicts (you can just run the codegen to fix them). Still interested in this PR?
|
No worries, I've been dealing with some real-life stuff. I'm still interested, I'll look to get it done towards the weekend. |
I got you ;) |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.changeset/many-candies-see.md (1)
5-11: Consider adding a valid example to complete the guidance.The changeset clearly describes the rule and shows an invalid pattern. However, including a valid example (e.g., extracting the function or using
useCallback) would help users understand the recommended approach and provide better contrast.Suggested enhancement
**Invalid:** ```jsx <Foo onClick={() => console.log('Hello!')}></Foo>
+Valid:
+
+jsx +const handleClick = () => console.log('Hello!'); +<Foo onClick={handleClick}></Foo> +Alternatively, if space is a concern, you could keep it brief with just the extracted function approach shown. </details> </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: Path: .coderabbit.yaml **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 72f79c33a3a27fad106a01a636c604d612c5dd67 and 702b11f235f026233482c744e9227192540fe39c. </details> <details> <summary>⛔ Files ignored due to path filters (1)</summary> * `crates/biome_cli/src/execute/migrate/eslint_any_rule_to_biome.rs` is excluded by `!**/migrate/eslint_any_rule_to_biome.rs` and included by `**` </details> <details> <summary>📒 Files selected for processing (1)</summary> * `.changeset/many-candies-see.md` (1 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>📓 Path-based instructions (1)</summary> <details> <summary>.changeset/*.md</summary> **📄 CodeRabbit inference engine (CONTRIBUTING.md)** > Write changesets that are concise (1-3 sentences), user-focused, use past tense for actions taken and present tense for Biome behavior, include code examples for rules, and end sentences with periods Files: - `.changeset/many-candies-see.md` </details> </details><details> <summary>🧠 Learnings (3)</summary> <details> <summary>📓 Common learnings</summary>Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-11-27T23:04:02.022Z
Learning: Applies to crates/biome_analyze//*analyze/src/lint//*.rs : Use thenoInvalidprefix for rules that report runtime errors from mistyping (e.g.,noInvalidConstructorSuper)</details> <details> <summary>📚 Learning: 2025-11-27T23:04:02.022Z</summary>Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-11-27T23:04:02.022Z
Learning: Applies to crates/biome_analyze//*analyze/src/lint/nursery//*.rs : New rules must be placed inside thenurserygroup as an incubation space exempt from semantic versioning, with promotion to appropriate groups done during minor/major releases**Applied to files:** - `.changeset/many-candies-see.md` </details> <details> <summary>📚 Learning: 2025-11-27T23:04:02.022Z</summary>Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-11-27T23:04:02.022Z
Learning: Commit rule changes with message format:feat(biome_<crate>): <ruleName>to follow Biome's conventional commit style**Applied to files:** - `.changeset/many-candies-see.md` </details> </details> </details> <details> <summary>⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)</summary> * GitHub Check: autofix * GitHub Check: Bench (biome_js_formatter) * GitHub Check: Bench (biome_js_parser) * GitHub Check: Bench (biome_js_analyze) * GitHub Check: Test Node.js API * GitHub Check: Check JS Files * GitHub Check: Test (depot-ubuntu-24.04-arm-16) * GitHub Check: Lint project (depot-ubuntu-24.04-arm-16) * GitHub Check: Documentation * GitHub Check: Test (depot-windows-2022-16) * GitHub Check: Lint project (depot-windows-2022) * GitHub Check: Check Dependencies * GitHub Check: End-to-end tests * GitHub Check: Validate rules documentation </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Netail
force-pushed
the
nojsxpropsbind
branch
from
df22508 to
265f382
Compare
|
The valid.jsx did not seem to have valid syntax, threw 6 parser errors. Fixed those. |
crates/biome_js_analyze/tests/specs/nursery/noJsxPropsBind/valid.jsx
Outdated
Show resolved
Hide resolved
|
Thank you for your contribution |
4 participants
Summary
Implement react/jsx-no-bind in Biome
#4637
One important case is still missing
I would need some help for this as I need to track down where the function was declared and whether it is stable
Test Plan
I tested all the examples for correct and incorrect code given in
https://github.com/jsx-eslint/eslint-plugin-react/blob/master/docs/rules/jsx-no-bind.md