javascript:S6480 was recently deployed on SonarCloud.
Are these issues being raised as new (instead of on Overall code)? Ideally, they should be backdated so that there isn’t noise in your New Code Period.
Colin This rule seems overreaching to me. It seems ok when we consider .bind functions but not arrow functions.
As React docs states “Using an arrow function in render creates a new function each time the component renders, which may break optimizations based on strict identity comparison.”
But for the majority of time React components don’t fall in that category, since memorizing components all around comes with a cost and its benefits for the most part are slim, or none.
Given that, now we face a rule that is covering exception cases. Even worse, now we have to wrap useCallback on these functions unnecessary, and the usage of useCallback itself comes with a cost.
Since this change our builds seem to have really slowed down too. We are using Bitbucket. The last build we stopped it after 15 minutes. The current build has been running for an hour.
Here is the final part of the build log where Sonar is running:
INFO: Reading type hierarchy from: /opt/atlassian/pipelines/agent/build/.scannerwork/ucfg2/python
INFO: Read 0 type definitions
INFO: No UCFGs have been included for analysis.
INFO: Sensor PythonSecuritySensor [security] (done) | time=0ms
INFO: Sensor JsSecuritySensor [security]
INFO: Reading type hierarchy from: /opt/atlassian/pipelines/agent/build/.scannerwork/ucfg2/js
INFO: Read 0 type definitions
INFO: Reading UCFGs from: /opt/atlassian/pipelines/agent/build/.scannerwork/ucfg2/js
INFO: 06:55:01.944254 Building Runtime Type propagation graph
INFO: 06:55:02.251601 Running Tarjan on 10106 nodes
INFO: 06:55:02.291766 Tarjan found 10106 components
INFO: 06:55:02.333819 Variable type analysis: done
INFO: 06:55:02.336878 Building Runtime Type propagation graph
INFO: 06:55:02.548116 Running Tarjan on 10106 nodes
INFO: 06:55:02.574083 Tarjan found 10106 components
INFO: 06:55:02.63977 Variable type analysis: done
INFO: Analyzing 1205 ucfgs to detect vulnerabilities.
INFO: Taint analysis starting. Entrypoints: 280
INFO: Running symbolic analysis for 'JS'
Before Sonar Scan step would take a 1-2 minutes to complete.
I’ve also ran into this new rule which seems a little overreaching. For the better part of the previous year, this is the first time i’m seeing this pop up after upgrading to 9.8 of the community edition.
Each time the parent is rendered, the function will be re-created and trigger a render of the component causing excessive renders and more memory use.
To add to rbuzatto’s point, the precondition for this statement is only relevant for class-based components extended from PureComponent, and function components wrapped in React.memo. For the vast majority of circumstances, every component is always rerendered below a rerendering parent, to negligible impact.
I don’t know if SonarCloud can go this deep when spidering code, but this rule might make sense if it can confirm that the component being sent the arrow function is memoized with extends PureComponent or React.memo(). Otherwise it’s an inaccurate warning.
Re: the build failures. I needed to increase the memory allocation in Bitbucket. I was doing it wrong. It might be that the latest update scan is also consuming more memory.
We have decided to disable the S6480 rule by default for now, until we can get more information and improve the rule. It can still be enabled manually.
See the Github issue:
Also another thread on the same topic:
For clarification, the memory issue faced by @jeffski has no relation to this particular rule.
There are a couple of incorrect assumptions in that rule:
The impact of using the arrow function in built-in components (like div) is negligible, and adding useCallback to every function -as the rule description sugggest- is worse (it uses more memory as it adds an internal state for the cache and a dependency check).
The are multiple strategies to avoid re-renders. Depending on how complex the component is, memoizing callbacks may not help. Another strategy is to memoize the whole sub-tree (for example One simple trick to optimize React re-renders).
Probably it’s better to remove the rule or at least re-word it (the rule description is technically wrong).