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.

Sign up for GitHub

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

Jump to bottom

fix(routing): fix history router based on history length #5004

Merged
merged 12 commits into from
Feb 7, 2022
Merged

fix(routing): fix history router based on history length #5004

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
fc1cb8e
fix(routing): fix history router based on history length
FabienMotte Feb 2, 2022
17938f9
chore(tests): add routing test
FabienMotte Feb 2, 2022
f839220
chore(tests): split tests to avoid flakiness
FabienMotte Feb 2, 2022
8cdda05
fix(routing): stay responsive on external updates (before dispose)
FabienMotte Feb 2, 2022
4ddd41f
chore(tests): assert replaceState behavior
FabienMotte Feb 2, 2022
d945d32
chore: run codesandbox
FabienMotte Feb 2, 2022
2192463
refactor(routing): update dispose flag logic
FabienMotte Feb 3, 2022
402d9e0
refactor(routing): remove unused line
FabienMotte Feb 3, 2022
e716db2
refactor(routing): apply code review suggestions
FabienMotte Feb 4, 2022
72b5b88
chore(tests): apply feedback from code review
FabienMotte Feb 4, 2022
e670f2c
Merge branch 'master' into fix/routing-history-length
FabienMotte Feb 4, 2022
de02dca
chore(tests): apply feedback
FabienMotte Feb 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 73 additions & 0 deletions src/lib/__tests__/routing/external-influence-test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
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;

describe('routing', () => {
FabienMotte marked this conversation as resolved.
Show resolved Hide resolved
beforeEach(() => {
window.history.replaceState({}, '', 'http://localhost/');
jest.clearAllMocks();
});

test('Something else updates the URL: IS.js stay responsive', async () => {
FabienMotte marked this conversation as resolved.
Show resolved Hide resolved
// --- Flow
// Initial: '/'
// Refine: '/?indexName[query]=Apple'
// External influence: '/about'
// Refine: '/about?indexName[query]=Samsung'

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 by refining
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);

// Navigate to a new page (like a router would do)
window.history.pushState({}, '', '/about');

// Check URL has been updated to new page
await wait(writeWait);
expect(window.location.pathname).toEqual('/about');
expect(window.location.search).toEqual('');
expect(pushState).toHaveBeenCalledTimes(2);

// Trigger an update by refining
search.renderState.indexName!.searchBox!.refine('Samsung');

// Check URL has been updated to previous page
await wait(writeWait);
expect(window.location.pathname).toEqual('/about');
expect(window.location.search).toEqual(
`?${encodeURI('indexName[query]=Samsung')}`
);
});
});
63 changes: 63 additions & 0 deletions src/lib/__tests__/routing/modal-test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
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;

describe('routing', () => {
FabienMotte marked this conversation as resolved.
Show resolved Hide resolved
beforeEach(() => {
window.history.replaceState({}, '', 'http://localhost/');
jest.clearAllMocks();
});

test('Modal use case: dispose is manually called', async () => {
FabienMotte marked this conversation as resolved.
Show resolved Hide resolved
// --- Flow
// Initial: '/'
// Refine: '/?indexName[query]=Apple'
// Dispose: '/'

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 by refining
search.renderState.indexName!.searchBox!.refine('Apple');

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

// Trigger a dispose (modal closed, for example)
search.dispose();

// Check URL has been cleaned
await wait(writeWait);
expect(window.location.pathname).toEqual('/');
expect(window.location.search).toEqual('');
expect(pushState).toHaveBeenCalledTimes(2);
});
});
93 changes: 93 additions & 0 deletions src/lib/__tests__/routing/spa-debounced-test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
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;

describe('routing', () => {
FabienMotte marked this conversation as resolved.
Show resolved Hide resolved
beforeEach(() => {
window.history.replaceState({}, '', 'http://localhost/');
jest.clearAllMocks();
});

test('SPA debounced (Single Page App) use case: URL should not be cleaned', async () => {
FabienMotte marked this conversation as resolved.
Show resolved Hide resolved
// --- Flow
// Initial: '/'
// Refine: '/?indexName[query]=Apple'
// Dispose: '/'
// Route change: '/about'
// Back: '/'
// Back: '/?indexName[query]=Apple'

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 by refining
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 updated to new page
await wait(writeWait);
expect(window.location.pathname).toEqual('/');
expect(window.location.search).toEqual('');
expect(pushState).toHaveBeenCalledTimes(2);

// Navigate to a new page (like a router would do)
window.history.pushState({}, '', '/about');
FabienMotte marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

question (non-blocking): to be fully clear, this is the behavior we want to document (third-party router did initiate a navigation but wrote later, so we can't "escape" an in-between write), correct?

Copy link
Contributor

Choose a reason for hiding this comment

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

In our testing, this behaviour doesn't actually happen, as route navigations happen immediately, no debounce.

We are testing it to be aware if it ever changes, but that doesn't mean we think this is 100% correct behaviour (ideally the in-between update doesn't happen either.

How could that be made more clear in the test?

Copy link
Member

Choose a reason for hiding this comment

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

The scenario as I understand it is:

  • User refines InstantSearch, route updates (/?indexName[query]=Apple)
  • User clicks navigation handled by 3rd party router, but router doesn't kick in right away because it's debounced (longer delay than ours)
  • InstantSearch is disposed, history length is the same, so it cleans (/)
  • Router finally writes (/about)

If yes I think this is clear, but we should probably document it because having this intermediary / route can be surprising.


// Check URL has been updated to new page
await wait(writeWait);
expect(window.location.pathname).toEqual('/about');
expect(window.location.search).toEqual('');
expect(pushState).toHaveBeenCalledTimes(3);

// Go back to previous page without the refinement
window.history.back();

// Check URL has been updated to previous page
await wait(writeWait);
expect(window.location.pathname).toEqual('/');
expect(window.location.search).toEqual('');

// Go back to previous page with the refinement
window.history.back();

// Check URL has been updated to previous page
await wait(writeWait);
expect(window.location.pathname).toEqual('/');
expect(window.location.search).toEqual(
`?${encodeURI('indexName[query]=Apple')}`
);
expect(pushState).toHaveBeenCalledTimes(3);
});
});
82 changes: 82 additions & 0 deletions src/lib/__tests__/routing/spa-replace-state-test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
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;

describe('routing', () => {
FabienMotte marked this conversation as resolved.
Show resolved Hide resolved
beforeEach(() => {
window.history.replaceState({}, '', 'http://localhost/');
jest.clearAllMocks();
});

// It behaves like this because we check the `window.history.length`
// to prevent `write` after `dispose`
test('SPA (Single Page App) `replaceState` use case: URL should not be cleaned', async () => {
FabienMotte marked this conversation as resolved.
Show resolved Hide resolved
// --- Flow
// Initial: '/'
// Refine: '/?indexName[query]=Apple'
// Route change (with replaceState): '/about'
// Debounced dispose: '/about'
FabienMotte marked this conversation as resolved.
Show resolved Hide resolved
// Back: '/about'

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 by refining
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();

// Navigate to a new page (like a router would do)
window.history.replaceState({}, '', '/about');
FabienMotte marked this conversation as resolved.
Show resolved Hide resolved

// Check URL has been updated to new page
// await wait(writeWait);
Copy link
Member

Choose a reason for hiding this comment

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

question: Why is this commented? It does affect the test output.

Copy link
Contributor

Choose a reason for hiding this comment

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

should be removed indeed, here we do not want to wait until the url is written

Suggested change
// await wait(writeWait);

Copy link
Member

Choose a reason for hiding this comment

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

Yup we do it in the next step.

Is this a behavior worth documenting? Should we say that our router doesn't work well with other routers that use replaceState?

expect(window.location.pathname).toEqual('/about');
expect(window.location.search).toEqual('');
expect(pushState).toHaveBeenCalledTimes(1);

await wait(writeWait);
expect(window.location.pathname).toEqual('/about');
expect(window.location.search).toEqual('');
expect(pushState).toHaveBeenCalledTimes(2);

// Go back to previous page
window.history.back();

// Check URL has been updated to previous page
await wait(writeWait);
expect(window.location.pathname).toEqual('/about');
expect(window.location.search).toEqual('');
});
});
76 changes: 76 additions & 0 deletions src/lib/__tests__/routing/spa-test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
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;

describe('routing', () => {
FabienMotte marked this conversation as resolved.
Show resolved Hide resolved
beforeEach(() => {
window.history.replaceState({}, '', 'http://localhost/');
jest.clearAllMocks();
});

test('SPA (Single Page App) use case: URL should not be cleaned', async () => {
FabienMotte marked this conversation as resolved.
Show resolved Hide resolved
// --- Flow
// Initial: '/'
// Refine: '/?indexName[query]=Apple'
// Route change: '/about'
// Back: '/?indexName[query]=Apple'

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 by refining
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();

// Navigate to a new page (like a router would do)
window.history.pushState({}, '', '/about');
FabienMotte marked this conversation as resolved.
Show resolved Hide resolved

// Check URL has been updated to new page
await wait(writeWait);
expect(window.location.pathname).toEqual('/about');
expect(window.location.search).toEqual('');
expect(pushState).toHaveBeenCalledTimes(2);

// Go back to previous page
window.history.back();

// Check URL has been updated to previous page
await wait(writeWait);
expect(window.location.pathname).toEqual('/');
expect(window.location.search).toEqual(
`?${encodeURI('indexName[query]=Apple')}`
);
});
});
Loading