_app.js > Site component fixing exhaustive-deps leads to massive memo leak

[sebastiangraz]

Love the repo, very helpful in getting started with Nextjs+Sanity. The other night I found that my linter recommended to supplying the useEffect dependencies inside the Site component. With the goal of being able to deploy easily without any errors I added togglePageTransition as a dep and the site started looping all my console.logs into the tens of thousands, depending on how far I scrolled. So definitely seems like there is something going on with the scroll restoration.

Either way its not a huge deal as I can just comment out the linter error and it'll deploy fine, but just wanted to let you guys know if you weren't aware. I do wonder though if there is a cleaner way to get the desired scroll restoration without having to comment out erroneous parts of the code.

Cheers

const Site = ({ Component, pageProps, router }) => {
  const togglePageTransition = useTogglePageTransition();
  const { isPageTransition } = useSiteContext();
  // Handle scroll position on history change
  useScrollRestoration(router, pageTransitionSpeed);

  // Trigger our loading class
  useEffect(() => {
    if (isBrowser) {
      document.documentElement.classList.toggle("is-loading", isPageTransition);
    }
  }, [isPageTransition]);

  // Setup page transition loading states
  useEffect(() => {
    Router.events.on("routeChangeStart", (_, { shallow }) => {
      // Bail if we're just changing URL parameters
      if (shallow) return;

      // Otherwise, start loading
      togglePageTransition(true);
    });

    Router.events.on("routeChangeComplete", () => {
      setTimeout(() => togglePageTransition(false), pageTransitionSpeed);
    });

    Router.events.on("routeChangeError", () => {
      togglePageTransition(false);
    });
    // dont fix exhaustive-deps for memo leak lol <<<<<<<<<<<<<<<<<<<<<<<<<<
    // eslint-disable-next-line react-hooks/exhaustive-deps
  }, []);
 ...
};

[ndimatteo]

Hey there @sebastiangraz thanks for the kind words, glad you're digging hull!

As for the eslint warning/issue you outlined:

I'm not sure exactly why this useEffect would cause your logs to loop simply by adding the context hook for togglePageTransition. That's totally separate from the useScrollRestoration hook.

I agree that the scroll restoration stuff is exhaustively complicated to do something inherently simple and obvious. Unfortunately this seems to be a side effect of Next.js for a lot of people, and this was the only solution that seemed to cover 99.99% of edge cases.

In any case, for some context: the useEffect you outlined above doesn't have any dependencies because we only want it to run once on the client to setup our router events. I know eslint can get weird about this, but I think telling it that this is "ok" is a fine work-around for now.

FWIW though: We're working on refactoring the entire _app.js to be a bit cleaner and move some of these client-side effects to their own, self-contained wrapper components ✨

This should make this a non-issue once we roll that out, so stay tuned! 🤘