-
-
Notifications
You must be signed in to change notification settings - Fork 442
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add noUselessStringConcat
rule
#2720
Conversation
CodSpeed Performance ReportMerging #2720 will not alter performanceComparing Summary
|
I haven't checked the code yet, but my first feedback is to change the name of the rule. At that beginning I thought it was about array concat: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/concat What about |
Or |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work!
Just left some questions and nits. As for the rule name itself, I think my vote would also be for noUselessStringConcatenation
.
|
||
i Consider turning the expression into a single string to improve readability and runtime performance. | ||
|
||
i Unsafe fix: Remove the useless concatenation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the fix unsafe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While implemented this rule, I realized string concatenations can get very weird sometimes in JS, so I decided to consider the fix "unsafe" because there's a small chance that a weird concatenation pattern can result in an incorrect fix that I didn't anticipate. However, I don't know the correct approach of fix categorization in these cases, so let me know if you want me to change to safe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a safe assumption :) We could re-assess the fix category once many users have tested the rule.
3 3 │ const a = 'a' + ('b' + 'c') | ||
4 4 │ const a = ('a' + 'b') + 'c' | ||
5 │ - const·a·=·foo·+·'a'·+·'b'·+·'c' | ||
5 │ + const·a·=·foo·+·'a'·+·"bc" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m not sure I fully understand the reason we can’t merge 'a'
with the other strings. Regardless of the value of foo
, wouldn’t the end result always end in "abc"
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was sure of that too, until I realized that {} + 'abc'
is different from {} + 'a' + 'bc'
in browser environments:
Browsers:
{} + 'abc' // NaN
{} + 'a' + 'bc' // "NaNbc"
Weirdly enough, the result is totally different in Node:
{} + 'a' + 'bc' // "[object Object]abc"
{} + 'abc' // "[object Object]abc"
a17fc13
to
62dc5c6
Compare
@ematipico @arendjr @Conaclos renamed to |
Could you add a changelog entry also? |
noUselessConcat
rulenoUselessStringConcat
rule
… a numeric calculation
c8192bc
to
37408c9
Compare
Yup, done @arendjr! |
@michellocana Thanks! |
This rule lacks a |
Oh, sorry about that, fixed it here #2748 |
No need to apologize, thank you! |
Implementation of the no-useless-concat ESLint rule in Biome.
Closes #2622
Summary
The rule handle many ways to express an useless concatenation and tries to fix the problem when the fix is safe to apply.
const a = 'a' + 'b'
const·a·=·"ab"
const a = 'a' + 'b' + 'c'
const·a·=·"abc"
const a = 'a' + ('b' + 'c')
const·a·=·"abc"
const a = ('a' + 'b') + 'c'
const·a·=·"abc"
const a = 'a' + `b`
const·a·=·"ab"
const a = `a` + 'b'
const·a·=·"ab"
const a = `a` + `b`
const·a·=·"ab"
const a = 'a' + 1
const·a·=·"a1"
const a = 1 + '1'
const·a·=·"11"
const a = 1 + `1`
const·a·=·"11"
const a = `1` + 1
const·a·=·"11"
Special case 1:
foo + 'a' + 'b' + 'c'
The expression
foo + 'a'
might result in different string values depending of the value offoo
, so we don't merge'a'
with'bc'
to preserve the expression result.const a = foo + 'a' + 'b' + 'c'
const·a·=·foo·+·'a'·+·"bc"
const a = (foo + 'a') + ('b' + 'c')
const·a·=·(foo·+·'a')·+·("bc")
const a = ((foo + 'a') + ('b' + 'c') + 1)
const·a·=·((foo·+·'a')·+·("bc")·+·1)
Special case 2:
const a = 1 + 1 + ""
We can't automatically infer the value of all expressions, so we don't suggest a fix unless we are sure of the expression result.
Test Plan
Added snapshots, for valid and invalid cases according to the no-useless-concat ESLint tests.