React state resets when set from prop event handler

Question

Cannot for the life of me figure out what is going on, but for some reason when "Click me" is clicked, the number increments as you'd expect. When a click is triggered by the Child component, it resets the state and ALWAYS prints 0.

function Child(props: {
    onClick?: (id: string) => void,
}) {
    const ref = useCallback((ref) => {
        ref.innerHTML = 'This Doesnt';
        ref.addEventListener('click',() => {
            props.onClick!('')
        })
    }, [])
    return (<div ref={ref}></div>)
}

function Parent() {
  const [number, setNumber] = useState(0);
  return <div>
            <div onClick={() => {
                setNumber(number + 1);
                console.log(number);
            }}>
                This Works
            </div>
            <Child
                onClick={(id) => {
                    setNumber(number + 1);
                    console.log(number);;
                }}
            />
        </div>
}

And here is a demonstration of the problem: https://jscomplete.com/playground/s333177

What is the reason for `useCallback()` in SVGView? I wonder if the memoization of this may be the cause of your problem? — Dacre Denny, Sep 18 '19 at 22:48
In that case, https://reactjs.org/docs/hooks-reference.html#useref might be a better fit — Dacre Denny, Sep 18 '19 at 22:50
Is the bang on `props.onClick!('')` something special for Typescript? Also, why are you wrapping `onClick` in a closure instead of just passing it directly to `addEventListener` as the callback argument? — coreyward, Sep 18 '19 at 23:57
The bang is for typescript and is irrelevant. It doesn't need to be wrapped in this instance. I removed a lot of code here trying to boil it down but the actual version of this sends some data to the onClick method — Jordan Soltman, Sep 18 '19 at 23:58
Another thing: using `[]` as the second argument to `useCallback` is going to result in a stale `onClick` reference as soon as the function is rerendered. — coreyward, Sep 18 '19 at 23:59
That's how hooks work. The second argument to `useCallback` is an array of dependencies. When any of your dependencies update, the callback function is redeclared; otherwise React hands back the last value. And since every time your parent component `setNumber` is called it rerenders, and you're creating your `onClick` handlers inline, those functions get updated too. So as soon as the number is incremented, your SVGView is going to be referring to closures that hold references to variables that are out of scope. — coreyward, Sep 19 '19 at 00:05
Ah, I see what you are saying. Still trying to get the hang of hooks but that makes sense. I'm trying to find a way to not redraw all my SVG on every change but it's tricky. — Jordan Soltman, Sep 19 '19 at 00:12
Looks like this might be the same: https://stackoverflow.com/questions/53845595/wrong-react-hooks-behaviour-with-event-listener — Jordan Soltman, Sep 19 '19 at 00:37

ckedar · Accepted Answer · 2019-09-20T06:00:42.670

Both the onClick handlers in parent component are re-created on every render, rightly so as they have a closure on number state field.

The problem is, the onClick property sent to Child component is used in ref callback, which is reacted only during initial render due to empty dependency list. So onClick prop received by Child in subsequent renders does not get used at all.

Attempt to resolve this error, by either removing dependency param or sending props.onClick as in dependency list, we land into issue due to caveat mentioned in documentation. https://reactjs.org/docs/refs-and-the-dom.html

So you add null handling, and you see your updated callback now getting invoked, but... all earlier callbacks are also invoked as we have not removed those event listeners.

const ref = useCallback((ref) => {
    if(!ref) return;
    ref.innerHTML = 'This Doesnt';
    ref.addEventListener('click',() => {
        props.onClick!('')
    })
}, [props.onClick])

I believe this just an experiment being done as part of learning hooks, otherwise there is no need to go in roundabout way to invoke the onClick from ref callback. Just pass it on as prop to div.

Edit:
As per your comment, as this is not just an experiment but simplification of some genuine requirement where click handler needs to be set through addEventListener, here is a possible solution:

const ref = useRef(null);
useEffect(() => {
    if(!ref.current) return;
    ref.current.innerHTML = 'This Doesnt';
    const onClick = () => props.onClick!('');
    ref.current.addEventListener('click',onClick)

    // return the cleanup function to remove the click handler, which will be called before this effect is run next time.
    return () => {ref.current.removeEventListener("click", onClick)}
}, [ref.current, props.onClick]);

Basically, we need to use useEffect so that we get a chance to remove old listener before adding new one.

Hope this helps.

The reasoning for all of it is lost in my simplification. I have an SVG view class that does a lot of manipulation of SVGs and as such is using snapsvg. I’m adding on click events to various svg elements and have been trying to avoid parsing and re-rendering the entire svg every time the props change but it’s looking like if I’m going to use hooks my best bet will be reestablishing the onclick handlers after each render. Might just revert to using a class for this case. — Jordan Soltman, Sep 19 '19 at 15:40
Oops. Just noticed that @kwdowik has provided similar solution. — ckedar, Sep 20 '19 at 06:03
Thanks. That’s essentially what I ended up doing. Realistically this all ended up being messier than just using a class but at least it was interesting! Thanks again. — Jordan Soltman, Sep 20 '19 at 07:57

score 1 · Answer 2 · answered Sep 19 '19 at 12:45

function Child(props: {
    onClick?: (id: string) => void,
}) {
    function handleClick() {
      props.onClick('')
    }
    const ref = useRef();
    useEffect(() => {
      ref.current.innerHTML = 'This Doesnt';
      ref.current.addEventListener('click', handleClick)
      return () => { ref.current.removeEventListener('click', handleClick); }
    }, [props.onClick])

    return (<div ref={ref}></div>)
}

@ckder almost works but console.log displayed all numbers from 0 to current number value. Issue was with event listener which has not been remove after Child component umount so to achive this I used useEffect and return function where I unmount listener.

React state resets when set from prop event handler

2 Answers2