Add exception to Cognitive Complexity for React functional components

[saberduck]

Consider React functional component such as

function ChatBoardComponent(props: ChatBoardComponentProps) {
    
    function handleShowAddRoomModal() {
        if(something){
            do something
    }

    function handleCloseAddRoomModal() {
        if(something){
            do something
    }

    function handleSomethingElse() {
        if(something){
            do something
    }
     
    if(something){} // we need some complexity-statement here to make top-level function considered by the rule
    return <p>Hello world</p>;
}

Currently, cognitive complexity of such component would be 7, while the equivalent component using class syntax will have only one. We should avoid adding complexity of nested functions to the outer function in such case.

See thread on community forum https://community.sonarsource.com/t/sonarqube-counts-incorrect-cognitive-complexity-for-react-functional-components-bug-no-functionality/30852

[vijkris99]

+1

I want to mention another rule that is closely related in this case, which is "Functions should not have too many lines of code". React functional components sometimes tend to break this rule because they tend to contain many nested functions within, whereas an equivalent class component would not trigger the same issue. If possible, it would be nice to address this together with the complexity issue for functional components.

[bunnyc1986]

The functional component is still a function. I don't think you need to make an exception to it. What is the rule for making exceptions? What about reducers, hooks, etc.? they could also easier get long as well.

[krreet]

The functional component is still a function. I don't think you need to make an exception to it. What is the rule for making exceptions? What about reducers, hooks, etc.? they could also easier get long as well.

In JavaScript /react a class component is just a faux name space for a function.

So yes this rule should treat all components similar ( because now the class component can now be written as a functional component with closures). ( the complexity of closures increasing the complexity of enclosing function only in functional component and not in class component is not right)

Hence this rule of treating all components similar should be present for react.js
However hooks, reducers don't need this treatment.

Here’s an excerpt from the whitepaper

Despite the recent addition of classes to JavaScript by the ECMAScript 6 specification, the feature is not yet widely adopted. In fact, many popular frameworks require the continued use of the compensating idiom: the use of an outer function as a stand-in to create a kind of namespace or faux class. So as not to penalize JavaScript users, such outer functions are ignored when they are used purely as a declarative mechanism, that is when they contain only declarations at the top level.
However, the presence at the top level of a function (i.e. not nested inside a sub-function) of statements subject to structural increments indicates something other than a pure declarative usage. Consequently, such functions should receive a standard treatment.

"So as not to penalize JavaScript users, such outer functions are ignored " hence , you should also treat the functional components(outer functions) in a similar way ( ignore it)

[arevi]

@saberduck Is this issue still on any roadmap to be corrected? Functional components, and hooks, are becoming the de facto standard for modern React applications.

It doesn't make sense to penalize code quality and weigh negative scores on modern coding standards, compared to using class based components.

[mullim]

I second that @arevi, I'm finding that the negative scores on functional components are actually causing worse code quality and less maintainable code.

[vilchik-elena]

Note that we already would not report if top level function does not have any constructs increasing the complexity. Still it's possible to have if statements or something else which would make top level function considered by the rule.

Example in real project of such component with some complexity inside of the top level function

[vilchik-elena]

When adding this exception I will apply following criteria:

• function returns JSX (looking at AST only, no type inference involved)
• function has name starting with capital letter

It would be great if someone from community could confirm that those criteria are making sense for real life projects.

[Pointerbender]

Hi @vilchik-elena , thank you for working on this issue! I'm an end-user of the SonarJS plugin and those criteria would work for all of my projects.

[gbalatckii]

Hello @vilchik-elena,

i just updated SonarQube to version 9.0 and I still get this error message. For deploying the project to SonarQube I am using sonar-scanner (2.8.0). Could you please check it?

[vilchik-elena]

@GeorgeBalat if you are returning JSX fragment then the fix is not yet released (SonarSource/eslint-plugin-sonarjs#245)

[gbalatckii]

@GeorgeBalat if you are returning JSX fragment then the fix is not yet released (SonarSource/eslint-plugin-sonarjs#245)

Thank you for the fast response. Can I change now something to pass this rule? Maybe another return type (example please)?
I see that #245 is merged into master, when will it be delivered to SonarQube?

[vilchik-elena]

@GeorgeBalat just won't fix the issue or mark it as false positive. Fix is part of analyzer release but I can't tell you yet when it will be available in SQ.

[gbalatckii]

@GeorgeBalat just won't fix the issue or mark it as false positive. Fix is part of analyzer release but I can't tell you yet when it will be available in SQ.

Okay, thank you for the info.

[kirillito]

Hi there! I can see that JSX fix for this is released.
Does it apply to Typescript (*.tsx) files as well?
We still see those Cognitive Complexity issues reported for functional components defined in *.tsx files in our SonarCloud account.

[vilchik-elena]

@kirillito yep, it applies to tsx as well. Create a new issue with your reproducer

[gbalatckii]

@GeorgeBalat just won't fix the issue or mark it as false positive. Fix is part of analyzer release but I can't tell you yet when it will be available in SQ.

Hi @vilchik-elena
was it fixed in the meantime? If yes, which SonarQube version is needed?

[vilchik-elena]

@GeorgeBalat latest :) I am not sure which version exactly, but the recommendation is anyway to rely on LTS or latest.

[gbalatckii]

@GeorgeBalat latest :) I am not sure which version exactly, but the recommendation is anyway to rely on LTS or latest.

Hello @vilchik-elena , it looks like it is still not fixed in 9.3

[vilchik-elena]

@GeorgeBalat Could you provide full definition of react component on which issue is reported?

[gbalatckii]

Hello Elena,
what do you mean with a full definition? Whole code or whole SonarQube analysis for this component?

[vilchik-elena]

@GeorgeBalat whole code of this function

[gbalatckii]

@GeorgeBalat whole code of this function
@vilchik-elena This is difficult because it is productive code and there may be rights issues. Is there anything in particular I can send you or describe?

[vilchik-elena]

@GeorgeBalat I need at least full signature and return statement(s). Also note that even if it's react component, complexity of code not nested inside inner function will be computed

function ChatBoardComponent(props: ChatBoardComponentProps) {
   
    function handleSomethingElse() {
        if(something){
            do something
    }
     
    if(something){} // if there are 20 statements like this, we will report on the component function
    return <p>Hello world</p>;
}

[gbalatckii]

@vilchik-elena
Ach, I didn't know that. I see 16 of 19 issues are there, cause I have if statements in the JSX. Something like this:
readonly={a || b || c || d} or a && <p>...</p>
Are there some refactoring possibilities to get rid of the SonarQube code smell? Will it be fixed, if all of them will be defined in useMemo or useCallback?

[vilchik-elena]

install SonarLint to have instant analysis (if not yet) and just try to simplify your code (extract some logic into functions external to it). Also don't forget that you can simply 'won't fix' the issue if you believe the simplification does not make sense.

[gbalatckii]

@vilchik-elena ok, thank you. I will try it out.

[skaraman]

does this exception not apply to Preact ?

[H91011]

I have same issue is it fixed or should I ignore it ?

Sonar version: 9.2.4 (build 50792)

[user4302]

I have same issue is it fixed or should I ignore it ?

Sonar version: 9.2.4 (build 50792)

same, SonarQube v9.3.3 and im still getting this issue