feat(js_analyze): implement noJsxLeakedDollar#9911
Conversation
🦋 Changeset detectedLatest commit: 91a476b 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 |
|
Unsure if this is also a common problem in svelte, astro or vue, else we can remove the JSX prefix |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a nursery lint rule Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Merging this PR will not alter performance
Comparing Footnotes
|
3eae469 to
2b9522f
Compare
| .note(markup! { | ||
| "This "<Emphasis>"'$'"</Emphasis>" will be rendered as text. Remove it, or move it inside the expression if it is intentional." | ||
| }), |
There was a problem hiding this comment.
wouldn't moving it inside cause the syntax to be invalid?
There was a problem hiding this comment.
Yeah, idk why I left that there. Copied and forgot to modify I am afraid
ematipico
left a comment
There was a problem hiding this comment.
The rule isn't there yet. Some cases seem to be missed, there's some coding mistake, diagnostics need some attention, and the code action seems wrong.
| > 3 │ const Invalid2 = () => <>Hello $${user.name}</> | ||
| │ ^ |
There was a problem hiding this comment.
Correct, the same is done in the original rule. Is that something we should do different?
There was a problem hiding this comment.
Does that mean <>Hello ${user.name}</> (one dollar) is correct? If so, then it's fine; if not, then it's a bug in the rule. I am asking because the code action would remove that dollar, and leave that code.
There was a problem hiding this comment.
<>Hello ${user.name}</> will also be flagged, but if there are two at the end it could mean that the user meant 1 for currency, the other as a refactor mistake. It basically only triggers if the last character of a text node is a dollar sign and the next node is an expression
There was a problem hiding this comment.
in that test case you linked, the dollar sign is escaped <>Hello $\${user.name}</> -> <>Hello \${user.name}</>
There was a problem hiding this comment.
That's because the tests are js string templates, else it becomes an expression in the string template
There was a problem hiding this comment.
<>Hello ${user.name}</>will also be flagged, but if there are two at the end it could mean that the user meant 1 for currency, the other as a refactor mistake. It basically only triggers if the last character of a text node is a dollar sign and the next node is an expression
I think it's a bug, at least for our architecture. If you happen to have $$$$$$$$$$$$$$${user.name}, the rule triggers, apply the code action, then triggers, apply the action. All of it for each dollar sign.
It's an edge case of course, but in Biome we apply code actions until none are left. Eventually, this is a performance penalty. I think we should remove all dollar signs in one go, or not trigger the rule.
| 14 │ }; | ||
| 15 │ | ||
|
|
||
| i This '$' will be rendered as text. Remove it, or move it inside the expression if it is intentional. |
There was a problem hiding this comment.
Here you mention a specific expression (there's the "the" article), but which one is it?
Did you mean "a"?
Also, as per rule pillars, the action is the solution, so I suggest rephrasing this.
There was a problem hiding this comment.
The expression part is false, tweaked the diagnostic
| } | ||
| } | ||
|
|
||
| // Return the range of the trailing `$` character |
There was a problem hiding this comment.
Did you mean leading? Trailing means "the one at the end"
There was a problem hiding this comment.
No, the rule triggers on a dollar sign at the end of a text node. Trailing in the text node, but leading in front of an expression
There was a problem hiding this comment.
Ok, maybe reword the comment then, that wasn't clear
| 13 │ → return·<div>Hello·${props.name}·is·your·name</div>; | ||
| │ - |
There was a problem hiding this comment.
It doesn't seem correct to me. It seems it's removing the space
There was a problem hiding this comment.
The - under the line isn't what it will remove? For me it's under the $
| "@biomejs/biome": patch | ||
| --- | ||
|
|
||
| Added the nursery rule [`noJsxLeakedDollar`](https://biomejs.dev/linter/rules/no-jsx-leaked-dollar), which disallows a leaked `$` sign before a JSX expression. |
There was a problem hiding this comment.
You assume that users know what leaked means here. Instead you should explain the rule with your own words. Same for docs. Explain why is called "leaked" (I mean you did but you didn't connect the dots)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.changeset/rude-crabs-mix.md (1)
5-5: Tiny wording polish for readability.“Unintentional mistake” is a bit redundant; “likely unintentional” reads cleaner.
Suggested wording tweak
-Added the nursery rule [`noJsxLeakedDollar`](https://biomejs.dev/linter/rules/no-jsx-leaked-dollar), which flags text with a trailing `$` if the next sibling node is a JSX expression. This could be an unintentional mistake, resulting in a '$' being rendered as text in the output. +Added the nursery rule [`noJsxLeakedDollar`](https://biomejs.dev/linter/rules/no-jsx-leaked-dollar), which flags text with a trailing `$` if the next sibling node is a JSX expression. This is likely unintentional and results in `'$'` being rendered as text.As per coding guidelines, “Write changeset descriptions concisely and clearly (1-3 sentences)”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.changeset/rude-crabs-mix.md at line 5, Edit the changeset description for the added nursery rule noJsxLeakedDollar to use more concise wording: replace "could be an unintentional mistake, resulting in a '$' being rendered as text in the output." with "is likely unintentional, resulting in a '$' being rendered as text in the output." and ensure the whole changeset remains 1–3 sentences long for clarity and adherence to guidelines.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/biome_js_analyze/src/lint/nursery/no_jsx_leaked_dollar.rs`:
- Line 75: The sources array currently includes two redundant RuleSource entries
(RuleSource::EslintReactJsx("no-leaked-dollar") and
RuleSource::EslintReactXyz("jsx-no-leaked-dollar")); remove the redundant
EslintReactXyz entry and leave only the JSX-focused
RuleSource::EslintReactJsx("no-leaked-dollar") in the sources slice so the rule
references a single correct documentation URL.
---
Nitpick comments:
In @.changeset/rude-crabs-mix.md:
- Line 5: Edit the changeset description for the added nursery rule
noJsxLeakedDollar to use more concise wording: replace "could be an
unintentional mistake, resulting in a '$' being rendered as text in the output."
with "is likely unintentional, resulting in a '$' being rendered as text in the
output." and ensure the whole changeset remains 1–3 sentences long for clarity
and adherence to guidelines.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 53d22ace-3389-463a-a1b3-e20773ad5fa9
⛔ Files ignored due to path filters (1)
crates/biome_js_analyze/tests/specs/nursery/noJsxLeakedDollar/invalid.jsx.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (2)
.changeset/rude-crabs-mix.mdcrates/biome_js_analyze/src/lint/nursery/no_jsx_leaked_dollar.rs
4399a97 to
34054a5
Compare
34054a5 to
91a476b
Compare
This PR contains the following updates: | Package | Update | Change | |---|---|---| | [@biomejs/biome](https://biomejs.dev) ([source](https://github.com/biomejs/biome/tree/HEAD/packages/@biomejs/biome)) | patch | `2.4.12` → `2.4.13` | --- ### Release Notes <details> <summary>biomejs/biome (@​biomejs/biome)</summary> ### [`v2.4.13`](https://github.com/biomejs/biome/blob/HEAD/packages/@​biomejs/biome/CHANGELOG.md#2413) [Compare Source](https://github.com/biomejs/biome/compare/@biomejs/[email protected]...@biomejs/[email protected]) ##### Patch Changes - [#​9969](biomejs/biome#9969) [`c5eb92b`](biomejs/biome@c5eb92b) Thanks [@​officialasishkumar](https://github.com/officialasishkumar)! - Added the nursery rule [`noUnnecessaryTemplateExpression`](https://biomejs.dev/linter/rules/no-unnecessary-template-expression/), which disallows template literals that only contain string literal expressions. These can be replaced with a simpler string literal. For example, the following code triggers the rule: ```js const a = `${"hello"}`; // can be 'hello' const b = `${"prefix"}_suffix`; // can be 'prefix_suffix' const c = `${"a"}${"b"}`; // can be 'ab' ``` - [#​10037](biomejs/biome#10037) [`f785e8c`](biomejs/biome@f785e8c) Thanks [@​minseong0324](https://github.com/minseong0324)! - Fixed [#​9810](biomejs/biome#9810): [`noMisleadingReturnType`](https://biomejs.dev/linter/rules/no-misleading-return-type/) no longer reports false positives on a getter with a matching setter in the same namespace. ```ts class Store { get status(): string { if (Math.random() > 0.5) return "loading"; return "idle"; } set status(v: string) {} } ``` - [#​10084](biomejs/biome#10084) [`5e2f90c`](biomejs/biome@5e2f90c) Thanks [@​jiwon79](https://github.com/jiwon79)! - Fixed [#​10034](biomejs/biome#10034): [`noUselessEscapeInRegex`](https://biomejs.dev/linter/rules/no-useless-escape-in-regex/) no longer flags escapes of `ClassSetReservedPunctuator` characters (`&`, `!`, `#`, `%`, `,`, `:`, `;`, `<`, `=`, `>`, `@`, `` ` ``, `~`) inside `v`-flag character classes as useless. These characters are reserved as individual code points in `v`-mode, so the escape is required. The following pattern is now considered valid: ```js /[a-z\&]/v; ``` - [#​10063](biomejs/biome#10063) [`c9ffa16`](biomejs/biome@c9ffa16) Thanks [@​Netail](https://github.com/Netail)! - Added extra rule sources from ESLint CSS. `biome migrate eslint` should do a bit better detecting rules in your eslint configurations. - [#​10035](biomejs/biome#10035) [`946b50e`](biomejs/biome@946b50e) Thanks [@​Netail](https://github.com/Netail)! - Fixed [#​10032](biomejs/biome#10032): [useIframeSandbox](https://biomejs.dev/linter/rules/use-iframe-sandbox/) now flags if there's no initializer value. - [#​9865](biomejs/biome#9865) [`68fb8d4`](biomejs/biome@68fb8d4) Thanks [@​dyc3](https://github.com/dyc3)! - Added the new nursery rule [`useDomNodeTextContent`](https://biomejs.dev/linter/rules/use-dom-node-text-content/), which prefers `textContent` over `innerText` for DOM node text access and destructuring. For example, the following snippet triggers the rule: ```js const foo = node.innerText; ``` - [#​10023](biomejs/biome#10023) [`bd1e74f`](biomejs/biome@bd1e74f) Thanks [@​ematipico](https://github.com/ematipico)! - Added a new nursery rule [`noReactNativeDeepImports`](https://biomejs.dev/linter/rules/no-react-native-deep-imports/) that disallows deep imports from the `react-native` package. Internal paths like `react-native/Libraries/...` are not part of the public API and may change between versions. For example, the following code triggers the rule: ```js import View from "react-native/Libraries/Components/View/View"; ``` - [#​9885](biomejs/biome#9885) [`3dce737`](biomejs/biome@3dce737) Thanks [@​dyc3](https://github.com/dyc3)! - Added a new nursery rule [`useDomQuerySelector`](https://biomejs.dev/linter/rules/use-dom-query-selector/) that prefers `querySelector()` and `querySelectorAll()` over older DOM query methods such as `getElementById()` and `getElementsByClassName()`. - [#​9995](biomejs/biome#9995) [`4da9caf`](biomejs/biome@4da9caf) Thanks [@​siketyan](https://github.com/siketyan)! - Fixed [#​9994](biomejs/biome#9994): Biome now parses nested CSS rules correctly when declarations follow them inside embedded snippets. - [#​10009](biomejs/biome#10009) [`b41cc5a`](biomejs/biome@b41cc5a) Thanks [@​Jayllyz](https://github.com/Jayllyz)! - Fixed [#​10004](biomejs/biome#10004): [`noComponentHookFactories`](https://biomejs.dev/linter/rules/no-component-hook-factories/) no longer reports false positives for object methods and class methods. - [#​9988](biomejs/biome#9988) [`eabf54a`](biomejs/biome@eabf54a) Thanks [@​Netail](https://github.com/Netail)! - Tweaked the diagnostics range for [useAltText](https://biomejs.dev/linter/rules/use-alt-text), [useButtonType](https://biomejs.dev/linter/rules/use-button-type), [useHtmlLang](https://biomejs.dev/linter/rules/use-html-lang), [useIframeTitle](https://biomejs.dev/linter/rules/use-iframe-title), [useValidAriaRole](https://biomejs.dev/linter/rules/use-valid-aria-role) & [useIfameSandbox](https://biomejs.dev/linter/rules/use-iframe-sandbox) to report on the opening tag instead of the full tag. - [#​10043](biomejs/biome#10043) [`fc65902`](biomejs/biome@fc65902) Thanks [@​mujpao](https://github.com/mujpao)! - Fixed [#​10003](biomejs/biome#10003): Biome no longer panics when parsing Svelte files containing `{#}`. - [#​9815](biomejs/biome#9815) [`5cc83b1`](biomejs/biome@5cc83b1) Thanks [@​dyc3](https://github.com/dyc3)! - Added the new nursery rule [`noLoopFunc`](https://biomejs.dev/linter/rules/no-loop-func/). When enabled, it warns when a function declared inside a loop captures outer variables that can change across iterations. - [#​9702](biomejs/biome#9702) [`ef470ba`](biomejs/biome@ef470ba) Thanks [@​ryan-m-walker](https://github.com/ryan-m-walker)! - Added the nursery rule [`useRegexpTest`](https://biomejs.dev/linter/rules/use-regexp-test/) that enforces `RegExp.prototype.test()` over `String.prototype.match()` and `RegExp.prototype.exec()` in boolean contexts. `test()` returns a boolean directly, avoiding unnecessary computation of match results. **Invalid** ```js if ("hello world".match(/hello/)) { } ``` **Valid** ```js if (/hello/.test("hello world")) { } ``` - [#​9743](biomejs/biome#9743) [`245307d`](biomejs/biome@245307d) Thanks [@​leetdavid](https://github.com/leetdavid)! - Fixed [#​2245](biomejs/biome#2245): Svelte `<script>` tag language detection when the `generics` attribute contains `>` characters (e.g., `<script lang="ts" generics="T extends Record<string, unknown>">`). Biome now correctly recognizes TypeScript in such script blocks. - [#​10046](biomejs/biome#10046) [`0707de7`](biomejs/biome@0707de7) Thanks [@​Conaclos](https://github.com/Conaclos)! - Fixed [#​10038](biomejs/biome#10038): [`organizeImports`](https://biomejs.dev/assist/actions/organize-imports/) now sorts imports in TypeScript modules and declaration files. ```diff declare module "mymodule" { - import type { B } from "b"; import type { A } from "a"; + import type { B } from "b"; } ``` - [#​10012](biomejs/biome#10012) [`94ccca9`](biomejs/biome@94ccca9) Thanks [@​ematipico](https://github.com/ematipico)! - Added the nursery rule [`noReactNativeLiteralColors`](https://biomejs.dev/linter/rules/no-react-native-literal-colors/), which disallows color literals inside React Native styles. The rule belongs to the `reactNative` domain. It reports properties whose name contains `color` and whose value is a string literal when they appear inside a `StyleSheet.create(...)` call or inside a JSX attribute whose name contains `style`. ```jsx // Invalid const Hello = () => <Text style={{ backgroundColor: "#FFFFFF" }}>hi</Text>; const styles = StyleSheet.create({ text: { color: "red" }, }); ``` ```jsx // Valid const red = "#f00"; const styles = StyleSheet.create({ text: { color: red }, }); ``` - [#​10005](biomejs/biome#10005) [`131019e`](biomejs/biome@131019e) Thanks [@​ematipico](https://github.com/ematipico)! - Added the nursery rule [`noReactNativeRawText`](https://biomejs.dev/linter/rules/no-react-native-raw-text/), which disallows raw text outside of `<Text>` components in React Native. The rule belongs to the new `reactNative` domain. ```jsx // Invalid <View>some text</View> <View>{'some text'}</View> ``` ```jsx // Valid <View> <Text>some text</Text> </View> ``` Additional components can be allowlisted through the `skip` option: ```json { "options": { "skip": ["Title"] } } ``` - [#​9911](biomejs/biome#9911) [`1603f78`](biomejs/biome@1603f78) Thanks [@​Netail](https://github.com/Netail)! - Added the nursery rule [`noJsxLeakedDollar`](https://biomejs.dev/linter/rules/no-jsx-leaked-dollar), which flags text nodes with a trailing `$` if the next sibling node is a JSX expression. This could be an unintentional mistake, resulting in a '$' being rendered as text in the output. **Invalid**: ```jsx function MyComponent({ user }) { return <div>Hello ${user.name}</div>; } ``` - [#​9999](biomejs/biome#9999) [`f42405f`](biomejs/biome@f42405f) Thanks [@​minseong0324](https://github.com/minseong0324)! - Fixed `noMisleadingReturnType` incorrectly flagging functions with reassigned `let` variables. - [#​10075](biomejs/biome#10075) [`295f97f`](biomejs/biome@295f97f) Thanks [@​ematipico](https://github.com/ematipico)! - Fixed [`#9983`](biomejs/biome#9983): Biome now parses functions declared inside Svelte `#snippet` blocks without throwing errors. - [#​10006](biomejs/biome#10006) [`cf4c1c9`](biomejs/biome@cf4c1c9) Thanks [@​minseong0324](https://github.com/minseong0324)! - Fixed [#​9810](biomejs/biome#9810): `noMisleadingReturnType` incorrectly flagging nested object literals with widened properties. - [#​10033](biomejs/biome#10033) [`11ddc05`](biomejs/biome@11ddc05) Thanks [@​ematipico](https://github.com/ematipico)! - Added the nursery rule [`useReactNativePlatformComponents`](https://biomejs.dev/linter/rules/use-react-native-platform-components/) that ensures platform-specific React Native components (e.g. `ProgressBarAndroid`, `ActivityIndicatorIOS`) are only imported in files with a matching platform suffix. It also reports when Android and iOS components are mixed in the same file. The following code triggers the rule when the file does not have an `.android.js` suffix: ```js // file.js import { ProgressBarAndroid } from "react-native"; ``` </details> --- ### Configuration 📅 **Schedule**: (UTC) - Branch creation - At any time (no schedule defined) - Automerge - At any time (no schedule defined) 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiI0My4xMzkuNiIsInVwZGF0ZWRJblZlciI6IjQzLjEzOS42IiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6W119--> Reviewed-on: https://git.oirnoir.dev/OIRNOIR/YouTube-Helper-Server/pulls/1
Summary
Ported https://www.eslint-react.xyz/docs/rules/jsx-no-leaked-dollar, disallows a leaked
$sign before a JSX expression.Partially generated by co-pilot
Test Plan
Unit tests
Docs