Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(routing): new router option to prevent write on dispose #4989

Closed
wants to merge 10 commits into from
189 changes: 189 additions & 0 deletions src/lib/__tests__/routing-test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,189 @@
import { createSearchClient } from '../../../test/mock/createSearchClient';
import { wait } from '../../../test/utils/wait';
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', () => {
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 search = instantsearch({
indexName: 'indexName',
searchClient: createSearchClient(),
routing: {
router: historyRouter({
writeDelay,
}),
},
});

search.addWidgets([connectSearchBox(() => {})({})]);

search.start();

// 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');

// Check URL has been updated
await wait(writeWait);
expect(window.location.search).toEqual(
`?${encodeURI('indexName[query]=Apple')}`
);
expect(pushState).toHaveBeenCalledTimes(1);

// Trigger a dispose
search.dispose();

// 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 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('Apple');

// Trigger a dispose
search.dispose();

// Trigger an update - push a change
search.renderState.indexName!.searchBox!.refine('Apple');

// 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]=Query')}`
);
expect(pushState).toHaveBeenCalledTimes(1);

// Trigger a dispose
search.dispose();

// 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('Test');

// 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 search = instantsearch({
indexName: 'indexName',
searchClient: createSearchClient(),
routing: {
router: historyRouter({
writeDelay,
writeOnDispose: false,
}),
},
});

search.addWidgets([connectSearchBox(() => {})({})]);

search.start();

// 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');

// Check URL has been updated
await wait(writeWait);
expect(window.location.search).toEqual(
`?${encodeURI('indexName[query]=Apple')}`
);
expect(pushState).toHaveBeenCalledTimes(1);

// Trigger a dispose
search.dispose();

// Check URL has not been cleaned
await wait(writeWait);
expect(window.location.search).toEqual(
`?${encodeURI('indexName[query]=Apple')}`
);
expect(pushState).toHaveBeenCalledTimes(1);
});
});
});
20 changes: 19 additions & 1 deletion src/lib/routers/history.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ type BrowserHistoryArgs<TRouteState> = {
// 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 => {
Expand Down Expand Up @@ -78,6 +79,15 @@ class BrowserHistory<TRouteState> implements Router<TRouteState> {
*/
private shouldPushState: boolean = true;

/**
* Indicates if `write` should be called on `dispose`.
FabienMotte marked this conversation as resolved.
Show resolved Hide resolved
* 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
*/
private writeOnDispose: boolean = true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to find a better name, but can't think of anything

  • cleanup: true/false
  • cleanupOnDispose: true/false
  • singlepage: false/true (not really a fan)
  • externalRouter: false/true (not really a fan)

Copy link
Member

@sarahdayan sarahdayan Jan 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Haroenv What don't you like about it? What meaning should it convey that you think is missing or improperly expressed right now?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for me it seems an implementation detail, I'd rather have the option explain why you'd want to do that in the first place

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add a paragraph in the docs to explain this new option (and keep the name as is it)?


/**
* Initializes a new storage provider that syncs the search state to the URL
* using web APIs (`window.location.pushState` and `onpopstate` event).
Expand All @@ -88,13 +98,15 @@ class BrowserHistory<TRouteState> implements Router<TRouteState> {
createURL,
parseURL,
getLocation,
writeOnDispose = true,
}: BrowserHistoryArgs<TRouteState>) {
this.windowTitle = windowTitle;
this.writeTimer = undefined;
this.writeDelay = writeDelay;
this._createURL = createURL;
this.parseURL = parseURL;
this.getLocation = getLocation;
this.writeOnDispose = writeOnDispose;

safelyRunOnBrowser(() => {
const title = this.windowTitle && this.windowTitle(this.read());
Expand Down Expand Up @@ -190,7 +202,11 @@ class BrowserHistory<TRouteState> implements Router<TRouteState> {
clearTimeout(this.writeTimer);
}

this.write({} as TRouteState);
if (this.writeOnDispose) {
this.write({} as TRouteState);
} else {
this.shouldPushState = false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this prevent? I've ran the tests without this line and nothing breaks.

We likely need to test this behavior if this is actually needed.

}
}
}

Expand Down Expand Up @@ -231,12 +247,14 @@ export default function historyRouter<TRouteState = UiState>({
},
});
},
writeOnDispose,
}: Partial<BrowserHistoryArgs<TRouteState>> = {}): BrowserHistory<TRouteState> {
return new BrowserHistory({
createURL,
parseURL,
writeDelay,
windowTitle,
getLocation,
writeOnDispose,
});
}
11 changes: 9 additions & 2 deletions src/middlewares/createRouterMiddleware.ts
Original file line number Diff line number Diff line change
Expand Up @@ -62,20 +62,25 @@ export const createRouterMiddleware = <

const initialUiState = instantSearchInstance._initialUiState;

let subscribed = false;

return {
onStateChange({ uiState }) {
const routeState = stateMapping.stateToRoute(uiState);

if (
lastRouteState === undefined ||
!isEqual(lastRouteState, routeState)
(lastRouteState === undefined ||
!isEqual(lastRouteState, routeState)) &&
subscribed
) {
router.write(routeState);
lastRouteState = routeState;
}
},

subscribe() {
subscribed = true;

instantSearchInstance._initialUiState = {
...initialUiState,
...stateMapping.routeToState(router.read()),
Expand All @@ -87,6 +92,8 @@ export const createRouterMiddleware = <
},

unsubscribe() {
subscribed = false;

router.dispose();
},
};
Expand Down