Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

React callback ref cleanup function #15176

Open
k15a opened this issue Mar 21, 2019 · 19 comments
Open

React callback ref cleanup function #15176

k15a opened this issue Mar 21, 2019 · 19 comments

Comments

@k15a
Copy link

k15a commented Mar 21, 2019

At the time React added callback refs the main use case for them was to replace string refs. A lot of the callback refs looked like this:

<div ref={node => this.node = node} />

With the introduction of createRef and useRef this use case is pretty much replaced by these alternatives so the use case of callback refs will shift to advanced use cases like measuring DOM nodes.

It would be nice if you could return a cleanup function from the callback ref which is called instead of the callback with null. This way it will behave more like the useEffect API.

<div ref={node => {
  // Normal ref callback

  return () => {
    // Cleanup function which is called when the ref is removed
  }
}} />

This will be super helpful when you need to set up a Resize-, Intersection- or MutationObserver.

function useDimensions() {
  const [entry, setEntry] = useState()
  
  const targetRef = useCallback((node) => {
    const observer = new ResizeObserver(([entry]) => {
      setEntry(entry)
    })

    observer.observe(node)

    return () => {
      observer.disconnect()
    }
  }, [])

  return [entry, targetRef]
}

function Comp() {
  const [dimensions, targetRef] = useDimensions()

  return (
    <pre ref={targetRef}>
      {JSON.stringify(dimensions, null, 2)}
    </pre>
  )
}

Currently, if you want to implement something like this you need to save the observer into a ref and then if the callback ref is called with null you have to clean up the observer from the ref.

To be 99% backward compatible we could call both the callback ref with null and the cleanup function. The only case where it isn't backward compatible is if currently someone is returning a function and doesn't expect the function to be called.

function ref(node) {
  if (node === null) {
    return
  }

  // Do something

  return () => {
    // Cleanup something
  }
}
@bisubus
Copy link

bisubus commented May 9, 2019

In your example targetRef looks more like an effect, I'd expect it to be implemented with useRef and useEffect, which has cleanup function. As for callback ref that doesn't hold state, node === null seems like a legit condition to do a cleanup if necessary.

@k15a
Copy link
Author

k15a commented May 11, 2019

You can't use useEffect there as if you want to pass the ref down to a component useEffect is not necessarily triggered when the subtree updates.

@butchler
Copy link

I would also like a useEffect-style API for interacting with refs. It is particularly useful when you want to attach event listeners to an element and clean them up when it is unmounted.

Motivating example

As far as I can tell, this is what's required to safely do this with callback refs:

import React, { useRef, useCallback } from 'react';

export default function CallbackRefComponent() {
  const elementRef = useRef();
  // Can't safely use useCallback() to define event handler because the memoized
  // value may be freed, and we must make sure to pass the same value to
  // addEventListener and removeEventListener
  const onClickRef = useRef();
  const callbackRef = useCallback((node) => {
    // Cleanup event listener on old node
    if (elementRef.current && onClickRef.current) {
      elementRef.current.removeEventListener('click', onClickRef.current);
      onClickRef.current = null;
    }

    elementRef.current = node;

    if (elementRef.current) {
      onClickRef.current = () => console.log('clicked!');
      elementRef.current.addEventListener('click', onClickRef.current);
    }
  }, []);

  return <div ref={callbackRef}>test</div>;
}

On the other hand, with a useEffect-style API, we can define the event handler in the effect callback itself, and the code becomes much simpler:

import { useRefEffect } from '???';
export default function CallbackRefEffectComponent() {
  const callbackRef = useRefEffect((node) => {
    const onClick = () => console.log('clicked!');

    elementRef.current.addEventListener('click', onClick);

    return () => {
      elementRef.current.removeEventListener('click', onClick);
    };
  });

  return <div ref={callbackRef}>test</div>;
}

Attempted implementation

I've attempted implementing useRefEffect as follows, but I have doubts about the safety of this implementation:

import { useRef, useCallback } from 'react';

export default function useRefEffect(callback) {
  const currentCallback = useRef(callback);
  // WARNING: Mutating a ref's value as a side effect of the render function
  // being called is probably a big anti-pattern, and might cause bugs
  //
  // However, useEffect and useLayoutEffect (which we would normally use for
  // updating a ref to the latest value) get triggered before callback refs.
  // Just mutating it in the render function is the only way I could think of to
  // update the value before callback ref is triggered by React.
  currentCallback.current = callback;

  const cleanupRef = useRef(null);

  const callbackRef = useCallback(node => {
    if (node) {
      cleanupRef.current = currentCallback.current(node) || null;
    } else if (cleanupRef.current) {
      cleanupRef.current();
      cleanupRef.current = null;
    }
  }, []);

  return callbackRef;
}

In particular, I don't know if it's safe to update the value of currentCallback.current as a side effect of render in this case. On the one hand, this value is only used in a callback ref, which is triggered by React, so maybe we can assume that the most recent render had the correct value. On the other hand, causing side effects in render is an anti-pattern in general, and it's hard to be confident that this will always do the right thing even if running in concurrent mode and even if the return value of useRefEffect gets passed through multiple layers of components with React.memos/shouldComponentUpdates in between.

I would greatly appreciate it if someone from the React team (or someone who knows more about the internals of React) could judge my implementation of useRefEffect and tell me if it is making any unsafe assumptions.

@k15a
Copy link
Author

k15a commented Jul 17, 2019

@butchler I think what you want is exactly what I am proposing just without the need of a new hook.

export default function CallbackRefEffectComponent() {
  const callbackRef = useCallback((node) => {
	// For backwards compatibility you would need to check if the node is null

    const onClick = () => console.log('clicked!');

    node.addEventListener('click', onClick);

    return () => {
      node.removeEventListener('click', onClick);
    };
  }, []);

  return <div ref={callbackRef}>test</div>;
}

@butchler
Copy link

I think what you want is exactly what I am proposing just without the need of a new hook.

Basically 🙂

There is a small difference in behavior with my attempt at implementing it as a hook and your proposal: because you are using useCallback, if you need to use props/state/etc. inside the ref callback, it is necessary to pass them to useCallback's second argument, but this also has the side effect of re-triggering the callback ref, which may not always be desired. However, I realize now that this issue is not directly related to this proposal, but is a more general issue with callback refs and hooks, so I'll open a separate issue for that.

Here is another attempt at implementing it via a hook, that more closely matches your proposal:

import { useRef, useCallback } from 'react';

export default function useCallbackRef(rawCallback, deps) {
  const cleanupRef = useRef(null);
  const callback = useCallback((node) => {
    if (cleanupRef.current) {
      cleanupRef.current();
      cleanupRef.current = null;
    }

    if (node) {
      cleanupRef.current = rawCallback(node);
    }
  }, deps);

  return callback;
}

Then you could just use useCallbackRef in place of useCallback.

Unfortunately, this has a pretty big downside, which is that useCallbackRef will probably not be checked by the exhaustive-deps linting rule. This isn't an issue with my proposed useRefEffect implementation, because it does not require you to pass deps and only triggers the callback when the ref actually changes (but as a result has to use a potentially dirty hack to keep a reference to the latest version of the callback).

@butchler
Copy link

butchler commented Jul 18, 2019

One reason that the original proposal (of adding new behavior to the ref prop) might not work is that callback refs get called with null when the element is unmounted or changed, but when returning a cleanup function, you probably only want it to to be called with an actual node once, and never get called with null (otherwise you'd have to manually check if the argument is null, which somewhat defeats the purpose).

React could try to detect if a cleanup function has been returned and change the behavior for that particular callback ref, but that seems quite confusing and would probably make it easy to introduce bugs.

It would be nice if it's possible to implement this as a custom hook outside of React itself, but I'm not sure how it can be implemented reliably.

@butchler
Copy link

Yet another attempt at implementing this as a custom hook:

import { useRef, useCallback } from 'react';

export default function useCallbackRef(rawCallback) {
  const cleanupRef = useRef(null);
  const callback = useCallback((node) => {
    if (cleanupRef.current) {
      cleanupRef.current();
      cleanupRef.current = null;
    }

    if (node) {
      cleanupRef.current = rawCallback(node);
    }
  }, [rawCallback]);

  return callback;
}

Usage:

const callback = useCallbackRef(useCallback((node) => {
  node.addEventListener(...);
  return () => {
    node.removeEventListener(...);
  };
}, []));

It's a bit more cumbersome to use, since you have to call both useCallback and useCallbackRef, but at least it allows the deps to be checked by the exhaustive-deps linting rule.

@k15a What do you think of this approach?

@vincerubinetti
Copy link

vincerubinetti commented Nov 15, 2019

I'm very new to React hooks and still don't quite get how they work under the hood. That being said, here's something I came up with that seems to work:

export const useBbox = () => {
  const ref = useRef();
  const [bbox, setBbox] = useState({});

  const set = () =>
    setBbox(ref && ref.current ? ref.current.getBoundingClientRect() : {});

  useEffect(() => {
    set();
    window.addEventListener('resize', set);
    return () => window.removeEventListener('resize', set);
  }, []);

  return [bbox, ref];
};

Then to use it:

const SignIn = () => {
  const [bbox, ref] = useBbox();

  return (
    <>
      <Button ref={ref}>open popup</Button>
      <Popup anchorBbox={bbox}>popup content</Popup>
    </>
  );
};

This is modeled after this example in the React docs, but is modified to also update the bbox any time the window is resized (which may or may not be enough for your use case; use this with caution).

It seems to unmount the listener properly, but there could definitely be something I missed. Comments are welcome.


If anyone reading this is looking for the same functionality, here is another solution I found:
https://gist.github.com/morajabi/523d7a642d8c0a2f71fcfa0d8b3d2846

Also here's an example of a library using hooks to update positions of a popup, but with added debouncing and other stuff:
https://github.com/mui-org/material-ui/blob/master/packages/material-ui/src/Popover/Popover.js

@butchler
Copy link

@vincerubinetti Indeed, useRef+useEffect is a perfectly fine alternative to using a callback ref in most cases, and when using that approach you can just do your cleanup using useEffect. However, there are still some (rare) cases where it's necessary or useful to use a callback ref instead, in which case doing cleanup is a little more cumbersome compared to using useEffect.

So I'd say that is slightly out of scope of this issue on the React project itself, although it is still a good alternative to know about when writing code that uses React.

@bekliev

This comment has been minimized.

@otakustay
Copy link
Contributor

I made a useEffectRef in @huse/effect-ref to solve exactly the same problem

@butchler
Copy link

butchler commented Mar 2, 2020

Given that updating the behavior of the ref prop would probably be tricky (see #15176 (comment)), and that several people have been able to implement some version of this as custom hooks, it seems unlikely that this will be added to React and this issue can probably be closed.

That said, for anybody who wants to use the useCallback-based approach that people have mentioned in the above comments along with the this pattern for accessing often-changing values inside useCallback, be warned that you may run into a really weird edge case if you get really unlucky: #16154

@Fi1osof
Copy link

Fi1osof commented Mar 10, 2021

@k15a thanks a lot!
It's works for me. I lost a lot of time to use useEffect, but it's hard to catch unmount event. fer function helps. Thanks!

@KurtGokhan
Copy link

KurtGokhan commented Sep 5, 2021

I am going to mention one use-case that cannot be solved by any of the workarounds listed here. It happens when you want to use the same ref callback for multiple elements.

As a simple example, let's say I want to write a ref that adds all elements to an array, and remove them from the array when they are removed from DOM. I should be able to simply write:

const list = [];

function Test() {
  const register = useCallback((el) => {
    if (!el) return;

    list.push(el);
    
    return () => {
      const index = list.indexOf(el);
      if (index >= 0) list.splice(index, 1);
    };
  }, []);

  return <>
    <span ref={register} />
    <span ref={register} />
    <span ref={register} />
  </>;
}

I don't know if other people would think of this use-case as a valid usage of ref, but I think it is a beautiful code pattern that both saves performance and provides an easy API.

As far as I know, there is no easy workaround for this. One library I know which uses this kind of pattern is react-hook-form. It solves the issue by making register a function and calling it with a unique name for each element like <input ref={register("password")} />. As for me, I was going to build a reusable tooltip hook but it's not going to be that easy without this functionality.

@gaearon
Copy link
Collaborator

gaearon commented Sep 8, 2021

Does anyone want to write an RFC for this?

https://github.com/reactjs/rfcs/

@KurtGokhan
Copy link

@gaearon I will try writing one.

@KurtGokhan
Copy link

I created the RFC. Let's keep discussing this there.

@steve-taylor
Copy link

steve-taylor commented Jan 8, 2024

Svelte handles this nicely with use directives.

<script lang="ts">
    function handleButton(button: HTMLButtonElement) {
        const onClickButton = (event: PointerEvent) => {
            // Do something here
        }

        button.addEventListener('click', onClickButton)

        return {
            destroy() {
                button.removeEventListener('click', onClickButton)
            }
        }
    }
</script>

<button use:handleButton>
    Click me
</button>

The problem with React is that we can't use useRef and useEffect, because if the ref's element changes, it doesn't trigger an effect.

This won't work:

type Props = {
    wrap: boolean
}

function MyComponent({wrap}: Props) {
    const ref = useRef<HTMLButtonElement>(null)

    const onClick = useCallback((event: PointerEvent) => {
        // Do something here
    }, [])

    // WARNING: When wrap changes, ref.current will change, but this effect won't be triggered,
    //          so the new button element won't have a listener.
    useEffect(() => {
        const button = ref.current

        if (button) {
            button.addEventListener('click', onClick)

            return () => button.removeEventListener('click', onClick)
        }
    }, [onClick])

    const buttonElement = (
        <button ref={ref}>
            Click me
        </button>
    )

    return wrap ? (
        <div>
            {buttonElement}
        </div>
    ) : buttonElement
}

Instead, we need a callback ref, but since callback refs don't have clean-up logic, we have to do it ourselves:

type Props = {
    wrap: boolean
}

function MyComponent({wrap}: Props) {
    const previousButtonRef = useRef<HTMLButtonElement>(null)

    const onClick = useCallback((event: PointerEvent) => {
        // Do something here
    }, [])

    const ref = useCallback((button: HTMLButtonElement | null) => {
        previousButtonRef?.removeEventListener('click', onClick)
        previousButtonRef.current = button;
        button?.addEventListener('click', onClick);
    }, [onClick])

    const buttonElement = (
        <button ref={ref}>
            Click me
        </button>
    )

    return wrap ? (
        <div>
            {buttonElement}
        </div>
    ) : buttonElement
}

Obviously this is a contrived scenario because both these frameworks have click event handling built-in, but there are plenty of examples where we want to maintain a reference to a DOM element and perform clean-up when that reference changes. useEffect doesn't cut it, because a ref value change doesn't trigger it.

@rickhanlonii
Copy link
Member

Following up from the RFC thread: this is landed in Canary and will ship in the next version of React: #25686

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests