React setState hook from scroll event listener

Question

First of all, with a class component, this works fine and does not cause any issues.

However, in functional component with hooks, whenever I try to set state from my scroll event listener's function handleScroll, my state fails to get updated or app's performance gets affected drastically even though I am using debounce.

import React, { useState, useEffect } from "react";
import debounce from "debounce";

let prevScrollY = 0;

const App = () => {
  const [goingUp, setGoingUp] = useState(false);

  const handleScroll = () => {
    const currentScrollY = window.scrollY;
    if (prevScrollY < currentScrollY && goingUp) {
      debounce(() => {
        setGoingUp(false);
      }, 1000);
    }
    if (prevScrollY > currentScrollY && !goingUp) {
      debounce(() => {
        setGoingUp(true);
      }, 1000);
    }

    prevScrollY = currentScrollY;
    console.log(goingUp, currentScrollY);
  };

  useEffect(() => {
    window.addEventListener("scroll", handleScroll);
    return () => window.removeEventListener("scroll", handleScroll);
  }, []);

  return (
    <div>
      <div style={{ background: "orange", height: 100, margin: 10 }} />
      <div style={{ background: "orange", height: 100, margin: 10 }} />
      <div style={{ background: "orange", height: 100, margin: 10 }} />
      <div style={{ background: "orange", height: 100, margin: 10 }} />
      <div style={{ background: "orange", height: 100, margin: 10 }} />
      <div style={{ background: "orange", height: 100, margin: 10 }} />
      <div style={{ background: "orange", height: 100, margin: 10 }} />
      <div style={{ background: "orange", height: 100, margin: 10 }} />
    </div>
  );
};

export default App;

Tried to use useCallback hook in handleScroll function but it did not help much.

What am I doing wrong? How can I set state from handleScroll without a huge impact on performance?

I've created a sandbox with this issue.

Solution 1

In your code I see several issues:

1) [] in useEffect means it will not see any changes of state, like changes of goingUp. It will always see initial value of goingUp

2) debounce does not work so. It returns a new debounced function.

3) usually global variables is an anti-pattern, thought it works just in your case.

4) your scroll listener is not passive, as mentioned by @skyboyer.

import React, { useState, useEffect, useRef } from "react";

const App = () => {
  const prevScrollY = useRef(0);

  const [goingUp, setGoingUp] = useState(false);

  useEffect(() => {
    const handleScroll = () => {
      const currentScrollY = window.scrollY;
      if (prevScrollY.current < currentScrollY && goingUp) {
        setGoingUp(false);
      }
      if (prevScrollY.current > currentScrollY && !goingUp) {
        setGoingUp(true);
      }

      prevScrollY.current = currentScrollY;
      console.log(goingUp, currentScrollY);
    };

    window.addEventListener("scroll", handleScroll, { passive: true });

    return () => window.removeEventListener("scroll", handleScroll);
  }, [goingUp]);

  return (
    <div>
      <div style={{ background: "orange", height: 100, margin: 10 }} />
      <div style={{ background: "orange", height: 100, margin: 10 }} />
      <div style={{ background: "orange", height: 100, margin: 10 }} />
      <div style={{ background: "orange", height: 100, margin: 10 }} />
      <div style={{ background: "orange", height: 100, margin: 10 }} />
      <div style={{ background: "orange", height: 100, margin: 10 }} />
      <div style={{ background: "orange", height: 100, margin: 10 }} />
      <div style={{ background: "orange", height: 100, margin: 10 }} />
    </div>
  );
};

export default App;

https://codesandbox.io/s/react-setstate-from-event-listener-q7to8

Solution 2

In short, you need to add goingUp as the dependency of useEffect.

If you use [], you will only create/remove a listener with a function(handleScroll, which is created in the initial render). In other words, when re-render, the scroll event listener is still using the old handleScroll from the initial render.

  useEffect(() => {
    window.addEventListener("scroll", handleScroll);
    return () => window.removeEventListener("scroll", handleScroll);
  }, [goingUp]);

Using custom hooks

I recommend move the whole logic into a custom hooks, which can make your code more clear and easy to reuse. I use useRef to store the previous value.

export function useScrollDirection() {
  const prevScrollY = useRef(0)
  const [goingUp, setGoingUp] = useState(false);

  const handleScroll = () => {
    const currentScrollY = window.scrollY;
    if (prevScrollY.current < currentScrollY && goingUp) {
      setGoingUp(false);
    }
    if (prevScrollY.current > currentScrollY && !goingUp) {
      setGoingUp(true);
    }
    prevScrollY.current = currentScrollY;
  };

  useEffect(() => {
    window.addEventListener("scroll", handleScroll);
    return () => window.removeEventListener("scroll", handleScroll);
  }, [goingUp]); 
  return goingUp ? 'up' : 'down';
}

Solution 3

Your are re-creating handleScroll function on each render so it's refers to actual data(goingup) so you don't need useCallback here.

But besides you recreate handleScroll you are set as event listener only first instance of it - since you give useEffect(..., []) it runs only once on mount.

The one solution is to re-subscribe to scroll with up-to-date handleScroll. To do that you have to return cleanup function within useEffect(by now it returns one more subscribing) and remove [] to run it on each render:

useEffect(() => {
  window.addEventListener("scroll", handleScroll);
  return () => window.removeEventListener("scroll", handleScroll);
});

PS and you better use passive event listener for scroll.

Solution 4

Here is my solution for this case

const [pageUp, setPageUp] = useState(false)
const lastScroll = useRef(0)

const checkScrollTop = () => {
  const currentScroll = window.pageYOffset
  setPageUp(lastScroll.current > currentScroll)
  lastScroll.current = currentScroll
}

// lodash throttle function
const throttledFunc = throttle(checkScrollTop, 400, { leading: true })

useEffect(() => {
  window.addEventListener("scroll", throttledFunc, false)
  return () => {
    window.removeEventListener("scroll", throttledFunc)
  }
}, [])

Adding event listeners should be once when componentDidMount only. not in every changes causes by pageUp. So, we don't need to add pageUp to useEffect dependencies.

Hint: you can add throttledFunc to useEffect if it changes and you need to re-render the component

View more solutions

Comments

[John Ruddell]

you're still creating the handlescroll for every tick of your debounce. you should memoize it. your unmount is still calling addEventListener rather than removeEventListener. Both of those can cause issues :)

[zilijonas]

@JohnRuddell hey, thanks for spotting the mistake on removeEventListener. However, this causes memory leaks but is not linked to the performance issue I am trying to solve. It was just a typo and on the original project is with remove instead of add, but the issue persists

[John Ruddell]

no it totally is, because you can setup hundreds of listeners with a simple scroll. which means each callback is trying to fire hundres of times per debounce tick.

[zilijonas]

@JohnRuddell ok, but this would happen only if I would unmount and re-mount my component hundreds of times (and that is not the case here) because the current effect runs only on mount/unmount.

[John Ruddell]

yep, so you should probably memoize handle scroll. or better yet, not drop class syntax since you already have a working solution.

[zilijonas]

@JohnRuddell I've tried moving handleScroll outside the component with more properties. This way it does not get recreated every time. I believed that this results in the same way, doesn't it?

[zilijonas]

Removing dependencies array from this effect does not help and instead calls add listener on every render. And even when I move handleScroll outside the function to prevent the recreation of it the issue still persists. All I need to do is to somehow determine when a user is scrolling up or down.

[zilijonas]

Great advices, this is awesome, thank you! Just one question - now whenever goingUp changes, useEffect is called (isn't it?). This means that everytime it is called, event listener is added. Wouldn't this cause memory leaks?

[Yozi]

useEffect will be called whenever goingUp is changed, as long as user does not change direction of scrolling hundreds of times per second performance will be fine.

[Yozi]

And there is no memory leak: every time goingUp is changed it will call removeEventListener

[Yozi]

I don't think you should worry about useEffect performance yet. Code on codesandbox shows it is fast enough. If you still would like improve performance even more you could memoise listener by using useDispatch. overreacted.io/a-complete-guide-to-useeffect . But I think it is a bad trade between the simplicity of the code and its performance

[skyboyer]

and it should call addEventListener on each render. otherwise you need much more complex way to both make handler using actual data(not stale one) and making it debounced.

[zilijonas]

I agree, this approach looks very clean and readable. Thank you, great insight.

[Kevin Burton]

Maybe I don't understand hooks but what if I want to add another custom hook that also listens to the scroll event but returns different data such as 'top' or 'bottom' when the scroll reaches the top of bottom of the window. Would these two hooks interfere with each other?

[old greg]

Could you please annotate your code or add an explanation above the code sample to explain how it works, so that this answer is applicable generally and not just in OP's specific use-case?