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

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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.