Skip to content
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

Remove S6480 from default profile #3614

Closed
saberduck opened this issue Dec 22, 2022 · 13 comments · Fixed by SonarSource/rspec#1487
Closed

Remove S6480 from default profile #3614

saberduck opened this issue Dec 22, 2022 · 13 comments · Fixed by SonarSource/rspec#1487
Assignees
Milestone

Comments

@saberduck
Copy link
Contributor

We need to improve documentation before including this rule in LTS

@saberduck saberduck added this to the 9.13 milestone Dec 22, 2022
@AndersonHqds
Copy link

AndersonHqds commented Dec 28, 2022

Yeah, sonarcloud is forcing the react applications to use the useCallback for all functions, and it should not be used for all of them.

@ls-yannick
Copy link

I'm not sure if this is on your radar as well, but applying useCallback without a memo() on the called component won't actually improve performance, but the extra useCallback+dependencies will arguably hurt readability of the code. The new react docs have a pretty comprehensive section about this.

@gabriel-vivas-sonarsource

Adding related Community thread for historic context:
https://community.sonarsource.com/t/6480-validation-typescript-code-smell/78668/6

@gabriel-vivas-sonarsource
Copy link

gabriel-vivas-sonarsource commented Jan 5, 2023

@ls-yannick thanks for the link.

Indeed, memo of direct children is the prominent use case.
Unfortunately, there are more subtle cases that are hard to track.

For example, when the function is used as a Hook dependency lower in the tree, it will trigger the hook.
Also, when passed to grand-child components, it will invalidate any memoization there.

Anyway, we are good listeners, so we are removing it from the "Sonar Way" until we can learn how to make this more useful for most.

Most probably, we will need to improve the rule before it comes back to the spotlight.

@MaelthiS
Copy link

@gabriel-vivas-sonarsource do you know if users of Sonar cloud have something manually to do to disable the rule ? Or the sonar way quality profile will update by itself ? Because we still have the issue on new code in our repo.

@gabriel-vivas-sonarsource

Hi @MaelthiS, it should be automatic if you're using the Sonar Way profile in SonarCloud

@JulioCVaz
Copy link

Hi @MaelthiS, it should be automatic if you're using the Sonar Way profile in SonarCloud

Hey @gabriel-vivas-sonarsource how you doing?

This update not reflected for us? Any solution?

We using sonar way with default rules.

Captura de Tela 2023-01-19 às 16 55 09

@ls-yannick
Copy link

Same here btw, it still hasn't come through.

@rubenmamo
Copy link

Same here as well, I still see these errors on my scans

@gabriel-vivas-sonarsource

Sorry for the confusion, we removed from SonarQube LTS before release, it will be updated in SonarCloud soon.
If you need this now, you can create a new profile that inherits from Sonar Way and disable the rule.
Otherwise, hang on, it should be transparent.

@lebbe
Copy link

lebbe commented Jul 4, 2023

We still are hassled by it in Sonar Cube.

Would be nice with some transparency around the introduction of this rule in the first place. Because when I first noticed this rule, it went against all my instincts with regard to premature optimization versus readable code. What team in Sonar discussed this, and landed in the rule being a clever thing to introduce in the first place?

I've already seen applications in havoc with hundreds of "useCallback" statements, in an attempt to oblige this lint rule. The developers feed lambdas to useCalkback, initialize a const with the return value and send this as an property. I wouldn't be surprised if this is more computationally expensive than just sending the lambda in the first place. I know that the code is much less readable.

I don't think this rule should be removed from the default, or have a better documentation. I think the rule should be nuked from existence.

@sbtnh
Copy link

sbtnh commented Oct 3, 2023

Hi @gabriel-vivas-sonarsource, it seems this rule is still active by default. Any plan to disable it?

@ilia-kebets-sonarsource
Copy link
Contributor

Hello,
For the default profile to reflect this change, you need to upgrade your SonarQube instance to at least version 9.9.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants