I don't think this is the right approach. This creates a problem where the event listeners will no longer be called if the drop zone element has been unmounted and remounted before.

This bug is not easily discoverable because useDropZone is often used in the same render function which assigns the ref to the element.

const ref = useDropZone({ onDragStart });
return <div ref={ref} />

However, if the element getting the ref is somewhere beneath the component's children by passing through different layers of forwardRef, and if one of the children components get remounted at any time, the bug will surface.

const Parent = () => {
  const ref = useDropZone({ onDragStart });
  return <Child ref={ref} />;
};

const Child = forwardRef((props, ref) => {
  return <div ref={ref} />;
});

Since we will not run the render function of Parent again, the event listeners' refs won't get updated to their fresh value, they will continue being null because they have been unmounted before (how ref callbacks or useRefEffect work).

I discovered this bug while working on Popover which unmounts itself (for whatever reason I don't know yet) and in a controlled testing environment.

As for the proper solution to this issue, I can't seem to replicate this warning anymore. It probably relates to the recent update of React to suppress these false positives. Either way, setting the refs here to null doesn't really solve the memory leaks if there's any. Browsers will automatically do GC to clean up those unused listeners anyway. There's likely other reasons behind the warning.

A possible bug might be at line 235?

Why do we want to add the listeners again when unmounting? Possibly a typo here (should be removeEventListener)?

As for the proper solution to this issue, I can't seem to replicate this warning anymore. It probably relates to the recent update of React to suppress these false positives.

Feel free to open a PR reverting these changes, and we can test to see if the warning that I had flagged is still happening.

Why do we want to add the listeners again when unmounting? Possibly a typo here (should be removeEventListener)?

I think so, it was probably meant to be removeEventListener

Browsers will automatically do GC to clean up those unused listeners anyway.

most of what you share sounds reasonable, but I believe this statement isn't accurate. if we're holding on to a ref in those state values the browser won't be able to GC the listeners because we'll still have a reference to the DOM nodes they are attached to.

probably a new fix is needed if you still see problems, and as @ciampo mentioned, a new PR would be a good place to improve this.

if we're holding on to a ref in those state values the browser won't be able to GC the listeners because we'll still have a reference to the DOM nodes they are attached to.

True! But that falls down to why or when we would still have a reference to those values. The ref callbacks fire themselves with the null value when the underlying element unmounts. And the refs will no longer hold the references when the components unmount. Thus, as long as we don't call useDropZone a bunch of times without passing the returned refs somewhere, React has no reason to still hold the values of those functions.

Feel free to open a PR reverting these changes, and we can test to see if the warning that I had flagged is still happening.

Sure! I'm working on #42722 though, so I'll probably just push a commit there since it's blocking the tests. If it's too much noise in that PR I can then split it into a separate PR ;)

Thanks for the quick responses folks!

The ref callbacks fire themselves with the null value when the underlying element unmounts. And the refs will no longer hold the references when the components unmount.

🤔

okay you got me. I don't see where those ref values are cleared out when they unmount. it looks like useFreshRef will still hold a reference to the old values even if useDropZone gets called with null for their values (after the initial render).

could just be my brain isn't working and I'm missing some hook sequencing, but don't we need some onDropRef.current = assignment somewhere up the stack to remove those references?

As long as we're using useRef, I think React will make sure to dispose those refs after unmounting since they aren't referenced anymore and the engine can then GC them. This assumes that useDropZone and the ref callback are in the same render function (component), or at least unmount at the same time. That is, we don't have to manually clear out the values in refs because React will do it for us (sorta, just think of them as local variables).

const A = () => {
  // The lifecycle of this ref only exists in this component (A).
  // Once A unmounts, ref no longer exists, and the value it holds can then be GCed.
  const ref = useRef();
}

it looks like useFreshRef will still hold a reference to the old values even if useDropZone gets called with null for their values (after the initial render).

Yes. It will still hold the value, but only the latest value, which is what we passed to useDropZone in the first place.

but don't we need some onDropRef.current = assignment somewhere up the stack to remove those references?

This is the part I don't understand 🤔, why do we need to remove those references? They will automatically get removed once the component that calls useDropZone is unmounted. Even if we set those to null at some point, on the next render (if there's any), they will again be set to the latest value (via useFreshRef).

FWIW, these functions will only get called if the event handlers aren't properly canceled/removed. Which means that the memory leak is likely because of the event handlers themselves, not the user-specified callbacks.

Thanks for explaining, @kevin940726. I don't have time to build an experiment at this time, but I think I need to see some of this working to fully understand.

It sounds like you're saying we do hold on to the reference to the ref, but it's the .current value of that ref which is actually the DOM node, updated by React to null.

I think that implies if we want access to the DOM node from here we'd have to do something like onDropRef.current.current?

Anyway, I'm just trying to clear my head on this and make sure I understand it all, but it's not important to this PR or your fix. I appreciate you taking the time to help me out.

Oops! Sorry I missed this discussion. @dmsnell do you still have questions/doubts about this? Since this is getting pretty old, I'd be happy to start a new conversation either privately or publicly somewhere! ❤️

It's fine for now @kevin940726 - thanks though. Somewhere in there is a question about the life of the DOM node itself and the life of the ref. If we find that we still have a memory leak we can investigate that avenue.

Have we ever measured if the memory leak this was supposed to fix is actually eliminated after this PR merge?