From 37f00f6f7ca1a7d9384dc78318ef24125c9030d0 Mon Sep 17 00:00:00 2001 From: Fabien Motte Date: Thu, 6 Jan 2022 16:36:05 +0100 Subject: [PATCH 1/6] fix(routing): new router option to whether write on dispose or not --- src/lib/__tests__/routing-test.ts | 152 ++++++++++++++++++++++++++++++ src/lib/routers/history.ts | 13 ++- 2 files changed, 164 insertions(+), 1 deletion(-) create mode 100644 src/lib/__tests__/routing-test.ts diff --git a/src/lib/__tests__/routing-test.ts b/src/lib/__tests__/routing-test.ts new file mode 100644 index 0000000000..a39e62b6ac --- /dev/null +++ b/src/lib/__tests__/routing-test.ts @@ -0,0 +1,152 @@ +import { createSearchClient } from '../../../test/mock/createSearchClient'; +import { wait } from '../../../test/utils/wait'; +import historyRouter from '../routers/history'; +import instantsearch from '../..'; +import { connectSearchBox } from '../../connectors'; + +describe('routing', () => { + afterEach(() => { + jest.clearAllMocks(); + }); + + describe('writeOnDispose=true', () => { + test('cleans URL on dispose', async () => { + const pushState = jest.spyOn(window.history, 'pushState'); + const writeDelay = 400; + + const search = instantsearch({ + indexName: 'indexName', + searchClient: createSearchClient(), + routing: { + router: historyRouter({ + writeDelay, + }), + }, + }); + + search.addWidgets([connectSearchBox(() => {})({})]); + + search.start(); + + // Wait for ${writeDelay} to pass then check URL has been initialized + await wait(1.5 * writeDelay); + expect(window.location.search).toEqual(''); + expect(pushState).toHaveBeenCalledTimes(0); + + // Trigger an update - push a change + search.renderState.indexName!.searchBox!.refine('Apple'); + + // Wait for ${writeDelay} to pass then check URL has been updated + await wait(1.5 * writeDelay); + expect(window.location.search).toEqual( + `?${encodeURI('indexName[query]=Apple')}` + ); + expect(pushState).toHaveBeenCalledTimes(1); + + // Trigger a dispose + search.dispose(); + + // Wait for ${writeDelay} to pass then check URL has been cleaned + await wait(1.5 * writeDelay); + expect(window.location.search).toEqual(''); + expect(pushState).toHaveBeenCalledTimes(2); + }); + + test('refine after dispose', async () => { + const pushState = jest.spyOn(window.history, 'pushState'); + const writeDelay = 400; + + const search = instantsearch({ + indexName: 'indexName', + searchClient: createSearchClient(), + routing: { + router: historyRouter({ + writeDelay, + }), + }, + }); + + search.addWidgets([connectSearchBox(() => {})({})]); + + search.start(); + + // Wait for ${writeDelay} to pass then check URL has been initialized + await wait(1.5 * writeDelay); + expect(window.location.search).toEqual(''); + expect(pushState).toHaveBeenCalledTimes(0); + + // Trigger an update - push a change + search.renderState.indexName!.searchBox!.refine('Apple'); + + // Wait for ${writeDelay} to pass then check URL has been updated + await wait(1.5 * writeDelay); + expect(window.location.search).toEqual( + `?${encodeURI('indexName[query]=Apple')}` + ); + expect(pushState).toHaveBeenCalledTimes(1); + + // Trigger a dispose + search.dispose(); + + // Wait for ${writeDelay} to pass then check URL has been cleaned + await wait(1.5 * writeDelay); + expect(window.location.search).toEqual(''); + expect(pushState).toHaveBeenCalledTimes(2); + + // Trigger an update - push a change + search.renderState.indexName!.searchBox!.refine('Apple'); + + // Wait for ${writeDelay} to pass then check URL has not been updated + await wait(1.5 * writeDelay); + expect(window.location.search).toEqual(''); + expect(pushState).toHaveBeenCalledTimes(2); + }); + }); + + describe('writeOnDispose=false', () => { + test('does not clean URL on dispose', async () => { + const pushState = jest.spyOn(window.history, 'pushState'); + const writeDelay = 400; + + const search = instantsearch({ + indexName: 'indexName', + searchClient: createSearchClient(), + routing: { + router: historyRouter({ + writeDelay, + writeOnDispose: false, + }), + }, + }); + + search.addWidgets([connectSearchBox(() => {})({})]); + + search.start(); + + // Wait for ${writeDelay} to pass then check URL has been initialized + await wait(1.5 * writeDelay); + expect(window.location.search).toEqual(''); + expect(pushState).toHaveBeenCalledTimes(0); + + // Trigger an update - push a change + search.renderState.indexName!.searchBox!.refine('Apple'); + + // Wait for ${writeDelay} to pass then check URL has been updated + await wait(1.5 * writeDelay); + expect(window.location.search).toEqual( + `?${encodeURI('indexName[query]=Apple')}` + ); + expect(pushState).toHaveBeenCalledTimes(1); + + // Trigger a dispose + search.dispose(); + + // Wait for ${writeDelay} to pass then check URL has been cleaned + await wait(1.5 * writeDelay); + expect(window.location.search).toEqual( + `?${encodeURI('indexName[query]=Apple')}` + ); + expect(pushState).toHaveBeenCalledTimes(1); + }); + }); +}); diff --git a/src/lib/routers/history.ts b/src/lib/routers/history.ts index e0768a8c14..2f3b419568 100644 --- a/src/lib/routers/history.ts +++ b/src/lib/routers/history.ts @@ -22,6 +22,7 @@ type BrowserHistoryArgs = { // so we should accept a subset of it that is easier to work with in any // environments. getLocation(): Location; + writeOnDispose?: boolean; }; const setWindowTitle = (title?: string): void => { @@ -78,6 +79,8 @@ class BrowserHistory implements Router { */ private shouldPushState: boolean = true; + private writeOnDispose: boolean = true; + /** * Initializes a new storage provider that syncs the search state to the URL * using web APIs (`window.location.pushState` and `onpopstate` event). @@ -88,6 +91,7 @@ class BrowserHistory implements Router { createURL, parseURL, getLocation, + writeOnDispose = true, }: BrowserHistoryArgs) { this.windowTitle = windowTitle; this.writeTimer = undefined; @@ -95,6 +99,7 @@ class BrowserHistory implements Router { this._createURL = createURL; this.parseURL = parseURL; this.getLocation = getLocation; + this.writeOnDispose = writeOnDispose; safelyRunOnBrowser(() => { const title = this.windowTitle && this.windowTitle(this.read()); @@ -190,7 +195,11 @@ class BrowserHistory implements Router { clearTimeout(this.writeTimer); } - this.write({} as TRouteState); + if (this.writeOnDispose) { + this.write({} as TRouteState); + } else { + this.shouldPushState = false; + } } } @@ -231,6 +240,7 @@ export default function historyRouter({ }, }); }, + writeOnDispose, }: Partial> = {}): BrowserHistory { return new BrowserHistory({ createURL, @@ -238,5 +248,6 @@ export default function historyRouter({ writeDelay, windowTitle, getLocation, + writeOnDispose, }); } From 45433de9e21801f8b0c6d0859cd4e6aacb666104 Mon Sep 17 00:00:00 2001 From: Fabien Motte Date: Thu, 6 Jan 2022 16:37:04 +0100 Subject: [PATCH 2/6] fix(routing): prevent write if middleware has been unsubscribed --- src/middlewares/createRouterMiddleware.ts | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/middlewares/createRouterMiddleware.ts b/src/middlewares/createRouterMiddleware.ts index 2747b991eb..00395586d2 100644 --- a/src/middlewares/createRouterMiddleware.ts +++ b/src/middlewares/createRouterMiddleware.ts @@ -62,13 +62,16 @@ export const createRouterMiddleware = < const initialUiState = instantSearchInstance._initialUiState; + let unsubscribed = false; + return { onStateChange({ uiState }) { const routeState = stateMapping.stateToRoute(uiState); if ( - lastRouteState === undefined || - !isEqual(lastRouteState, routeState) + (lastRouteState === undefined || + !isEqual(lastRouteState, routeState)) && + !unsubscribed ) { router.write(routeState); lastRouteState = routeState; @@ -87,6 +90,7 @@ export const createRouterMiddleware = < }, unsubscribe() { + unsubscribed = true; router.dispose(); }, }; From 854f0c9634295f853c9742211b5fcf927e60dfa7 Mon Sep 17 00:00:00 2001 From: Fabien Motte Date: Thu, 6 Jan 2022 17:08:01 +0100 Subject: [PATCH 3/6] chore(routing): add writeOnDispose comment --- src/lib/routers/history.ts | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/lib/routers/history.ts b/src/lib/routers/history.ts index 2f3b419568..5796c8c792 100644 --- a/src/lib/routers/history.ts +++ b/src/lib/routers/history.ts @@ -79,6 +79,11 @@ class BrowserHistory implements Router { */ private shouldPushState: boolean = true; + /** + * Indicates if `write` should be called on `dispose`. + * + * @default true + */ private writeOnDispose: boolean = true; /** From 8a9e1527f02f47842cc89b091c20cee626602a06 Mon Sep 17 00:00:00 2001 From: Fabien Motte Date: Mon, 24 Jan 2022 11:09:00 +0100 Subject: [PATCH 4/6] chore(tests): decrease delay and add a test --- src/lib/__tests__/routing-test.ts | 99 +++++++++++++++++++++---------- 1 file changed, 68 insertions(+), 31 deletions(-) diff --git a/src/lib/__tests__/routing-test.ts b/src/lib/__tests__/routing-test.ts index a39e62b6ac..6f0f391ba1 100644 --- a/src/lib/__tests__/routing-test.ts +++ b/src/lib/__tests__/routing-test.ts @@ -4,15 +4,21 @@ import historyRouter from '../routers/history'; import instantsearch from '../..'; import { connectSearchBox } from '../../connectors'; +const writeDelay = 10; +const writeWait = 1.5 * writeDelay; + +// This test may tear and not execute the tests in the right order. +// It seems to be related to a timing issue but we are not sure. + describe('routing', () => { - afterEach(() => { + beforeEach(() => { + window.history.pushState({}, '', 'http://localhost/'); jest.clearAllMocks(); }); describe('writeOnDispose=true', () => { test('cleans URL on dispose', async () => { const pushState = jest.spyOn(window.history, 'pushState'); - const writeDelay = 400; const search = instantsearch({ indexName: 'indexName', @@ -28,16 +34,16 @@ describe('routing', () => { search.start(); - // Wait for ${writeDelay} to pass then check URL has been initialized - await wait(1.5 * writeDelay); + // Check URL has been initialized + await wait(writeWait); expect(window.location.search).toEqual(''); expect(pushState).toHaveBeenCalledTimes(0); // Trigger an update - push a change search.renderState.indexName!.searchBox!.refine('Apple'); - // Wait for ${writeDelay} to pass then check URL has been updated - await wait(1.5 * writeDelay); + // Check URL has been updated + await wait(writeWait); expect(window.location.search).toEqual( `?${encodeURI('indexName[query]=Apple')}` ); @@ -46,15 +52,14 @@ describe('routing', () => { // Trigger a dispose search.dispose(); - // Wait for ${writeDelay} to pass then check URL has been cleaned - await wait(1.5 * writeDelay); + // Check URL has been cleaned + await wait(writeWait); expect(window.location.search).toEqual(''); expect(pushState).toHaveBeenCalledTimes(2); }); test('refine after dispose', async () => { const pushState = jest.spyOn(window.history, 'pushState'); - const writeDelay = 400; const search = instantsearch({ indexName: 'indexName', @@ -67,46 +72,78 @@ describe('routing', () => { }); search.addWidgets([connectSearchBox(() => {})({})]); - search.start(); - // Wait for ${writeDelay} to pass then check URL has been initialized - await wait(1.5 * writeDelay); - expect(window.location.search).toEqual(''); - expect(pushState).toHaveBeenCalledTimes(0); + // Trigger an update - push a change + search.renderState.indexName!.searchBox!.refine('Apple'); + + // Trigger a dispose + search.dispose(); // Trigger an update - push a change search.renderState.indexName!.searchBox!.refine('Apple'); - // Wait for ${writeDelay} to pass then check URL has been updated - await wait(1.5 * writeDelay); + // Check URL has not been updated + await wait(writeWait); + expect(window.location.search).toEqual(''); + expect(pushState).toHaveBeenCalledTimes(1); + }); + + test('URL is updated after starting instantsearch again', async () => { + const pushState = jest.spyOn(window.history, 'pushState'); + + const search = instantsearch({ + indexName: 'indexName', + searchClient: createSearchClient(), + routing: { + router: historyRouter({ + writeDelay, + }), + }, + }); + + search.addWidgets([connectSearchBox(() => {})({})]); + + search.start(); + + // Trigger an update - push a change + search.renderState.indexName!.searchBox!.refine('Query'); + + // Check URL has been updated + await wait(writeWait); expect(window.location.search).toEqual( - `?${encodeURI('indexName[query]=Apple')}` + `?${encodeURI('indexName[query]=Query')}` ); expect(pushState).toHaveBeenCalledTimes(1); // Trigger a dispose search.dispose(); - // Wait for ${writeDelay} to pass then check URL has been cleaned - await wait(1.5 * writeDelay); + // Check URL has been cleaned + await wait(writeWait); expect(window.location.search).toEqual(''); expect(pushState).toHaveBeenCalledTimes(2); + // Start again + search.addWidgets([connectSearchBox(() => {})({})]); + + search.start(); + // Trigger an update - push a change - search.renderState.indexName!.searchBox!.refine('Apple'); + search.renderState.indexName!.searchBox!.refine('Test'); - // Wait for ${writeDelay} to pass then check URL has not been updated - await wait(1.5 * writeDelay); - expect(window.location.search).toEqual(''); - expect(pushState).toHaveBeenCalledTimes(2); + // Check URL has been updated + await wait(writeWait); + expect(window.location.search).toEqual( + `?${encodeURI('indexName[query]=Test')}` + ); + expect(pushState).toHaveBeenCalledTimes(3); }); }); describe('writeOnDispose=false', () => { test('does not clean URL on dispose', async () => { const pushState = jest.spyOn(window.history, 'pushState'); - const writeDelay = 400; const search = instantsearch({ indexName: 'indexName', @@ -123,16 +160,16 @@ describe('routing', () => { search.start(); - // Wait for ${writeDelay} to pass then check URL has been initialized - await wait(1.5 * writeDelay); + // Check URL has been initialized + await wait(writeWait); expect(window.location.search).toEqual(''); expect(pushState).toHaveBeenCalledTimes(0); // Trigger an update - push a change search.renderState.indexName!.searchBox!.refine('Apple'); - // Wait for ${writeDelay} to pass then check URL has been updated - await wait(1.5 * writeDelay); + // Check URL has been updated + await wait(writeWait); expect(window.location.search).toEqual( `?${encodeURI('indexName[query]=Apple')}` ); @@ -141,8 +178,8 @@ describe('routing', () => { // Trigger a dispose search.dispose(); - // Wait for ${writeDelay} to pass then check URL has been cleaned - await wait(1.5 * writeDelay); + // Check URL has not been cleaned + await wait(writeWait); expect(window.location.search).toEqual( `?${encodeURI('indexName[query]=Apple')}` ); From b3beaadd4b5e6ef72585713156aed681abd66ac2 Mon Sep 17 00:00:00 2001 From: Fabien Motte Date: Mon, 24 Jan 2022 11:09:39 +0100 Subject: [PATCH 5/6] chore(routing): update writeOnDispose comment --- src/lib/routers/history.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/lib/routers/history.ts b/src/lib/routers/history.ts index 5796c8c792..aea003ba4d 100644 --- a/src/lib/routers/history.ts +++ b/src/lib/routers/history.ts @@ -81,6 +81,8 @@ class BrowserHistory implements Router { /** * Indicates if `write` should be called on `dispose`. + * When using other client-side routing utilities to navigate between pages + * (e.g., a front-end SPA routing library), set this option to `false`. * * @default true */ From 93889a2f16459089d3774bd4466183cebd51b51e Mon Sep 17 00:00:00 2001 From: Fabien Motte Date: Mon, 24 Jan 2022 11:10:11 +0100 Subject: [PATCH 6/6] fix(routing): reset flag on subscribe --- src/middlewares/createRouterMiddleware.ts | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/middlewares/createRouterMiddleware.ts b/src/middlewares/createRouterMiddleware.ts index 00395586d2..6fde39f18d 100644 --- a/src/middlewares/createRouterMiddleware.ts +++ b/src/middlewares/createRouterMiddleware.ts @@ -62,7 +62,7 @@ export const createRouterMiddleware = < const initialUiState = instantSearchInstance._initialUiState; - let unsubscribed = false; + let subscribed = false; return { onStateChange({ uiState }) { @@ -71,7 +71,7 @@ export const createRouterMiddleware = < if ( (lastRouteState === undefined || !isEqual(lastRouteState, routeState)) && - !unsubscribed + subscribed ) { router.write(routeState); lastRouteState = routeState; @@ -79,6 +79,8 @@ export const createRouterMiddleware = < }, subscribe() { + subscribed = true; + instantSearchInstance._initialUiState = { ...initialUiState, ...stateMapping.routeToState(router.read()), @@ -90,7 +92,8 @@ export const createRouterMiddleware = < }, unsubscribe() { - unsubscribed = true; + subscribed = false; + router.dispose(); }, };