ampproject / ampproject/amp-react-prototype

A safe useEffect for refs

Open
#55 0 comments 0 reactions 0 assignees View on GitHub
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.