ampproject / amp-react-prototype

A scratch pad to experiment with React rendered AMP Components
Apache License 2.0
36 stars 9 forks source link

Rerun element-based hook when element changes #45

Closed dvoytenko closed 4 years ago

dvoytenko commented 4 years ago

The context: useResizeEffect hook. This hook takes two arguments: an element's ref and a callback function. The callback function is called whenever the element is resized, e.g. due to responsive styling. This hook only subscribes/unsubscribes to/from ResizeObserver when mounted/unmounted to reduce waste.

Currently the issue is that ref.current could change and this code does not take this into account. I see to options to remedy this:

/1/ Use dependency-based effect.

function useResizeEffect(ref, callback) {
  useEffect(() => {...}, [ref.current]);
}

The "bad" part here is that on initial rendering, the useEffect will be called twice: first with ref.current == null and then with ref.current == rendered-element. Thus this version does one extra subscribe/unsubscribe from the ResizeObserver.

/2/ Manually compare elements

This approach is more efficient, but really messy.

function useResizeEffect(ref, callback) {
  const prev = useRef();
  const observer = useRef();
  useEffect(() => {
    if (prev.current != ref.current) {
      if (observer.current) {
        observer.current.unobserve(prev.current);
      } else {
        observer.current = new ResizeObserver();
      }
      observer.current.observe(ref.current);
      prev.current = ref.current;
    }
    return () => {
      if (observer.current) {
        observer.current.disconnect();
        observer.current = null;
      }
    };
  }, [/*mount only*/]);
}

Which of these more idiomatic?

developit commented 4 years ago

If the initial render has ref.current == null, is it really an extra subscribe? My understanding was that ResizeObserver would not be initialized if the ref's current value was null.

I find the second one reasonably idiomatic. If it's unlikely the ref will ever swap between two nodes in need of observation, it could be simplified as follows (incurs extra ResizeObserver if swapped):

function useResizeEffect(ref, callback) {
  const current = ref.current;
  const observer = useRef();
  useEffect(() => {
    if (current) {
      observer.current = new ResizeObserver(callback);
      observer.current.observe(current);
    }
    return () => {
      if (observer.current) {
        observer.current.disconnect();
      }
    };
  }, [current]);
}
dvoytenko commented 4 years ago

If the initial render has ref.current == null, is it really an extra subscribe? My understanding was that ResizeObserver would not be initialized if the ref's current value was null.

Yes. This is a bit of a misconnect with how effects are scheduled and executed when using a ref. In this example:

function useResizeEffect(ref, callback) {
  const current = ref.current;
  useEffect(() => {
    if (current) {             // <- Executed post-rendering so the current is now NOT null.
      ...
    }
    return () => {...};
  }, [current]);                // <- Executed during rendering phase, so current starts as null.
}
dvoytenko commented 4 years ago

Another approach using useCallback: https://github.com/ampproject/amp-react-prototype/pull/46

dvoytenko commented 4 years ago

Dedupping this to #55.