-
Notifications
You must be signed in to change notification settings - Fork 47.3k
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
Comments
In your example |
You can't use |
I would also like a Motivating exampleAs 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 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 implementationI've attempted implementing 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 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 |
@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>;
} |
Basically 🙂 There is a small difference in behavior with my attempt at implementing it as a hook and your proposal: because you are using 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 Unfortunately, this has a pretty big downside, which is that |
One reason that the original proposal (of adding new behavior to the 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. |
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 @k15a What do you think of this approach? |
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: Also here's an example of a library using hooks to update positions of a popup, but with added debouncing and other stuff: |
@vincerubinetti Indeed, 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. |
This comment has been minimized.
This comment has been minimized.
I made a |
Given that updating the behavior of the That said, for anybody who wants to use the |
@k15a thanks a lot! |
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 As far as I know, there is no easy workaround for this. One library I know which uses this kind of pattern is |
Does anyone want to write an RFC for this? |
@gaearon I will try writing one. |
I created the RFC. Let's keep discussing this there. |
Svelte handles this nicely with <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 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 |
Following up from the RFC thread: this is landed in Canary and will ship in the next version of React: #25686 |
I've generally used the |
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:
With the introduction of
createRef
anduseRef
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.This will be super helpful when you need to set up a Resize-, Intersection- or MutationObserver.
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.
The text was updated successfully, but these errors were encountered: