Hook triggers unnecessary renderings (fix provided)

[sapkra]

Every time the interval is executed or the window gets focused or blurred the entire app re-renders. I fixed the hook but don't have any time to create a PR. If you find any issues in my code, please leave a comment. With my code you can also remove 2 dependencies.

Feel free to use it:

import {
  useCallback,
  useEffect,
  useRef,
  useState,
} from 'react';

const getCurrentVersion = async (endpoint: string) => {
  const response = await fetch(endpoint);
  if (response.status > 400) {
    console.error(
      '[next-deploy-notifications] Could not find current app version. Did you setup the API route?',
    );
    return { version: 'unknown' };
  }
  const json = await response.json();
  return json;
};

type HookOptions = {
  interval?: number;
  endpoint?: string;
  debug?: boolean;
};

type HookValues = {
  hasNewDeploy: boolean;
  version: string;
};

type UseHasNewDeploy = (options?: HookOptions) => HookValues;

const useHasNewDeploy: UseHasNewDeploy = (options = {}) => {
  const debug = useCallback((message: string) => {
    if (options.debug) {
      console.log(...['[Deploy notifications] ', message]);
    }
  }, [options.debug]);

  const [hasNewDeploy, setHasNewDeploy] = useState<boolean>(false);
  const [currentVersion, setCurrentVersion] = useState<string>('unknown');
  const lastFetchedRef = useRef<number>();
  const intervalInstanceRef = useRef<NodeJS.Timer>();
  const windowFocused = useRef<boolean>(true);

  const interval = options.interval ?? 30_000;
  const loopInterval = interval < 3_000 ? interval : 3_000;
  const endpoint = options.endpoint ?? '/api/has-new-deploy';
  const isUnknown = currentVersion === 'unknown';

  const startInterval = useCallback(
    () => setInterval(async () => {
      debug('Looping...');

      const enoughTimeHasPassed =
      !lastFetchedRef.current || Date.now() >= lastFetchedRef.current + interval;

      if (enoughTimeHasPassed && !isUnknown) {
        debug('Fetching version');
        const { version } = await getCurrentVersion(endpoint);
        debug(`Version ${version}`);

        if (currentVersion !== version) {
          debug('Found new deploy');
          setHasNewDeploy(true);
          setCurrentVersion(version);
        }

        lastFetchedRef.current = Date.now();
      }
    }, loopInterval),
    [currentVersion, debug, endpoint, interval, isUnknown, loopInterval],
  );

  useEffect(() => {
    if (!hasNewDeploy) return;
    clearInterval(intervalInstanceRef.current);
  }, [hasNewDeploy]);

  useEffect(() => {
    if (!hasNewDeploy && windowFocused.current) {
      clearInterval(intervalInstanceRef.current);
      intervalInstanceRef.current = startInterval();
    }
    const onFocus = () => {
      debug('focus');
      windowFocused.current = true;
      clearInterval(intervalInstanceRef.current);
      intervalInstanceRef.current = startInterval();
    };
    const onBlur = () => {
      debug('blur');
      windowFocused.current = false;
      clearInterval(intervalInstanceRef.current);
    };
    debug('addEventListeners');
    window.addEventListener('focus', onFocus);
    window.addEventListener('blur', onBlur);

    return () => {
      debug('removeEventListeners');
      window.removeEventListener('focus', onFocus);
      window.removeEventListener('blur', onBlur);
    };
  }, []);

  useEffect(() => {
    const fetchInitialVersion = async () => {
      debug('Fetching initial version');
      const { version } = await getCurrentVersion(endpoint);
      if (version === 'unknown') {
        console.warn(
          '[next-deploy-notifications] Could not find current app version.',
        );
      } else {
        debug(`Version ${version}`);
        setCurrentVersion(version);
        lastFetchedRef.current = Date.now();
      }
    };

    fetchInitialVersion();
  }, [endpoint, debug]);

  return {
    hasNewDeploy,
    version: currentVersion,
  };
};

export { useHasNewDeploy };

[ryanto]

Awesome, thanks for sharing this.

I really like the idea of storing the lastFetched Date in a ref. There's no benefit of using React state here since the date is not used during render, so the ref idea is great! 👍

I think it makes sense to stick with use-window-focus though, even if it triggers re-renders on focus. In my experience it's easier to rely on libraries for this functionality, even if the behavior isn't perfect for the use case (ie triggers a re-render when we don't need it to).

Are there any problems with your app caused by focus triggered re-renders?

If you don't want your app component to re-render one suggestion would be to put the call to useHasNewDeploy() call in it's own component, so only that component will re-render.

Something like

App = () => {
  return (
    <div>
      {/* My app here */}
      <DeployNotifications />
    </div>
  );
}

let DeployNotifications = () => {
  let { hasNewDeploy } = useHasNewDeploy();
  // .. whatever UI for you need for displaying notifications
}

Again thanks for sharing this and I'll update this library to use a ref for last fetched.

[sapkra]

I'm glad that you like my code and thanks for the feedback. But I disagree with your opinion about using external libraries for extremely simple event handlers, explicitly if the library is triggering unexpected behaviors. It's always better to have no dependencies at all, also if you think about security.

The re-renders triggered that some form elements became empty for some reason or prefilled with the browser autocomplete again.

The tip with the wrapping component is a good idea and might work as a hacky workaround but it's still a misbehavior in the library. You can't expect that all users are noticing it and implement the hook in it that way. 😉