facebook / react

The library for web and native user interfaces.
https://react.dev
MIT License
227.21k stars 46.32k forks source link

Feature request: adding a `triggers` optional array to `useEffect` #22132

Closed eric-burel closed 4 months ago

eric-burel commented 3 years ago

Currently, useEffect accepts an array of dependencies as its second parameters. It must be filled with all values used by the effect callback, to make the call consistent.

However, this array of dependencies have itself a side-effect... of triggering the effect.

Therefore, it is not really just a dependency array, but more a dependenciesAndTriggers array. This leads to confusion, and makes it nearly impossible to correctly control the triggering an effect. Most common issue is with function dependencies : people omit fucntions from the dependencies because they don't want an effect ot fire just because an onChange prop passed by the parent changed.

This documentation partially address this issue but it doesn't apply correctly when a function dependency to the effect is passed by the parent, useCallback is useless in this scenario. The ref alternative is correctly documented but is far-fetched and complicated pattern.

Possible solutions

Main limitation: user should first try to understand how to set their dependencies correctly, and try to memoize callbacks etc. Adding a triggers parameter might lead to a "lazy" approach, where devs use this parameter all the time instead of digging memoization, leading to reduced performances.

For instance, in this comment: https://github.com/facebook/react/issues/14920#issuecomment-471070149, the answer to "react to Compound Value change" the answers from @gaearon states that maybe it should be handled by an event handler (onChange, onClick, onSubmit). That's what you would do before hooks existed. This is not clear to me. useEffect seems on the contrary to be a cleaner approach: event handlers update the states, and the state updates generate side-effects. A bit like you'd separate Redux reducers from Sagas. This is in particular relevant when you have zillions of event handlers that could alter the state: you don't want all of them to track down what kind of side-effect the state change should trigger. Instead, you just update the state, and let the effect do its thing.

Related

Codesandbox

In this example, Child is triggering an onChange prop when some state has changed. But the effect is triggered anytime the parent component rerenders, because it recreates an onChange function. We suppose we can only modify the Child, eg because Parent is too complex, a 3rd party component, owned by another team... useCallback won't work in this scenario.

https://codesandbox.io/s/fancy-sea-zj5e4

Sangrene commented 3 years ago

It might be great indeed to have a mechanism to control the useEffect trigger. Either by passing a list of "triggers" as @eric-burel suggests or have a shouldTrigger function as useEffect last argument that takes the previous and next dependencies state array as argument and returns a boolean indicating wether or not the useEffect callback should trigger. A bit like React.memo actually.

voliva commented 3 years ago

I don't fully understand how this would help though. React doesn't need to know what does that effect depend on. The list of dependencies is just to prevent stale variables to surface up because of how Javascript closures work.

This sentence right here The callback is still correctly updated whenever a dependency change, but not run doesn't make sense to me.

How would this work?

const [value, setValue] = useState(0);
const [id, setId] = useState(1);

useEffect(() => {
  const token = subscribeToEntity(id, (update) => {
    bumpSomethingElse(update.score + value);
  });
  return () => unsubscribeFromEntity(token)
}, [value, id], [id])

If id is the trigger, and value is just a dependency that shouldn't retrigger the effect, how does React "update" the captured value on the first run after it changes? It literally can't unless it re-runs the effect function.

The only possibility would be if the dependencies are supplied into an object which is mutable:

const [value, setValue] = useState(0);
const [id, setId] = useState(1);

useEffect((dependencies) => {
  const token = subscribeToEntity(id, (update) => {
    bumpSomethingElse(update.score + dependencies.value);
  });
  return () => unsubscribeFromEntity(token)
}, {value}, [id])

Only in this way React would be able to "update" each dependency value without needing to re-run the effect, by just mutating the inner dependencies object. That's however a big change, and I'm not sure I like this pattern either.

I agree with OP that dealing with the dependency array in useEffect it's a hard problem. The linter will only cover the simple cases, but for some other (where you would ignore the linter's warning) can become quite tricky, specially when you have variables you don't want to retrigger the effect, or have things that are not dependencies but should trigger the effect. In those cases, it might need a complete rethinking of how the component should work.

Shrugsy commented 3 years ago

I'm not sure if I'm missing something in the explanation, but how would this be different to merely omitting dependencies from the dependency array?

e.g.

useEffect(cb, [...triggers])

where triggers is actually missing some dependencies.

note: I don't advocate omitting dependencies from the dependency array

eric-burel commented 3 years ago

@Shrugsy I can't tell just from my CodeSandbox. It works as expected. However I guess in some scenarios the latest lastClick could be triggered with a stale onChange? Also the documentation is against this pattern, even though after reading it a few time it's still not completely clear to me.

The docs says:

It is only safe to omit a function from the dependency list if nothing in it (or the functions called by it) references props, state, or values derived from them.

Maybe my example somehow fits this scenario? Because onChange is passed by the parent, it doesn't references props, state, or values derived from them. onChange is passed from the props, but doesn't depend on other props, so it could as well have been defined as a global value. Since lastClick will trigger the effect anyway, you always get a fresh onChange. However, I might miss some point and have a truncated understanding of how it works.

@voliva Much thanks for the detailed answer. I think the missing piece here is how useEffect work. I don't know, and the documentation doesn't really explains the internal, more what is expected from the user in terms of usage. So far I've focused my post from the user standpoint only, for whom the concept of dependency is blurry despite the documentation and the concept of triggers is more intuitive. I'll try to dig the internals.

Btw, the example from the doc there is the perfect illustration of this issue (at the time I write this): the "wrong" syntax and the "corrected" one have totally different behaviour.

jasontlouro commented 2 years ago

Can we just get a ref that points back to the most recent value of state as part of the useState call? I'd love to be able to do this:

const [value, setValue, valueRef] = useState(0);

And then just reference valueRef.current from within effects, timeouts, intervals, event listener functions, etc. The amount of times I have to use the messy ref workaround is frustrating, and this looks like something React would be able to do so easily just by sticking the latest value in the ref on every render. People could then use it if they want it or continue to call useState() as normal otherwise.

eric-burel commented 2 years ago

I wonder if the new useEvent hook could mitigate this issue, at least for callback dependencies: https://github.com/reactjs/rfcs/pull/220 @Sangrene

gfox1984 commented 2 years ago

I wonder if the new useEvent hook could mitigate this issue, at least for callback dependencies: reactjs/rfcs#220 @Sangrene

@voliva's example would then look something like this wouldn't it?

const [value, setValue] = useState(0);
const [id, setId] = useState(1);

const onSubscribe = useEvent(update => {
  bumpSomethingElse(update.score + value);
});

useEffect(() => {
  const token = subscribeToEntity(id, onSubscribe);
  return () => unsubscribeFromEntity(token)
}, [id]);

I find this pretty neat.

github-actions[bot] commented 5 months ago

This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize it yet. If you have any new additional information, please include it with your comment!

github-actions[bot] commented 4 months ago

Closing this issue after a prolonged period of inactivity. If this issue is still present in the latest release, please create a new issue with up-to-date information. Thank you!