infinite loop caused by isInViewRef

[jjenzz]

When calling setRef from this package, it causes an infinite loop if called from within an inline function e.g. when passed as a param to packages like react-merge-ref.

Here's a simple repro of the issue (calling manually from inside an inline ref callback):

https://codesandbox.io/s/sweet-rgb-6werdb?file=/src/App.js

When an inline ref function is used, React will set the ref to null and back again on every render. So, from my quick dive into this package's code I believe this caveat is causing the observer to set up again which is firing setState and re-rendering, which calls setRef with null (and back again), and loops.

[thebuilder]

Hey @jjenzz, see the FAQ. If your application code needs access to the ref, then you must make a stable useCallback function.

There's no perfect way to solve this (that I'm aware of) - The library needs to use the callback ref function, since this is the only way it can correctly determine when the ref is set/changed.

Valid use cases I've seen include:

• Not setting the ref on initial render (maybe a spinner is shown)
• Removing the ref after initial render
• Moving the ref between elements
• Toggling the ref based on UI state

All of these require the IntersectionObserver to be recreated, to correctly determine the new state.

[jjenzz]

There's no perfect way to solve this (that I'm aware of)

The lib is calling setState with a new object reference regardless of whether the intersection value has actually changed (causing an infinite loop). Checking for change there would prevent it:

• https://codesandbox.io/s/epic-pare-2d9w9j?file=/src/App.js
• https://codesandbox.io/s/boring-field-2qx7rw?file=/src/App.js

Another approach is to take advantage of React's batching to set the node in state:

https://codesandbox.io/s/fervent-wood-h7jj1g?file=/src/App.js

There are likely other ways I haven't considered but it does seem that the lib should be able to handle it.

[thebuilder]

Thank you for the ideas, @jjenzz. 🙏
I'll try and see if I can come up with a stable solution - It has been bugging me, but just seemed like it was that way across the React ecosystem.

Is it safe to use batching/setState instead of the useCallback for handling ref? Or any caveats to be aware of?

[jjenzz]

Is it safe to use batching/setState instead of the useCallback for handling ref? Or any caveats to be aware of?

All safe as far as I know. A callback ref expects a function and setState is a function after all. Only caveat is an additional render on mount.

[thebuilder]

Seems to be causing quite a few extra renders - You already get two because of setState when the initial inView is set.

I've tried updating the callback, so it compares the entry with previous entry before updating state.

(inView, entry) => {
  setState((prevState) => {
    if (!mounted) {
      mounted = true;

      if (
        prevState.entry &&
        prevState.entry.target === entry.target &&
        prevState.entry.isIntersecting === entry.isIntersecting &&
        prevState.entry.intersectionRatio === entry.intersectionRatio
      ) {
        // Hook has previously been used. Don't update the state
        return prevState;
      }
    }
    return {
      inView,
      entry,
    };
  });

  if (entry.isIntersecting && triggerOnce && unobserve.current) {
    // If it should only trigger once, unobserve the element after it's inView
    unobserve.current();
    unobserve.current = undefined;
  }
},

It's kinda working, but if you are scrolling slowly on the page, it will trigger multiple updates since the intersectionRatio keeps changing. In general this approach will cause the entry to be incorrect. It might be fine, but if a consumer is using specific values (like time, intersectionRatio, intersectionRect), then it becomes unreliable.

It will also recreate the IntersectionObserver on each render, instead of reusing it the instance - Would have to make the cleanup function lazy, which is going to cause extra headaches with async timeouts.

[thebuilder]

Tried with batching setState instead, and that seems to be working nicely. But it is triggering 3 renders, instead of 2 when mounted. Will try it out some more. 🤞

[jjenzz]

are you in StrictMode on dev when you see 3 renders? React will trigger an extra render in this case on purpose.

[thebuilder]

are you in StrictMode on dev when you see 3 renders? React will trigger an extra render in this case on purpose.

No, StrictMode would double the renders. You can see it in your CodeSandbox (updated with the changes from the PR)

The following happens on every mount of the Hook:

1. Initial render, set the ref on HTML element
2. State updated with ref
3. Callback from IntersectionObserver with a new state

[jjenzz]

1. Initial render, set the ref on HTML element
2. State updated with ref
3. Callback from IntersectionObserver with a new state

Ah, yeah that makes sense, there's not much you can do about that tho with this approach.

[thebuilder]

🎉 This issue has been resolved in version 9.4.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[heikomat]

I'm a little confused about the number of initial renders.
In the PR, a commit switches from useCallback to useEffect, a later commit reverts that and another later commit undoes that revert, so that in the end, useEffect is used.

Does that mean that we have to live with 3 renders on mount, or was another solution found, that results in 2 renders on mount, even with the useEffect?

Am i misinterpreting something here?

[thebuilder]

It's doing 3 renders at the moment. I tried different solutions, but the others still had issues. So ended up with useState