ampproject / ampproject/amp-react-prototype
A safe useEffect for refs
- Dominant language
- JavaScript
- Stars
- 36
- Forks
- 6
- PR merge metrics
- No merged PRs in 30d
Description
The pattern:
```
const ref = useRef();
useEffect(() => {
ref.current.addEventListener(...);
return () => ref.current.removeEventListener(...);
}, [...])
return (
{props.mode === 1 ? : }
);
```
In this pattern, it's hard to react safely to changes of `ref.current`, e.g. when a node mapped to it is deleted/changes. The "right" way to do this is to ensure that the `deps` array contains the same condition that affects `ref`. E.g. `[props.mode]` in the example above. However, it's not always obvious and easy to miss.
Some solutions are below.
/1/ Ask nicely for `deps` to be correct and hope for the best
Hopefully an "exhaustive deps" linter would not remove the extra dep.
/2/ Ban changing of Ref mapping.
I.e. disallow the example above. This could be hard with `forwardRef`.
/3/ Use state function instead of ref:
```
const [node, setNode] = useState();
useEffect(() => {...}, [node])
return <...>
```
The negative: it forces the second rerender each time the ref changes.
/4/ Use a funky `xEffectWithRef` version.
It'd manage the `ref` value internally and could look something like this:
```
function useEffectWithRef(ref, effect, deps) {
const unsubscribe = useRef(null);
const prev = useRef(null);
useEffect(() => {
return () => doUnsubscribe(prev, unsubscribe);
}, deps || []);
useEffect(() => {
const {current} = ref;
if (current !== prev.current) {
doUnsubscribe(prev, unsubscribe);
prev.current = current;
if (current) {
unsubscribe.current = effect(current);
}
}
});
}
```
The positive: it doesn't cause rerender.
A negative: one effect is executed each time, but it will almost always do nothing.
Contributor guide
Research direction
Start with the useEffect and ref patterns shown in the issue, including the state-based and useEffectWithRef alternatives. Compare their handling of ref changes and cleanup; the issue does not identify a target file, test, or agreed implementation, so a definition of done is not specified.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- javascript, react
- Domain
- frontend
- Issue type
- Feature
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Needs clarification
- Newbie friendliness
- 25/100