ampproject/amp-react-prototype

A safe useEffect for refs

dvoytenko opened this issue · 0 comments

The pattern:

const ref = useRef();
useEffect(() => {
  ref.current.addEventListener(...);
  return () => ref.current.removeEventListener(...);
}, [...])

return (
  <div>
     {props.mode === 1 ? <Mode1 ref={ref}/> : <Mode2 ref={ref}/>}
  </div>
);

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 <...><Mode1 ref={setNode}></...>

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.