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

fix(middleware): subscribe middleware before initializing main index #4849

Merged
merged 17 commits into from
Sep 2, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
8 changes: 4 additions & 4 deletions src/lib/InstantSearch.ts
Original file line number Diff line number Diff line change
Expand Up @@ -482,16 +482,16 @@ See ${createDocumentationLink({

this.mainHelper = mainHelper;

this.middleware.forEach(({ instance }) => {
instance.subscribe();
});

this.mainIndex.init({
Haroenv marked this conversation as resolved.
Show resolved Hide resolved
instantSearchInstance: this,
parent: null,
uiState: this._initialUiState,
});

this.middleware.forEach(({ instance }) => {
instance.subscribe();
});

mainHelper.search();

// Keep the previous reference for legacy purpose, some pattern use
Expand Down
4 changes: 2 additions & 2 deletions src/lib/__tests__/InstantSearch-test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -1540,8 +1540,8 @@ describe('use', () => {
.invocationCallOrder[0];
const middlewareSubscribeCallOrder =
middlewareSpy.subscribe.mock.invocationCallOrder[0];
// Checks that `mainIndex.init` was called before subscribing the middleware.
expect(widgetsInitCallOrder).toBeLessThan(middlewareSubscribeCallOrder);
// Checks that `mainIndex.init` was called after subscribing the middleware.
expect(widgetsInitCallOrder).toBeGreaterThan(middlewareSubscribeCallOrder);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is technically a breaking change, but I don't think it will break anything in real life.

Copy link
Member

Choose a reason for hiding this comment

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

We've got a customer report saying that their app broke. I think that's because of this commit, because instantSearchInstance.renderState[instantSearchInstance.indexName] is undefined in createInsightsMiddleware.

Any middleware that relies on instantSearchInstance might break now, because there's no certainty that the instance has started anymore.

Could we rather have a setInitialUiState method which updates initialUiState without these PR changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We actually fixed that regression at #4897

I tried to fix the issue without introducing new API, but I let you and the team decide whether to revert #4849 and #4897 and introduce setInitialUiState.


await wait(0);

Expand Down
62 changes: 38 additions & 24 deletions src/middlewares/__tests__/createInsightsMiddleware.ts
Original file line number Diff line number Diff line change
@@ -1,28 +1,34 @@
import algoliasearch from 'algoliasearch';
import algoliasearchHelper from 'algoliasearch-helper';
import instantsearch from '../../index.es';
import { createInsightsMiddleware } from '../';
import { createInstantSearch } from '../../../test/mock/createInstantSearch';
import {
createInsights,
createInsightsUmdVersion,
} from '../../../test/mock/createInsightsClient';
import { createSearchClient } from '../../../test/mock/createSearchClient';
import { warning } from '../../lib/utils';
import type { SearchClient } from '../../types';

describe('insights', () => {
const createTestEnvironment = () => {
const { analytics, insightsClient } = createInsights();
const instantSearchInstance = createInstantSearch({
client: algoliasearch('myAppId', 'myApiKey'),
const indexName = 'my-index';
const instantSearchInstance = instantsearch({
searchClient: createSearchClient({
// @ts-expect-error only available in search client v4
transporter: {
headers: {
'x-algolia-application-id': 'myAppId',
'x-algolia-api-key': 'myApiKey',
},
},
}),
indexName,
});
const helper = algoliasearchHelper({} as SearchClient, '');
const getUserToken = () => {
return (helper.state as any).userToken;
};
// @ts-expect-error
instantSearchInstance.mainIndex = {
getHelper: () => helper,
};
instantSearchInstance.start();

const helper = instantSearchInstance.mainIndex.getHelper();

// @ts-expect-error `userToken` exists in only search client v4
const getUserToken = () => helper.state.userToken;

return {
analytics,
Expand All @@ -37,17 +43,25 @@ describe('insights', () => {
const { analytics, insightsClient, libraryLoadedAndProcessQueue } =
createInsightsUmdVersion();

const instantSearchInstance = createInstantSearch({
client: algoliasearch('myAppId', 'myApiKey'),
const indexName = 'my-index';
const instantSearchInstance = instantsearch({
searchClient: createSearchClient({
// @ts-expect-error only available in search client v4
transporter: {
headers: {
'x-algolia-application-id': 'my-app-id',
'x-algolia-api-key': 'my-api-key',
},
},
}),
indexName,
});
const helper = algoliasearchHelper({} as SearchClient, '');
const getUserToken = () => {
return (helper.state as any).userToken;
};
// @ts-expect-error
instantSearchInstance.mainIndex = {
getHelper: () => helper,
};
instantSearchInstance.start();

const helper = instantSearchInstance.mainIndex.getHelper();

const getUserToken = () => (helper.state as any).userToken;

return {
analytics,
insightsClient,
Expand Down
100 changes: 100 additions & 0 deletions src/middlewares/__tests__/createRouterMiddleware.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,100 @@
import { createSearchClient } from '../../../test/mock/createSearchClient';
import { wait } from '../../../test/utils/wait';
import instantsearch from '../../index.es';
import { searchBox } from '../../widgets';

describe('router', () => {
it('sets initial ui state after reading URL', async () => {
const searchClient = createSearchClient();
const search = instantsearch({
indexName: 'my-index',
searchClient,
routing: {
router: {
onUpdate: () => {},
read: () => {
return {
'my-index': {
query: 'iPhone',
},
};
},
write: () => {},
createURL: () => '',
dispose: () => {},
},
},
});
search.addWidgets([
searchBox({ container: document.createElement('div') }),
]);
search.start();

await wait(0);

expect(search.getUiState()).toMatchInlineSnapshot(`
{
"my-index": {
"query": "iPhone",
},
}
`);
});

it('sets initial ui state correctly when instantSearchInstance is re-used', async () => {
const currentRouteState = {
value: {
'my-index': {
query: 'iPhone',
},
},
};
const searchClient = createSearchClient();
const search = instantsearch({
indexName: 'my-index',
searchClient,
routing: {
router: {
onUpdate: () => {},
read: () => {
return currentRouteState.value;
},
write: () => {},
createURL: () => '',
dispose: () => {},
},
},
});
search.addWidgets([
searchBox({ container: document.createElement('div') }),
]);
search.start();
await wait(0);
expect(search.getUiState()).toMatchInlineSnapshot(`
{
"my-index": {
"query": "iPhone",
},
}
`);

search.dispose();
currentRouteState.value = {
'my-index': {
query: 'MacBook',
},
};
search.addWidgets([
searchBox({ container: document.createElement('div') }),
]);
search.start();
await wait(0);
expect(search.getUiState()).toMatchInlineSnapshot(`
{
"my-index": {
"query": "MacBook",
},
}
`);
});
});
42 changes: 31 additions & 11 deletions src/middlewares/createInsightsMiddleware.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import type {
} from '../types';
import { getInsightsAnonymousUserTokenInternal } from '../helpers';
import { warning, noop, getAppIdAndApiKey, find } from '../lib/utils';
import connectConfigure from '../connectors/configure/connectConfigure';
Haroenv marked this conversation as resolved.
Show resolved Hide resolved

export type InsightsEvent = {
insightsMethod?: InsightsClientMethod;
Expand Down Expand Up @@ -88,25 +89,32 @@ export const createInsightsMiddleware: CreateInsightsMiddleware = (props) => {
});
insightsClient('init', { appId, apiKey, ...insightsInitParams });

const createWidget = connectConfigure(noop);
let configureClickAnalytics: ReturnType<typeof createWidget> | undefined;
let configureUserToken: ReturnType<typeof createWidget> | undefined;

return {
onStateChange() {},
subscribe() {
insightsClient('addAlgoliaAgent', 'insights-middleware');

// At the time this middleware is subscribed, `mainIndex.init()` is already called.
// It means `mainIndex.getHelper()` exists.
const helper = instantSearchInstance.mainIndex.getHelper()!;
Copy link
Contributor Author

@eunjae-lee eunjae-lee Aug 31, 2021

Choose a reason for hiding this comment

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

The insights middleware used to rely on the helper to set/get user token. However, now that we get this subscribe() called before mainIndex.init() we no longer have access to the helper at subscribe(). We could setTimeout(fn, 0) but it will make the first query being fired without user token.

Anyway I don't like touching helper directly here, so changed the implementation to create a custom configure widget within the middleware to manipulate the search parameters.

configureClickAnalytics = createWidget({
Haroenv marked this conversation as resolved.
Show resolved Hide resolved
searchParameters: { clickAnalytics: true },
});
instantSearchInstance.addWidgets([configureClickAnalytics]);

const setUserTokenToSearch = (userToken?: string) => {
if (userToken) {
helper.setState(
helper.state.setQueryParameter('userToken', userToken)
);
if (configureUserToken) {
instantSearchInstance.renderState[
instantSearchInstance.indexName
].configure!.refine({ userToken });
Haroenv marked this conversation as resolved.
Show resolved Hide resolved
} else {
configureUserToken = createWidget({
searchParameters: { userToken },
});
instantSearchInstance.addWidgets([configureUserToken]);
}
};
const hasUserToken = () => Boolean((helper.state as any).userToken);

helper.setState(helper.state.setQueryParameter('clickAnalytics', true));

const anonymousUserToken = getInsightsAnonymousUserTokenInternal();
if (hasInsightsClient && anonymousUserToken) {
Expand All @@ -132,7 +140,13 @@ export const createInsightsMiddleware: CreateInsightsMiddleware = (props) => {
if (onEvent) {
onEvent(event, _insightsClient);
} else if (event.insightsMethod) {
if (hasUserToken()) {
// At this point, instantSearchInstance must be started and
// it means there is a configure widget (added above).
const hasUserToken = Boolean(
instantSearchInstance.renderState[instantSearchInstance.indexName]
.configure!.widgetParams.searchParameters.userToken
Haroenv marked this conversation as resolved.
Show resolved Hide resolved
);
if (hasUserToken) {
insightsClient(event.insightsMethod, event.payload);
} else {
warning(
Expand All @@ -154,6 +168,12 @@ See documentation: https://www.algolia.com/doc/guides/building-search-ui/going-f
},
unsubscribe() {
insightsClient('onUserTokenChange', undefined);
instantSearchInstance.removeWidgets([
configureClickAnalytics!,
configureUserToken!,
]);
configureClickAnalytics = undefined;
configureUserToken = undefined;
instantSearchInstance.sendEventToInsights = noop;
},
};
Expand Down
11 changes: 7 additions & 4 deletions src/middlewares/createRouterMiddleware.ts
Original file line number Diff line number Diff line change
Expand Up @@ -57,13 +57,11 @@ export const createRouterMiddleware = <
// casting to UiState here to keep createURL unaware of custom UiState
// (as long as it's an object, it's ok)
instantSearchInstance._createURL = topLevelCreateURL as CreateURL<UiState>;
instantSearchInstance._initialUiState = {
...instantSearchInstance._initialUiState,
...stateMapping.routeToState(router.read()),
};

Haroenv marked this conversation as resolved.
Show resolved Hide resolved
let lastRouteState: TRouteState | undefined = undefined;

const initialUiState = instantSearchInstance._initialUiState;

return {
onStateChange({ uiState }) {
const routeState = stateMapping.stateToRoute(uiState);
Expand All @@ -78,6 +76,11 @@ export const createRouterMiddleware = <
},

subscribe() {
instantSearchInstance._initialUiState = {
...initialUiState,
Haroenv marked this conversation as resolved.
Show resolved Hide resolved
...stateMapping.routeToState(router.read()),
};

router.onUpdate((route) => {
instantSearchInstance.setUiState(stateMapping.routeToState(route));
});
Expand Down