[feat] no-direct-set-state-in-use-effect: option to only warn on symmetric set function call with dependency

[karlhorky]

Describe the problem

Hi @Rel1cx, thanks for this plugin, amazing!

I'm looking to ban the usage of symmetrical set functions in useEffect without banning set functions entirely:

Only setLimit would be a reported problem (because of the limit in the dependency array), not setItems

import { useEffect, useState } from 'react';
 
function List({ items }) {
  const [items, setItems] = useState([]);
  const [limit, setLimit] = useState(false);
 
  useEffect(() => {
    setLimit(/* ... */);
    setItems(/* ... */);
  }, [limit]);
  // ...
}

Describe the solution you'd like

A new option (default true? would be a breaking change though) to only check for symmetrical setters

Maybe this also affects no-direct-symmetrical-set-state-in-use-layout-effect?

Alternatives considered

A new rule no-direct-symmetrical-set-state-in-use-effect (maybe also no-direct-symmetrical-set-state-in-use-layout-effect?)

Additional context

This would be a less strict rule that would catch a lot of infinite loop patterns without restricting set functions for use cases that can make sense

Use case: teaching students to avoid infinite loops while not being overly strict:

• New Rule: Detect infinite loops in code and warn upleveled/eslint-plugin-upleveled#36

[SukkaW]

  useEffect(() => {
    setItems(/* ... */);
  }, [limit]);

This is exactly the bad pattern this eslint rule is trying to report. Always use useMemo for the derived state. In your case, you should do const items = useMemo(() => { /** */ }. [limit]) instead.

[karlhorky]

Hmm, useMemo isn't commonly used to retrieve data from an external location eg. data fetching, so I would still say there's a valid (if maybe not always best practice for performance) use case. Fetching in useEffect is a simple, widespread pattern. It can be replaced by RSC and data fetching libraries and can have some downsides, but it's not always wrong.

Gut feel says there are also other use cases other than data fetching where using a set function in useEffect is ok too.

I still think the option suggestion is valid.

[SukkaW]

external location eg. data fetching
Fetching in useEffect is a simple, widespread pattern.

Then you will typically call setItems() inside .then(() => {}) or (async () => { })(), see #628. Those patterns should already be allowed by the rule.

If you are calling the setItems inside the direct scope of useEffect(() => {}, []), you are doing this wrong.

[karlhorky]

Then you will typically call setItems() inside .then(() => {}) or (async () => { })(), see #628. Those patterns should already be allowed by the rule.

Oh nice, that's great! That's one step closer to making it possible for us to adopt (at least for our projects - for students and learning, it's probably still too strict)

What about other patterns to call set functions in a useEffect? Eg. Updating data passed via context, based on component-local information?

const { globalData, setGlobalData } = useContext(globalDataContext)

useEffect(() => {
  setGlobalData(componentLocalData)
}, [])

Maybe in that case, you would recommend setting state during render?

-useEffect(() => {
+if (/* check if componentLocalData is already in globalData */) {
  setGlobalData(componentLocalData)
-}, [])
+}

Feels weird to do this, but I guess as long as you have a good if condition, then it should be fine...

[SukkaW]

What about other patterns to call set functions in a useEffect? Eg. Updating data passed via context, based on component-local information?

Would you like to give me an example of "component-local" information?

See also: https://react.dev/learn/thinking-in-react#step-4-identify-where-your-state-should-live

[karlhorky]

for students and learning, it's probably still too strict

I think there are still use cases where being less strict with this rule would be nice (could also apply to teams which prefer rules to be less strict).

For these types of use cases, would the maintainers here consider an option (default false) that would only check for symmetrical set functions?

[SukkaW]

for students and learning, it's probably still too strict

If it is for students, then IMHO this rule is a must-have. It is teaching them not to abuse useEffect for derived state.

[karlhorky]

It is teaching them not to abuse useEffect for derived state.

I agree that teaching about avoiding extra state variables for derived state is good! (we do this already, in multiple ways, lots of demonstrations in lectures and showcasing Kent C. Dodds' post about deriving state)

However, this rule is too general and strict for this - banning all set function usage inside of useEffect is not an effective way to teach this:

1. There are other bad derived state patterns that this doesn't catch (the main thing is actually extra state variables, not useEffect)
2. This prevents all usage of set functions inside of useEffect, which is:
   1. More wide-ranging than only teaching about derived state
   2. Unnecessarily strict + complainy - configuring ESLint for beginner students is a fine balance between teaching and still allowing them some patterns which may not be the best. Students already have problems with ESLint showing them too many warnings, and not making enough progress because of this.

But this is not only about the students / teaching case - I am proposing this also as a less strict alternative of this rule, which we (and I'm guessing, other teams) could adopt right away without thinking too much about it. And in future, incrementally changing our configuration to be more strict, if we would like to further improve.

[Rel1cx]

Hi there!
I agree with you about providing a way to only prevent "effect loops" rather than disabling the set function entirely in useEffect, but unfortunately neither "A new option to only check for symmetrical setter" or "A new rule no-direct-symmetrical-set-state-in-use-effect" will not be able to check and prevent the very common effect loop from occurring.
Consider the following example, slightly modified from the one above:

import { useEffect, useState } from "react";

export function List() {
  const [items, setItems] = useState([4, 3, 2, 1, 0]);
  const [limit, setLimit] = useState(false);

  useEffect(() => {
    setItems(x => [...x].reverse());
  }, [limit]);

  // ...Many other hooks between the two `useEffect` calls

  useEffect(() => {
    setLimit(x => !x);
  }, [items]);
  // ...

  return null;
}

We probably need a new specialized rule to prevent update loops in render cascades.
Another advantage of the new rule is that we can set the severity of the rule to error, and no-direct-set-state-in-use-effect to warn.

FYI:
https://x.com/dan_abramov2/status/1825697422494019851
https://x.com/fraser_drops/status/1825953286786265424
https://x.com/fraser_drops/status/1582408358883397633

[karlhorky]

Consider the following example, slightly modified from the one above

Good point!

I was thinking of just targeting the simple cases at first for an MVP, but it does make sense to make it more robust for loops across multiple useEffect calls

[karlhorky]

Should this issue be closed now in favor of #755 ?