Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent enterAll and leaveAll from calling callbacks on observers with no observed nodes #54

Conversation

davidr64
Copy link
Contributor

@davidr64 davidr64 commented Feb 23, 2024

This PR addresses an issue encountered in React where callbacks were being invoked on observers that had unobserved all nodes. React frequently registers and unregisters callbacks during its life cycle, which often resulted in observers being left with no observed nodes.

To resolve this, the PR modifies the behaviour of the enterAll and leaveAll functions to skip calling callbacks on observers that have unobserved all nodes. This ensures that observers only receive callbacks when they have actively observed nodes, this prevents the callback functions from being called with an empty array, which should be impossible in non test scenarios.

Example code that would cause the existence of empty observers. If a child component with this kind of useEffect is being added and removed from the dom during a test it would create a series of IntersectionObservers that are not subscribed to a node.

useEffect(() => {
    const observer = new IntersectionObserver(handleObserver , { threshold: 1 });
    ref.current && observer.observe(ref.current);

    return () => {
      ref.current && observer.unobserve(ref.current);
    };
  }, [handleObserver, ref.current]);

Changes:
It seemed most efficient to update the MockedIntersectionObserver to simply return if triggerNodes was called with an empty array. EnterAll and LeaveAll both call this method with a mapped array of the observers nodes.

Notes:
If for some reason we need to support calling IntersectionObservers that are not currently observing any nodes we could add an argument to enterAll and leaveAll methods to indicate if empty observers should be called. However, I think it makes sense to not call observers that aren't observing anything.

@davidr64
Copy link
Contributor Author

davidr64 commented Feb 26, 2024

For more specificity on why I am making this change. I am using another library that defines a hook useInView

export function useInView<T extends Element>() {
  const ref = useRef<T>(null);
  const [isInView, setIsInView] = useState(false);

  if (window.IntersectionObserver) {
    const observer = new IntersectionObserver(entries => {
      setIsInView(entries[0].isIntersecting);
    });

    useEffect(() => {
      ref.current && observer.observe(ref.current);

      return () => {
        if (!ref.current) return;
        observer.unobserve(ref.current);
      };
    });
  }

  return { ref, isInView };
}

It assumes that the intersection observer callback receives an array that always has at least one entry in it. The tests I am writing include child components that rely on this hook. All of the components should be able to be on the screen at the same time. However, right now when I call enterAll an exception is thrown when the callbacks try to access isIntersecting on undefined (entries[0] is undefined if there are no nodes in the observer). This prevents the rest of the observers from firing.

@trurl-master
Copy link
Owner

Hey! Thanks for reminding me, i forgot, i'll take a look today :)

@trurl-master trurl-master merged commit 9a36419 into trurl-master:master Feb 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants