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
152 changes: 152 additions & 0 deletions src/lib/__tests__/routing-test.ts
Original file line number Diff line number Diff line change
@@ -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;
FabienMotte marked this conversation as resolved.
Show resolved Hide resolved

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
FabienMotte marked this conversation as resolved.
Show resolved Hide resolved
await wait(1.5 * writeDelay);
FabienMotte marked this conversation as resolved.
Show resolved Hide resolved
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);
FabienMotte marked this conversation as resolved.
Show resolved Hide resolved

// 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);
});
});
});
18 changes: 17 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,13 @@ 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
*
* @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

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 +96,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 +200,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 +245,14 @@ export default function historyRouter<TRouteState = UiState>({
},
});
},
writeOnDispose,
}: Partial<BrowserHistoryArgs<TRouteState>> = {}): BrowserHistory<TRouteState> {
return new BrowserHistory({
createURL,
parseURL,
writeDelay,
windowTitle,
getLocation,
writeOnDispose,
});
}
8 changes: 6 additions & 2 deletions src/middlewares/createRouterMiddleware.ts
Original file line number Diff line number Diff line change
Expand Up @@ -62,13 +62,16 @@ export const createRouterMiddleware = <

const initialUiState = instantSearchInstance._initialUiState;

let unsubscribed = false;
FabienMotte marked this conversation as resolved.
Show resolved Hide resolved

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;
Expand All @@ -87,6 +90,7 @@ export const createRouterMiddleware = <
},

unsubscribe() {
unsubscribed = true;
router.dispose();
},
};
Expand Down