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

InView always forces a React Tree Reconciliation #195

Closed
etripier opened this issue Mar 8, 2019 · 14 comments
Closed

InView always forces a React Tree Reconciliation #195

etripier opened this issue Mar 8, 2019 · 14 comments

Comments

@etripier
Copy link
Contributor

etripier commented Mar 8, 2019

The handleChange callback always calls setState regardless of whether or not the value for state.inView will change. This forces a React Tree Reconciliation for each observed component when the tree is mounted (IO calls the callback once for each observed component).

I think an optimization could be made to https://github.com/thebuilder/react-intersection-observer/blob/master/src/InView.tsx#L96:

handleChange = (inView: boolean, entry: IntersectionObserverEntry) => {
  if (inView !== this.state.inView || inView) {
    this.setState({ inView, entry })
  }
  if (this.props.onChange) {
    this.props.onChange(inView, entry)
  }
}

This would allow the child to receive a new value for entry when in view (where it counts), but would prevent the re-render/reconciliation that happens at the very beginning.

What do you think? Am I missing some context here?

@thebuilder
Copy link
Owner

thebuilder commented Mar 8, 2019

The IntersectionObserver only fires a change event when it actually crosses a threshold. So if you only have a single threshold value, it will only be triggered when the element enters or leaves view.

If you added multiple thresholds you'll want to to be notified whenever it crosses a threshold - so it needs to update the state with the current IntersectionObserverEntry, so it can be properly passed along to to the render prop function.

@etripier
Copy link
Contributor Author

etripier commented Mar 8, 2019

My observation is that the component fires a callback as soon as the observer is instantiated (see https://stackoverflow.com/questions/53214116/intersectionobserver-callback-firing-immediately-on-page-load). A component in the viewport will have two callbacks fired - one where inView is false and another where inView is true. Components outside of the viewport will fire only the first callback.

@thebuilder
Copy link
Owner

This is the intended behavior. Are you seeing any actual performance issues because of this? To ignore the first callback would require extra logic, and if I recall correctly, not all (older) browsers trigger it.

@etripier
Copy link
Contributor Author

etripier commented Mar 9, 2019

I think the problem I'm seeing is that the initial callback kicks off setState({ inView }), even if it would simply set state.inView === false to false. That causes a reconciliation which could be avoided.

@thebuilder
Copy link
Owner

Looking at what happens in the Storybook, it only triggers a single onChange event, when starting inside the view. Logging out when onChange gets called, it also seems like it's only being called once.
https://thebuilder.github.io/react-intersection-observer/?path=/story/intersection-observer--start-in-view

Even if inView === false on the initial callback, it still gets the entry, that should be set in the state.

@etripier
Copy link
Contributor Author

etripier commented Mar 9, 2019

To clarify what I mean, here's an example (I probably should have led with this):

() => {
  console.log('render');
  return (
    <>
      <div className="container">
        <div className="grid">
          {[...Array(40)].map((_, i) => (
            <InView key={i}>
              {({ inView, ref }) => {
                console.log(inView);
                return <div ref={ref} />;
              }}
            </InView>
          ))}
        </div>
        <style jsx>{`
          .container {
            padding: 0 16px 16px;
            width: 100%;
          }

          .grid {
            display: grid;
            grid-gap: 16px;
            grid-template-columns: repeat(auto-fill, minmax(150px, 1fr));
          }
        `}</style>
      </div>
    </>
  );
};

Rendering the above gives me the following:

Screen Shot 2019-03-09 at 6 46 39 AM

This is what I would expect and lines up with the built-in behavior for IntersectionObserver, like you mentioned.

What is unexpected, however, are the following stack traces:

The first observation:

Screen Shot 2019-03-09 at 6 56 37 AM

The second observation:

Screen Shot 2019-03-09 at 6 57 00 AM

In this case the extra work done by React to reconcile a state update of state.inView from false to false for the 22 components below the fold incurs a reasonably significant cost (relatively, at least). I don't need the update to entry for those components because they still haven't come in view, either.

@thebuilder
Copy link
Owner

One of the reasons you might want to read the entry, even if the component is not inside the viewport, is to let you know that it has triggered the callback. Maybe you need to trigger an animation, only when the element starts outside the viewport.

If you did try to render a heavy component, would using React.memo to stop an unnecessary update help?

I have no way of knowing how this library is actually consumed, so maybe it wouldn't be a problem to change the behaviour.

@thebuilder
Copy link
Owner

I went ahead and implemented this check - Let's see if it causes any actual issues. Properly not!

@etripier
Copy link
Contributor Author

Oh, great! Thank you for the fix!

@ChrisW-B
Copy link

This also seems to happen to the hook still, unless the update hasn't been pushed to npm yet!

@thebuilder
Copy link
Owner

The hook does an extra render when the ref is set. Is this what you're seeing? This is how hooks are designed.

@ChrisW-B
Copy link

Sorry for the delay! I'm really sorry if this is just me misunderstanding intersection observer, but it seems like if I set {threshold: [0,1]}, the component continually sets inView to true when the component is visible, though it only returns one when the component is out of view

@thebuilder
Copy link
Owner

That sounds like the intended behavior. It will trigger whenever it crosses the threshold, and when it doesn't match a threshold. You should look at the entry you also get back, to determine which threshold it just crossed

@ChrisW-B
Copy link

Thanks again, your most recent update fixed this for me! I was using [0,1] assuming I needed it since I wasn't getting an event when the element was no longer visible, but that was a misunderstanding on my part bc of the bug you fixed. Sorry about that!

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

No branches or pull requests

3 participants