fix: remove extra useEffect used for resetting initial state

[calinoracation]

This is an attempt to reduce the amount of re-renders generated by the useInView hook. We can leverage the built-in batching during the render cycle to eliminate the need for this reset useEffect that should have a really nice impact on performance. https://beta.reactjs.org/learn/you-might-not-need-an-effect#adjusting-some-state-when-a-prop-changes goes over an explanation of this, I honestly wasn't aware of the pattern until I read it but it seemed pertinent for here.

I'm not too familiar with vitest and couldn't determine a great way to test the number of re-renders in the current testing setup, but thought I'd open this to get your read on it before diving any deeper.

I see this referenced in #572 so not sure if this has some of the issues you were initially running into or not. I upgraded to the latest version and noticed we'd doubled the re-renders so thought I'd see if there was a workaround.

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated
react-intersection-observer	✅ Ready (Inspect)	Visit Preview	💬 Add your feedback	Feb 8, 2023 at 6:44PM (UTC)

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 6b905af:

Sandbox	Source
useInView	Configuration
InView render props	Configuration
InView plain children	Configuration

[thebuilder]

Hi @calinoracation - This looks really nice. I wasn't super happy with the solution that caused these extra renders, but it was a tradeoff for dealing with the ref issues in userland..
I'll have a deeper look at it tomorrow, see if there's any issues!

[calinoracation]

@thebuilder Thanks so much! Yeah we use the library extensively, so cutting down the re-renders is definitely a win if possible. I was even working to see if we could take it down even further and got pretty far with it down to a single useEffect with no dependencies. There's still one last issue involving the fallback support (the 2 snapshot tests are failing and I'm not as familiar with that area), but it's using pretty much the exact same pattern for the other useEffect. Would be pretty fantastic to land this if it works out for the re-render performance.

// globally somewhere
type ObserveOptions = Pick<IntersectionOptions, 'root' | 'rootMargin' | 'threshold' | 'trackVisibility' | 'delay' | 'fallbackInView' | 'skip'>;
function areObserveOptionsDifferent(previous: ObserveOptions, current: ObserveOptions) {
  return (
    previous.root !== current.root ||
    previous.rootMargin !== current.rootMargin ||
    previous.trackVisibility !== current.trackVisibility ||
    previous.delay !== current.delay ||
    previous.fallbackInView !== current.fallbackInView ||
    previous.skip !== current.skip ||
    ((Array.isArray(previous.threshold) ? previous.threshold.toString() : previous.threshold) !== (Array.isArray(current.threshold) ? current.threshold.toString() : current.threshold))
  );
}

  // inside useInView
  const previousRef = React.useRef<Element | null>(null);
  const unobserveRef = React.useRef<() => void>();
  const previousObserveOptions = React.useRef<ObserveOptions>({});
  const currentObserveOptions = {
    delay,
    fallbackInView,
    root,
    rootMargin,
    skip,
    threshold,
    trackVisibility,
  };

  if (typeof window !== 'undefined' && (previousRef.current !== ref || areObserveOptionsDifferent(previousObserveOptions.current, currentObserveOptions))) {
    unobserveRef.current?.();
    unobserveRef.current = undefined;
    previousRef.current = ref;
    previousObserveOptions.current = currentObserveOptions;

    if (!skip && ref) {
      unobserveRef.current = observe(ref, (inView, entry) => {
        setState({
          inView,
          entry,
        });

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

  React.useEffect(
    () => {
      return () => {
        if (unobserveRef.current) {
          unobserveRef.current();
          unobserveRef.current = undefined;
        }
      };
    },
    [],
  );

[thebuilder]

Don't think the window check is needed, since you wouldn't have the entryTarget on the server.

[thebuilder]

You could use the optionsToId method, to create a unique id for the options - That should work for detecting changes.

[thebuilder]

I think this PR makes sense for getting rid of the useEffect reset - Just one minor comment.

If you are up for it, it would be cool with some extra eyes trying out performance improvements on the main useEffect. 🙏 But maybe do that with another PR.

[calinoracation]

@thebuilder Yeah that totally makes sense on the comment one for a new PR and more 👁️ 's. I updated this PR with the removal of the unnecessary typeof window check.

[thebuilder]

🎉 This PR is included in version 9.4.2 🎉

The release is available on:

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

Your semantic-release bot 📦🚀