asyarb / use-intersection-observer

Small React hook wrapper around the IntersectionObserver API.
MIT License
6 stars 3 forks source link

Callback never updated #8

Open fabb opened 4 years ago

fabb commented 4 years ago

While researching another problem, I noticed that useIntersectionObserver is never updating the callback:

  const [intersectObs] = useState(() =>
    IS_BROWSER ? new IntersectionObserver(handleIntersect, options) : undefined
  )

Since the initial state of useState is only executed for the first run of the hook, that means intersectObs will always point to an instance of IntersectionObserver with the handleIntersect function of the first run of the hook.

But handleIntersect changes for every run of the hook, and even when useCallback would be used for it, that still would change depending on its dependency array.

I do not really understand why this seems to make no problems? Theoretically, when either callback or triggerOnce is changing between runs of useIntersectionObserver, it would not pick up these changes, and still execute the first intersectObs callback. Maybe I need to test this differently.

If this is really a problem, a solution would be to use something like this:

    const handleIntersect = useCallback(
        (entries: IntersectionObserverEntry[], observer: IntersectionObserver) => {...},
        [callback, options.triggerOnce]
    )

    // because of useState, the callback function is never updated, therefore we need to use a stable callback that never changes by using an empty dependency array
    const handleIntersectRef = useRef(handleIntersect)
    handleIntersectRef.current = handleIntersect
    const handleIntersectStable = useCallback((entries: IntersectionObserverEntry[], observer: IntersectionObserver) => handleIntersectRef.current(entries, observer), [])
    const [intersectObs] = useState(() => (IS_BROWSER ? new IntersectionObserver(handleIntersectStable, options) : null))

But this also doesn‘t update the options of the IntersectionObserver. Or maybe use useMemo for the IntersectionObserver?

const intersectObs = useMemo(() => (IS_BROWSER ? new IntersectionObserver(handleIntersect, options) : null), [handleIntersect, options])

The two ideas could also be combined to both avoid unnecessary resubscribes when only the callback changes, and allow to change the options.

fabb commented 4 years ago

No, the useMemo is problematic because it would always update on every render, because options is an object, and clients very probably won't make it stable using useMemo or useCallback. This makes it worse, since it would cause re-observing for every render, and also break triggerOnce.

fabb commented 4 years ago

Ah, in this hook, the elements of the options array are just directly used in the deps array: https://github.com/thebuilder/react-intersection-observer/blob/master/src/useInView.tsx#L51

But note that they still have a bug when using an array for threshold, so I'd recommend to use JSON.stringify() in that case: https://github.com/thebuilder/react-intersection-observer/issues/248#issuecomment-599999725