[no-access-state-in-setstate] equivalent for useState

[rodrigofariow]

Hey guys!

I was wondering if there are any plans to do a similar rule to react/no-access-state-in-setstate but for useState?
I feel that there is still a lot of benefit of doing that to prevent previous state bugs and having react-hooks rules suggesting to put previous state on dependencies array (for useEffect, for instance).

Thank you for your time :)

[ljharb]

Can you provide some code samples you'd want to warn?

[rodrigofariow]

Hi @ljharb

Absolutely!

function SquareRoom() {
  const [numRooms, setNumRooms] = useState(0)

  const increment = () => {
    // eslint should report here saying that we should use the setState callback to access the previous state when calculating a new one
    setNumRooms(numRooms + 1) // 👎 
    
    setNumRooms(prev => prev + 1) // 👍 
  }

  return ...
}

This isn't extremely necessary here but imagine we have logic within a useEffect or useCallback. We may have to add numRooms so that the closure is up to date, which is unfortunate. Having the callback form in use would prevent the following headaches:

function SquareRoom() {
  const [numRooms, setNumRooms] = useState(0)
  
  useEffect(() => {
   const id = window.setInterval(() => {
     // we can also start having issues when having intervals involved 
      setNumRooms(numRooms + 1) 

    }, 2000)

    return () => {
      window.clearInterval(id)
    }
  }, []) // eslint[react-hooks/exhaustive-deps] will say we need to add numRooms to the dependencies array

const increment = useCallback(() => {
    setNumRooms(numRooms + 1)
  }, []) // eslint[react-hooks/exhaustive-deps] will say we need to add numRooms to the dependencies array

  return ...
}

SIDE NOTE:

To be fair,react-hooks/exhaustive-deps rule does warn the following:
React Hook useEffect has a missing dependency: 'numRooms'. Either include it or remove the dependency array. You can also do a functional update 'setNumRooms(n => ...)' if you only need 'numRooms' in the 'setNumRooms'

However, having the react team recommending this rule to be a warning and not an error, we don't have the strictest code. Inexperienced devs will still include numRooms on the dep array and waste time having to solve perf issues or bugs.
The functional form also encourages code reuse.

In short, this rule would kick in whenever the previous state is accessed to calculate the new one.

I'm not sure how complex it can be to implement this but if we keep the rule only for useState I think it can be somewhat doable. Even if it doesn't catch more complex use cases like:

const increment = () => {
    const finalRoomsValue = someOtherFunction(numRooms)
    setNumRooms(finalRoomsValue + 1)
  }

I still think it's a beneficial rule to have enabled.

What do you think?

Thank you once again for your time.

[ljharb]

Regarding exhaustive deps, if the rule already exists, and you can choose to set it to an error (the react team unfortunately believes, incorrectly, that eslint rules set to "warn" still preserve their usefulness), I don't think it'd make sense to reimplement it here just so we could make a different severity recommendation.

I think that if a very thorough set of test cases was provided, and all of the warnings were legitimate, and the bad patterns that were not warned were exceedingly rare, then that might still be useful - but a rule that only catches a subset of a problematic pattern can often be worse than no rule at all, because it lulls people into a false sense of safety.

[eliw00d]

This would be great!

[Vladyslav-Soldatenko]

@ljharb, could you take a look at this code? Here useEffect is of no use, and this rule might save a lot of debug time
https://codesandbox.io/s/cocky-tristan-52dmpk?file=/src/App.js

I'm asking, because I might try to implement it in my free time, but would like to have your approval that you'll find this rule useful and might merge it

[ljharb]

I’m confused; that code doesn’t have useEffect in it, but i agree that calling useState more than once without the function fork is problematic - but especially for a counter, nobody would ever do that. Do you have actual, not contrived, code that would benefit?

[Vladyslav-Soldatenko]

@ljharb, Thanks for such a quick answer and for your time
Above you've mentioned that useEffect and "react-hooks/exhaustive-deps" might catch these errors, and I just wanted to prove, that the rule still might be useful on its own.

As for code: I don't find it contrived.

nobody would ever do that

Well, some juniors definitely might. Replace "counter" with anything business related. Here is another example
https://codesandbox.io/s/zealous-http-xgwjlx?file=/src/App.js
Imagine that we count user likes, and if they are logged in - add it to their list. This rule might save from a bug here, right?

feel free to ask any additional questions or argue, why you find this rule unhelpful

[Vladyslav-Soldatenko]

@ljharb, did you find time to look at the provided code examples? What do you think? Should we add the rule, or maybe close the issue altogether?

[denyo]

This would actually be the hook version of https://github.com/jsx-eslint/eslint-plugin-react/blob/master/docs/rules/no-access-state-in-setstate.md which would be really helpful.