Feature proposal(eslint-react-hooks): don't require empty dependency useCallback in another dependency array

[georeith]

Consider the following:

  const [mouseDown, setMouseDown] = useState(false);
  const onMouseDown = useCallback(() => { 
    setMouseDown(true);
  }, []);
  const onMouseMove = useCallback(e => { /* ... */ }, []);
  const onMouseUp = useCallback(() => {
    setMouseDown(false);
  }, []);
  useEffect(() => {
    if (mouseDown) {
      document.addEventListener('mousemove', onMouseMove);
      document.addEventListener('mouseup', onMouseUp);
      return () => {
        document.removeEventListener('mousemove', onMouseMove);
        document.removeEventListener('mouseup', onMouseUp);
      }
    }
  }, [mouseDown]);

Above the useEffect() will complain that it didn't receive onMouseMove and onMouseUp in its dependency array and it's correct. But it could be smarter, because they were all defined by useCallback(() => {}, []) meaning they will all remain the same value throughout the lifetime of this component.

Given this information I could write

  useEffect(() => {
    if (mouseDown) {
      document.addEventListener('mousemove', onMouseMove);
      document.addEventListener('mouseup', onMouseUp);
      return () => {
        document.removeEventListener('mousemove', onMouseMove);
        document.removeEventListener('mouseup', onMouseUp);
      }
    }
  }, [mouseDown, onMouseMove, onMouseUp]);

To satisfy the linter and that would work but only because I know they were defined by useCallback(() => {}, []). If someone were to change the dependency array for onMouseMove or onMouseUp this would now break (the event listeners won't be removed and readded if onMouseMove changes for instance), but the linter will be happy.

However if I was able to specify it like I did in the first example it is the same as saying, this works as long as these specific variables don't change, if someone unwittingly changes the dependency array of onMouseMove the linter would shout at them again and they would have to rewrite this into something more flexible.

This is similar to the way useCallback doesn't complain about my usage of setMouseDown as it knows it can't change.

[bvaughn]

This looks like a substantial change being proposed. 😄 Changes like this should go through our RFC process:
https://github.com/reactjs/rfcs#react-rfcs

The RFC process is a great opportunity to get more eyeballs on your proposal before it becomes a part of a released version of React. Quite often, even proposals that seem "obvious" can be significantly improved once a wider group of interested people have a chance to weigh in.

The RFC process can also be helpful to encourage discussions about a proposed feature as it is being designed, and incorporate important constraints into the design while it's easier to change, before the design has been fully implemented.

[georeith]

@bvaughn how is this a substantial change? It already does it for the function returned by useState. Its just a lint rule exception 🤔

EDIT: it is the exact same thing as #19125

[bvaughn]

Admittedly I skimmed this pretty quickly, saw that you were proposing a change to the dependencies array, and closed it with a canned response. If I had scanned more carefully I probably would have left it open for discussion. My apologies. (I don't think it's quite the same as #19125 or I would suggest leaving it closed as a duplicate.)

The updater function returned by useState, or the dispatch function returned by useReducer, are known by React to be stable. Other values are not, although I see why in this case, you could reason that the functions are stable.

[georeith]

@bvaughn no problem, thanks for reopening, I didn't really know the language to explain it in the right terms (stable vs unstable) so I can see how it's an easy mistake to make.

[bvaughn]

No prob.

To give you a little more insight, there are only ~6 core contributors to this repo right now and we're spread pretty thin. If you feel strongly about this issue, you'd probably have a lot better luck championing it through by submitting a PR (with tests!) 😄

[thien-do]

Hi, thanks for opening the issue. I have a similar (but not exactly the same) when I use values from 3rd party libraries that are guarantee to be stable but the lint doesn't know about them (like "useState", but from other libraries).

So basically I'm seeing there are 2 cases:

1. Something is stable because they are guaranteed to be so (for example, the value returned by Recoil's useRecoilSetState)
2. Something is stable because recursively they depend on things that are also logically stable (for example, onMouseDown in above samples)

To give you a little more insight, there are only ~6 core contributors to this repo right now and we're spread pretty thin. If you feel strongly about this issue, you'd probably have a lot better luck championing it through by submitting a PR (with tests!) smile

For starting, is it ok to PR to address the 1st use case first? I mean allow additional values to the knownStableValues array? I think it's simpler.

I don't have experience working with linter so I don't really know how much is needed to implement (2) or will it have performance concerns

[gaearon]

Something is stable because they are guaranteed to be so (for example, the value returned by Recoil's useRecoilSetState)

The problem with making it configurable is that as a result, each library will recommend additions to the configuration. That's bad because it means everybody has to change their configs and defaults don't work. It has a rippling effect on the ecosystem.

Instead, the solution is to simply keep these dependencies in the array. If they are truly stable, this will not cause any problems.

[gaearon]

But it could be smarter, because they were all defined by useCallback(() => {}, []) meaning they will all remain the same value throughout the lifetime of this component.

Why useCallback at all in this example? What are you trying to achieve? Why not use plain functions?

[gaearon]

If someone were to change the dependency array for onMouseMove or onMouseUp this would now break (the event listeners won't be removed and readded if onMouseMove changes for instance), but the linter will be happy.

We might have somewhat different definitions of "break".

If onMouseMove used some prop, and that prop changed, it would be correct that the event would get reattached. Since otherwise it would "see" the old prop value. So in that sense, the linter would do the right thing.

There are other solutions if you really don't want that event to be re-attached (such as using refs). But it's hard to say why in this example it would be important. Adding and removing event handlers is extremely cheap.

[thien-do]

Why useCallback at all in this example? What are you trying to achieve? Why not use plain functions?

I guess because the function needs access to a ref maybe

The problem with making it configurable is that as a result, each library will recommend additions to the configuration. That's bad because it means everybody has to change their configs and defaults don't work. It has a rippling effect on the ecosystem.

Thank you. I understand that's bad in the long term. I'm glad to put them all in the dependencies array now

[georeith]

@gaearon there were some issues in my example and I did since find a better pattern with fresher eyes (it had been a long day). I actually edited it from the erroneous (which would have done bad things if onMouseDown or onMouseUp changed):

 useEffect(() => {
    if (mouseDown) {
      document.addEventListener('mousemove', onMouseMove);
      document.addEventListener('mouseup', onMouseUp);
    } else {
      document.removeEventListener('mousemove', onMouseMove);
      document.removeEventListener('mouseup', onMouseUp);   
    }
  }, [mouseDown, onMouseDown, onMouseUp]);

That prompted me to ask the original question. The example in my other comment is actually OK now and doesn't depend upon values being stable at all.

Why useCallback at all in this example? What are you trying to achieve? Why not use plain functions?

It's entirely unnecessary for onMouseMove and onMouseUp here, it's used on onMouseDown to prevent unnecessary re-renders in a child component.

Regardless that I was doing the wrong thing in my example (which I since edited, sorry for the confusion) someone might find it useful, is there anything wrong with marking something provably stable as stable if it helps catch errors ahead of time?

[gaearon]

I don't think we want to do this.

The problem is, eventually you might want to add some dependency to that callback. Then it's confusing if that starts showing up as an error everywhere that callback was used (rather than in a single place). It really doesn't hurt to include it, so you might as well include it from the beginning.