Repository: streamich/react-use
Pull request #1700 (2020-12-11 16:06:10)

fix for useWindowScroll may lose window scroll change at mount #1699
# Description

useWindowScroll may lose window scroll change at mount (see bug #1699)

We have to update window scroll at mount, before subscription.
Window scroll may be changed between render and effect handler.
Also, we can use lazy initial value for state (to prevent scroll position recalculation at each render).
It is legal, see there: https://kentcdodds.com/blog/use-state-lazy-initialization-and-function-updates

## Type of change

- [x] Bug fix _(non-breaking change which fixes an issue)_

# Checklist
- [x] Read the [Contributing Guide](https://github.com/streamich/react-use/blob/master/CONTRIBUTING.md)
- [x] Perform a code self-review
- [x] Comment the code, particularly in hard-to-understand areas
- [x] Cover changes with tests
- [x] Ensure the test suite passes (`yarn test`)
- [x] Provide 100% tests coverage
- [x] Make sure code lints (`yarn lint`). Fix it with `yarn lint:fix` in case of failure.
- [x] Make sure types are fine (`yarn lint:types`).


eaglus: @streamich @joecritch  
Hello! Please review my pr, it will help a lot for our mobile application
eaglus: @streamich @joecritch @mathiassoeholm 
Hello! Please review my pr, now it is tested with all edge cases.

mathiassoeholm: > @streamich @joecritch @mathiassoeholm
> Hello! Please review my pr, now it is tested with all edge cases.

Looks good, but I'm not maintainer, so I can't approve it.
xobotyi: @eaglus rebase PR please so i can merge it.

Patch:
diff --git a/src/useWindowScroll.ts b/src/useWindowScroll.ts
index 0145ef4a..36ae8e18 100644
--- a/src/useWindowScroll.ts
+++ b/src/useWindowScroll.ts
@@ -1,6 +1,6 @@
 import { useEffect } from 'react';
-import { isBrowser, off, on } from './misc/util';
 
+import { isBrowser, off, on } from './misc/util';
 import useRafState from './useRafState';
 
 export interface State {
@@ -9,19 +9,30 @@ export interface State {
 }
 
 const useWindowScroll = (): State => {
-  const [state, setState] = useRafState<State>({
+  const [state, setState] = useRafState<State>(() => ({
     x: isBrowser ? window.pageXOffset : 0,
     y: isBrowser ? window.pageYOffset : 0,
-  });
+  }));
 
   useEffect(() => {
     const handler = () => {
-      setState({
-        x: window.pageXOffset,
-        y: window.pageYOffset,
+      setState((state) => {
+        const { pageXOffset, pageYOffset } = window;
+        //Check state for change, return same state if no change happened to prevent rerender
+        //(see useState/setState documentation). useState/setState is used internally in useRafState/setState.
+        return state.x !== pageXOffset || state.y !== pageYOffset
+          ? {
+              x: pageXOffset,
+              y: pageYOffset,
+            }
+          : state;
       });
     };
 
+    //We have to update window scroll at mount, before subscription.
+    //Window scroll may be changed between render and effect handler.
+    handler();
+
     on(window, 'scroll', handler, {
       capture: false,
       passive: true,


Test patch:
diff --git a/tests/useWindowScroll.test.tsx b/tests/useWindowScroll.test.tsx
new file mode 100644
index 00000000..0bece5ad
--- /dev/null
+++ b/tests/useWindowScroll.test.tsx
@@ -0,0 +1,136 @@
+import React, { useEffect } from 'react';
+import { render, act as reactAct } from '@testing-library/react';
+import { act, renderHook } from '@testing-library/react-hooks';
+import { replaceRaf } from 'raf-stub';
+
+import useWindowScroll from '../src/useWindowScroll';
+
+declare var requestAnimationFrame: {
+  reset: () => void;
+  step: (steps?: number, duration?: number) => void;
+};
+
+describe('useWindowScroll', () => {
+  beforeAll(() => {
+    replaceRaf();
+  });
+
+  afterEach(() => {
+    requestAnimationFrame.reset();
+  });
+
+  it('should be defined', () => {
+    expect(useWindowScroll).toBeDefined();
+  });
+
+  function getHook() {
+    return renderHook(() => {
+      return useWindowScroll();
+    });
+  }
+
+  function setWindowScroll(x: number, y: number) {
+    (window.pageXOffset as number) = x;
+    (window.pageYOffset as number) = y;
+  }
+
+  function triggerScroll(dimension: 'x' | 'y', value: number) {
+    if (dimension === 'x') {
+      (window.pageXOffset as number) = value;
+    } else if (dimension === 'y') {
+      (window.pageYOffset as number) = value;
+    }
+
+    window.dispatchEvent(new Event('scroll'));
+  }
+
+  it('should return window scroll value at mount time', () => {
+    setWindowScroll(1, 2);
+
+    const hook = getHook();
+
+    expect(hook.result.current).toEqual({
+      x: 1,
+      y: 2,
+    });
+  });
+
+  it('should re-render after X scroll change on closest RAF', () => {
+    setWindowScroll(1, 2);
+    const hook = getHook();
+
+    act(() => {
+      triggerScroll('x', 100);
+      expect(hook.result.current.x).toBe(1);
+
+      requestAnimationFrame.step();
+    });
+
+    expect(hook.result.current.x).toBe(100);
+
+    act(() => {
+      triggerScroll('x', 1000);
+      expect(hook.result.current.x).toBe(100);
+      requestAnimationFrame.step();
+    });
+
+    expect(hook.result.current.x).toBe(1000);
+  });
+
+  it('should re-render after Y scroll change on closest RAF', () => {
+    setWindowScroll(1, 2);
+    const hook = getHook();
+
+    act(() => {
+      triggerScroll('y', 200);
+      expect(hook.result.current.y).toBe(2);
+      requestAnimationFrame.step();
+    });
+
+    expect(hook.result.current.y).toBe(200);
+
+    act(() => {
+      triggerScroll('y', 300);
+      expect(hook.result.current.y).toBe(200);
+      requestAnimationFrame.step();
+    });
+
+    expect(hook.result.current.y).toBe(300);
+  });
+
+  it('should set window scroll in mount effect, just before subscription, to prevent losing scroll change between render and mount', () => {
+    const initialScroll = { x: 1, y: 2 };
+    const afterRenderScroll = { x: 2, y: 3 };
+    const result = {
+      x: 0, y: 0
+    };
+
+    setWindowScroll(initialScroll.x, initialScroll.y);
+
+    const TestComponent = () => {
+      useEffect(() => {
+        // Simulate window scroll changing between component render and useWindowScroll effect handler,
+        // before adding the event listener
+        setWindowScroll(afterRenderScroll.x, afterRenderScroll.y);
+      }, []);
+
+      const { x, y } = useWindowScroll();
+      result.x = x;
+      result.y = y;
+      return <div />;
+    };
+
+    const { rerender } = render(<TestComponent />);
+    rerender(<TestComponent />);
+
+    //result update is delayed by requestAnimationFrame
+    expect(result).toEqual(initialScroll);
+
+    reactAct(() => {
+      requestAnimationFrame.step();
+    });
+
+    //result is updated next requestAnimationFrame
+    expect(result).toEqual(afterRenderScroll);
+  });
+});