Clear inView status if ref is removed

[thebuilder]

This fixes #303 by clearing the inView status when the ref changes.

[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 38107b5:

Sandbox	Source
react-intersection-observer	Configuration
react-intersection-observer	Issue #303

[coveralls]

Pull Request Test Coverage Report for Build 1727

• 6 of 6 (100.0%) changed or added relevant lines in 2 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage decreased (-0.5%) to 98.446%

---

Totals
Change from base Build 1719:	-0.5%
Covered Lines:	107
Relevant Lines:	107

---

💛 - Coveralls

[sebastienbarre]

@thebuilder Hi. I don't think this was correctly back-ported to the hook. Try as I might, this does not behave properly with useInView() for me -- inView is not reset property to false when the ref node is changed.

Looking at the code:

useEffect(() => {
    if (!unobserve.current && state.entry && !triggerOnce && !skip) {
      // If we don't have a ref, then reset the state (unless the hook is set to only `triggerOnce` or `skip`)
      // This ensures we correctly reflect the current state - If you aren't observing anything, then nothing is inView
      setState({
        inView: !!initialInView,
      });
    }
  });

I placed console.log() all over this, and useEffect() is never executed when unobserve.current is undefined for reasons I don't quite understand, thus !unobserve.current is always false thus setState() is never executed again when the ref is changed to null before it being changed to a new node.

My gut feeling is that the observed component is unmounted, thus triggering a call to setRef(null), then remounted with the new node, and both these calls to setRef() are collapsed before useEffect() is triggered, therefore unobserve.current is always defined, even though it is no longer the same node. The correct way might be to track the actual change of one node to another (by value)?

[sebastienbarre]

My use case could be simplified to the usual todo list example:

• I have a todo list viewer, which creates todo items, and I want to observe the last one in the list so that when I scroll to that last one, I fetch more todos.

However, picture a situation where I have a list of todos for each days of the week, and the user can select the day of the week.

• When they select a different day, the partial list of todo objects for that day is fetched and passed to the same list viewer, which creates the todo items to render; the viewer then observes the last item so that more todos can be fetched when the user scrolls to the last one.
• In other words the switch from selecting one day to another, say from Monday to Tuesday, results in the last todo item for Monday being unmounted, before the list viewer renders the items for Tuesday. The list viewer uses the same ref setter from your hook to observe the last item for Tuesday, but the visibility of the last item for Tuesday needs to be set back to false, because it was never seen before -- the last item for Monday is the one that had been last seen if the user scrolled own, not the last item for Tuesday.
• Unfortunately right now, this behavior does not work. If I scrolled down to the last item for Monday its visibility changed to true as expected and I fetched the remaining todos for that day. If I now select Tuesday, that last item for Monday is unmounted, and the last item for Tuesday shows up at the same scroll position... but its visibility is true right away. It should be reset to false, then set to true back by the Intersection Observer automatically, so that the change from false to true triggers a state change, which I can be notified about to fetch more todos for Tuesday right away.
• As mentioned above, I believe both calls to setRef() might be collapsed by React during the render phase, before useEffect() is triggered, at which point just testing for unobserve.current is not enough, because it is always defined: it's the node that changed.

[sebastienbarre]

OK so it seems like a known issue with refs and useEffect(), but I'm a bit confused because you had a good conversation about this very problem with Dan Abramov himself in 2019 which you used in a very instructive blog post: Ref objects inside useEffect Hooks | by Daniel Schmidt | Medium

He suggested using useCallback... and you are... but the issue I believe is still with the code in your useEffect(), which should be called from inside your useCallback(), not from outside, shouldn't it? Am I missing something? Otherwise setState() will never be called when the ref is changed to another node, since it's always defined either way, and useEffect() is not called in between the two calls to setRef(null) and setRef(new node).

Thanks

[thebuilder]

Hey, thanks for the write up - The intention is that useEffect is only called after the node is update. So, if changing ref it will be called with null and then the new ref, without triggering the useEffect in between. Thus you won't see the inView state switch to false.

If there is a bug, then it could be that it doesn't correctly trigger the initial Intersection state on the new node, that would replace the old one. But would need to validate if that's actually the case - I haven't heard of it before.

[sebastienbarre]

@thebuilder thanks for the quick reply.

First, let me acknowledge that you have the most popular solution for using the Intersection Observer API with React, so if nobody complained, then my problem must be very niche for sure.

I guess it might come down to different assumptions. Mine is that if the node is changed, then it is no longer the same node and since nothing is known about it then inView should always be reset to initialInView before its value can be assessed again by the intersection API for that new node. This is the same situation, to me, as the original situation, the one you would be in the first time you use the hook on a node -- since initialInView is false by default, the visibility of that original node has to be set later on by the Intersection observer. In other words, switching the ref to null before it is switched to a new node should be like going back to the original state of the hook--like starting from scratch.

It's possible your assumption is that if the node is changed and both the previous node and the new node are visible on screen, then inView should stay true and never switched back to false in between. That's a perfectly fine assumption for sure, and that seems to be working great for most people. This implies I'll have to write my own hook, as this prevents me from knowing that the new node became visible. And that's all good, knowing I can start from the great work you did already.

Going back to my TodoList component, the one that notifies me when the last TodoItem is in view.

• If I give it an array of 10 todo objects and it renders them as 10 TodoItem while using the ref on the last one, and if ALL of them are visible on screen (to simplify), then I expect inView to switch from false (initialInView) to true, because that last item just sprung out of existence (it was literally just mounted), and it is visible right away. And that is happening for sure.
• If I then give that same TodoList component a different array of 10 todo objects, and it also renders them all on screen, and also uses that same ref on the last one, then I will never know that this new last item, which is different from the previous last item, is now in view, because inView was never switched back to false -- it remained true. And yet, these are two totally different DOM nodes and two totally different todo items. The new item in the last position, just like the previous item in the last position, literally appeared from nothing (since they were both just mounted), so their initial visibility should be, to me, false.

In the context of infinite scrolling, say our browser window right now can only show 10 todos.

• In the first case above, I fetched 10 todos from a remote database, and that's all there was to fetch. inView becomes true right away, I can react to it, and in this case I will do nothing because I know I have fetched everything for the user.
• Subsequently, I fetched 10 todos (say, for a different project the user switched to) and passed them to the same TodoList component, but there are 10 more to fetch online. What I would want to know, just like in the previous case, is when the 10th todo became visible, at which point I could have reacted... and fetch the next 10 automatically. But I can't do so, because the second set of 10 todos replaced the first set of 10 todos, and the 10th todo was visible in both case... therefore the hook is not notifying me that the 10th todo from the second set became visible.

Thanks

[thebuilder]

@sebastienbarre try inspecting the entry from the useInView hook. It will contain the target (your node). You should be able to leverage this information inside your TodoList, and reset the state locally. (Just to be sure, you are setting a unique key on the TodoItem?)

The problem with changing it to flow you expect, is that it will require a double renderpass, to correctly reflect the state updates. It just opens the hook up for lots of race conditions and headaches.

As an alternative, if the hook isn't working for you, use the observe() method directly in your own custom hook. This will give you full control over how you attach and remove the IntersectionObserver.

[sebastienbarre]

Thanks.

I guess what threw me off looking at the code is that the behavior I'm looking for is implemented in the <InView> component, as a result of this very PR, in handleNode:

if (!node && !this.props.triggerOnce && !this.props.skip) {
        // Reset the state if we get a new node, and we aren't ignoring updates
        this.setState({ inView: !!this.props.initialInView, entry: undefined });
      }

but it is not implemented in the hook, so they do not behave the same.

Unfortunately using <InView> with render props completely hangs and crashes my Chrome tab. I tried yesterday, switched to the hook, had this whole discussion with you here, and tried again tonight since the hook wasn't going to work -- still crashing. However, and that's the good news, it works when used as a "plain children", and it behaves the way I need, by resetting inView to false when the node changes.

Thanks for your time.

[thebuilder]

Unfortunately using <InView> with render props completely hangs and crashes my Chrome tab.

Make sure you correctly handle the ref callback, so it doesn't trigger infinite rerenders.
https://github.com/thebuilder/react-intersection-observer#how-can-i-assign-multiple-refs-to-a-component