fix(router): don't write an existing URL (#4392)

* fix(router): don't write an existing URL

Every write puts a history entry, we never want to write the same as what's already written to the URL.

The history router didn't have tests yet, so I added just one for this case, as not to lose time writing tests for existing cases.

Demonstration of the bug being fixed in user land: https://codesandbox.io/s/vigilant-mayer-u3lyd?file=/src/app.js:2097-2279 (note that this searchFunction is very similar to the default one in Shopify)

* remove type creation in favor of Required

* add more tests

* add test for external write scenario

* add inline snapshots to see the route values

Author: Haroenv

src/lib/routers/__tests__/history.test.ts

@@ -0,0 +1,141 @@
+import historyRouter from '../history';
+
+const wait = (ms = 0) => new Promise(res => setTimeout(res, ms));
+
+describe('life cycle', () => {
+  beforeEach(() => {
+    window.history.pushState(null, '-- divider --', 'http://localhost/');
+    jest.restoreAllMocks();
+  });
+
+  it('does not write the same url twice', async () => {
+    const pushState = jest.spyOn(window.history, 'pushState');
+    const router = historyRouter({
+      writeDelay: 0,
+    });
+
+    router.write({ some: 'state' });
+    await wait(0);
+
+    router.write({ some: 'state' });
+    await wait(0);
+
+    router.write({ some: 'state' });
+    await wait(0);
+
+    expect(pushState).toHaveBeenCalledTimes(1);
+    expect(pushState.mock.calls).toMatchInlineSnapshot(`
+      Array [
+        Array [
+          Object {
+            "some": "state",
+          },
+          "",
+          "http://localhost/?some=state",
+        ],
+      ]
+    `);
+  });
+
+  it('does not write if already externally updated to desired URL', async () => {
+    const pushState = jest.spyOn(window.history, 'pushState');
+    const router = historyRouter({
+      writeDelay: 0,
+    });
+
+    const fakeState = { identifier: 'fake state' };
+
+    router.write({ some: 'state one' });
+
+    // external update before timeout passes
+    window.history.pushState(
+      fakeState,
+      '',
+      'http://localhost/?some=state%20two'
+    );
+
+    // this write isn't needed anymore
+    router.write({ some: 'state two' });
+    await wait(0);
+
+    expect(pushState).toHaveBeenCalledTimes(1);
+    expect(pushState.mock.calls).toMatchInlineSnapshot(`
+      Array [
+        Array [
+          Object {
+            "identifier": "fake state",
+          },
+          "",
+          "http://localhost/?some=state%20two",
+        ],
+      ]
+    `);
+
+    // proves that InstantSearch' write did not happen
+    expect(history.state).toBe(fakeState);
+  });
+
+  it('does not write the same url title twice', async () => {
+    const title = jest.spyOn(window.document, 'title', 'set');
+    const pushState = jest.spyOn(window.history, 'pushState');
+
+    const router = historyRouter({
+      writeDelay: 0,
+      windowTitle: state => `My Site - ${state.some}`,
+    });
+
+    expect(title).toHaveBeenCalledTimes(1);
+    expect(window.document.title).toBe('My Site - undefined');
+
+    router.write({ some: 'state' });
+    await wait(0);
+
+    router.write({ some: 'state' });
+    await wait(0);
+
+    router.write({ some: 'state' });
+    await wait(0);
+
+    expect(pushState).toHaveBeenCalledTimes(1);
+    expect(pushState.mock.calls).toMatchInlineSnapshot(`
+      Array [
+        Array [
+          Object {
+            "some": "state",
+          },
+          "My Site - state",
+          "http://localhost/?some=state",
+        ],
+      ]
+    `);
+
+    expect(title).toHaveBeenCalledTimes(2);
+    expect(window.document.title).toBe('My Site - state');
+  });
+
+  it('writes after timeout is done', async () => {
+    const pushState = jest.spyOn(window.history, 'pushState');
+
+    const router = historyRouter({
+      writeDelay: 0,
+    });
+
+    router.write({ some: 'state' });
+    router.write({ some: 'second' });
+    router.write({ some: 'third' });
+    await wait(0);
+
+    expect(pushState).toHaveBeenCalledTimes(1);
+    expect(pushState.mock.calls).toMatchInlineSnapshot(`
+      Array [
+        Array [
+          Object {
+            "some": "third",
+          },
+          "",
+          "http://localhost/?some=third",
+        ],
+      ]
+    `);
+  });
+});

src/lib/routers/history.ts

@@ -19,11 +19,11 @@ type ParseURL = ({
   location: Location;
 }) => RouteState;
 
-type BrowserHistoryProps = {
+type BrowserHistoryArgs = {
   windowTitle?: (routeState: RouteState) => string;
-  writeDelay: number;
-  createURL: CreateURL;
-  parseURL: ParseURL;
+  writeDelay?: number;
+  createURL?: CreateURL;
+  parseURL?: ParseURL;
 };
 
 const defaultCreateURL: CreateURL = ({ qsModule, routeState, location }) => {
@@ -63,25 +63,25 @@ class BrowserHistory implements Router {
   /**
    * Transforms a UI state into a title for the page.
    */
-  private readonly windowTitle?: BrowserHistoryProps['windowTitle'];
+  private readonly windowTitle?: BrowserHistoryArgs['windowTitle'];
   /**
    * Time in milliseconds before performing a write in the history.
    * It prevents from adding too many entries in the history and
    * makes the back button more usable.
    *
    * @default 400
    */
-  private readonly writeDelay: BrowserHistoryProps['writeDelay'];
+  private readonly writeDelay: Required<BrowserHistoryArgs>['writeDelay'];
   /**
    * Creates a full URL based on the route state.
    * The storage adaptor maps all syncable keys to the query string of the URL.
    */
-  private readonly _createURL: BrowserHistoryProps['createURL'];
+  private readonly _createURL: Required<BrowserHistoryArgs>['createURL'];
   /**
    * Parses the URL into a route state.
    * It should be symetrical to `createURL`.
    */
-  private readonly parseURL: BrowserHistoryProps['parseURL'];
+  private readonly parseURL: Required<BrowserHistoryArgs>['parseURL'];
 
   private writeTimer?: number;
   private _onPopState?(event: PopStateEvent): void;
@@ -96,7 +96,7 @@ class BrowserHistory implements Router {
       writeDelay = 400,
       createURL = defaultCreateURL,
       parseURL = defaultParseURL,
-    }: BrowserHistoryProps = {} as BrowserHistoryProps
+    }: BrowserHistoryArgs = {} as BrowserHistoryArgs
   ) {
     this.windowTitle = windowTitle;
     this.writeTimer = undefined;
@@ -128,9 +128,11 @@ class BrowserHistory implements Router {
     }
 
     this.writeTimer = window.setTimeout(() => {
-      setWindowTitle(title);
+      if (window.location.href !== url) {
+        setWindowTitle(title);
 
-      window.history.pushState(routeState, title || '', url);
+        window.history.pushState(routeState, title || '', url);
+      }
       this.writeTimer = undefined;
     }, this.writeDelay);
   }
@@ -192,6 +194,6 @@ class BrowserHistory implements Router {
   }
 }
 
-export default function(...args: BrowserHistoryProps[]): BrowserHistory {
-  return new BrowserHistory(...args);
+export default function(props?: BrowserHistoryArgs): BrowserHistory {
+  return new BrowserHistory(props);
 }