24

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.

Edit React setState from event listener

Improve this question
6
  • 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 :) Commented Jul 18, 2019 at 7:13
  • @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 Commented Jul 18, 2019 at 7:20
  • 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. Commented Jul 18, 2019 at 7:24
  • @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. Commented Jul 18, 2019 at 7:28
  • yep, so you should probably memoize handle scroll. or better yet, not drop class syntax since you already have a working solution. Commented Jul 18, 2019 at 7:31

5 Answers 5

Reset to default
25

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

Improve this answer
Sign up to request clarification or add additional context in comments.

4 Comments

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?
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.
And there is no memory leak: every time goingUp is changed it will call removeEventListener
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
9

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';
}
Improve this answer

2 Comments

I agree, this approach looks very clean and readable. Thank you, great insight.
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?
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.

Improve this answer

2 Comments

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.
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.
1

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.

Edit toggle on scroll

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

Improve this answer

Comments

-1 
const objDiv = document.getElementById('chat_body');
objDiv.scrollTop = objDiv.scrollHeight;
Improve this answer

1 Comment

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?

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.