From d963f8d6155e6bb56f852e00528ed10dc9bcc461 Mon Sep 17 00:00:00 2001 From: Eunjae Lee Date: Thu, 12 Nov 2020 18:14:58 +0100 Subject: [PATCH] fix(insights): do not throw when sending event right after creating insights middleware (#4575) * test: test if insightsClient does not throw when an event is sent right after creating the middleware * fix: restore user token which was set before creating the middleware * test: make it clearer * use find instead of loop * add queue to InsightsClient type * clean up flow * change the way to test viewedObjectIDs --- .../__tests__/createInsightsMiddleware.ts | 106 +++++++++++------- src/middlewares/createInsightsMiddleware.ts | 73 ++++++------ src/types/insights.ts | 10 +- test/mock/createInsightsClient.ts | 26 ++++- 4 files changed, 134 insertions(+), 81 deletions(-) diff --git a/src/middlewares/__tests__/createInsightsMiddleware.ts b/src/middlewares/__tests__/createInsightsMiddleware.ts index 1cd291fa49..2d8c4aa988 100644 --- a/src/middlewares/__tests__/createInsightsMiddleware.ts +++ b/src/middlewares/__tests__/createInsightsMiddleware.ts @@ -6,6 +6,7 @@ import { createAlgoliaAnalytics, createInsightsClient, createInsightsUmdVersion, + AlgoliaAnalytics, ANONYMOUS_TOKEN, } from '../../../test/mock/createInsightsClient'; import { warning } from '../../lib/utils'; @@ -36,6 +37,30 @@ describe('insights', () => { }; }; + const createUmdTestEnvironment = (algoliaAnalytics?: AlgoliaAnalytics) => { + const { + insightsClient, + libraryLoadedAndProcessQueue, + } = createInsightsUmdVersion(algoliaAnalytics); + const instantSearchInstance = createInstantSearch({ + client: algoliasearch('myAppId', 'myApiKey'), + }); + const helper = algoliasearchHelper({} as SearchClient, ''); + const getUserToken = () => { + return (helper.state as any).userToken; + }; + instantSearchInstance.mainIndex = { + getHelper: () => helper, + } as Index; + return { + insightsClient, + libraryLoadedAndProcessQueue, + instantSearchInstance, + helper, + getUserToken, + }; + }; + beforeEach(() => { warning.cache = {}; }); @@ -79,22 +104,46 @@ describe('insights', () => { }); }); - it('warns dev if userToken is set before creating the middleware', () => { - const { insightsClient, instantSearchInstance } = createTestEnvironment(); - insightsClient('setUserToken', 'abc'); - expect(() => { - createInsightsMiddleware({ - insightsClient, - })({ instantSearchInstance }); - }) - .toWarnDev(`[InstantSearch.js]: You set userToken before \`createInsightsMiddleware()\` and it is ignored. -Please set the token after the \`createInsightsMiddleware()\` call. + it('does not throw when an event is sent right after the creation in UMD', () => { + const algoliaAnalytics = createAlgoliaAnalytics(); + const { + insightsClient, + libraryLoadedAndProcessQueue, + instantSearchInstance, + } = createUmdTestEnvironment(algoliaAnalytics); -createInsightsMiddleware({ /* ... */ }); + const middleware = createInsightsMiddleware({ + insightsClient, + })({ instantSearchInstance }); + middleware.subscribe(); -insightsClient('setUserToken', 'your-user-token'); -// or -aa('setUserToken', 'your-user-token');`); + // It tries to send an event. + instantSearchInstance.sendEventToInsights({ + eventType: 'view', + insightsMethod: 'viewedObjectIDs', + payload: { + eventName: 'Hits Viewed', + index: '', + objectIDs: ['1', '2'], + }, + widgetType: 'ais.hits', + }); + expect(algoliaAnalytics.viewedObjectIDs).toHaveBeenCalledTimes(0); + + // But, the library hasn't been loaded yet, so the event stays in the queue. + expect(insightsClient.queue[insightsClient.queue.length - 1]).toEqual([ + 'viewedObjectIDs', + { eventName: 'Hits Viewed', index: '', objectIDs: ['1', '2'] }, + ]); + + // When the library is loaded later, it consumes the queue and sends the event. + libraryLoadedAndProcessQueue(); + expect(algoliaAnalytics.viewedObjectIDs).toHaveBeenCalledTimes(1); + expect(algoliaAnalytics.viewedObjectIDs).toHaveBeenCalledWith({ + eventName: 'Hits Viewed', + index: '', + objectIDs: ['1', '2'], + }); }); it('applies clickAnalytics', () => { @@ -153,7 +202,7 @@ aa('setUserToken', 'your-user-token');`); expect(getUserToken()).toEqual(ANONYMOUS_TOKEN); }); - it('ignores userToken set before init', () => { + it('applies userToken which was set before init', () => { const { insightsClient, instantSearchInstance, @@ -166,33 +215,10 @@ aa('setUserToken', 'your-user-token');`); insightsClient, })({ instantSearchInstance }); middleware.subscribe(); - expect(getUserToken()).toEqual(ANONYMOUS_TOKEN); + expect(getUserToken()).toEqual('token-from-queue-before-init'); }); describe('umd', () => { - const createUmdTestEnvironment = () => { - const { - insightsClient, - libraryLoadedAndProcessQueue, - } = createInsightsUmdVersion(); - const instantSearchInstance = createInstantSearch({ - client: algoliasearch('myAppId', 'myApiKey'), - }); - const helper = algoliasearchHelper({} as SearchClient, ''); - const getUserToken = () => { - return (helper.state as any).userToken; - }; - instantSearchInstance.mainIndex = { - getHelper: () => helper, - } as Index; - return { - insightsClient, - libraryLoadedAndProcessQueue, - instantSearchInstance, - helper, - getUserToken, - }; - }; it('applies userToken from queue if exists', () => { const { insightsClient, @@ -222,6 +248,7 @@ aa('setUserToken', 'your-user-token');`); insightsClient, instantSearchInstance, getUserToken, + libraryLoadedAndProcessQueue, } = createUmdTestEnvironment(); // call init and setUserToken even before the library is loaded. @@ -236,6 +263,7 @@ aa('setUserToken', 'your-user-token');`); insightsClient, })({ instantSearchInstance }); middleware.subscribe(); + libraryLoadedAndProcessQueue(); expect(getUserToken()).toEqual('token-from-queue'); }); diff --git a/src/middlewares/createInsightsMiddleware.ts b/src/middlewares/createInsightsMiddleware.ts index 09cd78ee21..f52db1ec04 100644 --- a/src/middlewares/createInsightsMiddleware.ts +++ b/src/middlewares/createInsightsMiddleware.ts @@ -38,26 +38,36 @@ export const createInsightsMiddleware: CreateInsightsMiddleware = props => { _insightsClient === null ? (noop as InsightsClient) : _insightsClient; return ({ instantSearchInstance }) => { - insightsClient('_get', '_hasCredentials', (hasCredentials: boolean) => { - if (!hasCredentials) { - const [appId, apiKey] = getAppIdAndApiKey(instantSearchInstance.client); - insightsClient('_get', '_userToken', (userToken: string) => { - warning( - !userToken, - `You set userToken before \`createInsightsMiddleware()\` and it is ignored. -Please set the token after the \`createInsightsMiddleware()\` call. + const [appId, apiKey] = getAppIdAndApiKey(instantSearchInstance.client); + let queuedUserToken: string | undefined = undefined; + let userTokenBeforeInit: string | undefined = undefined; -createInsightsMiddleware({ /* ... */ }); - -insightsClient('setUserToken', 'your-user-token'); -// or -aa('setUserToken', 'your-user-token'); - ` - ); - }); - insightsClient('init', { appId, apiKey }); - } + if (Array.isArray(insightsClient.queue)) { + // Context: The umd build of search-insights is asynchronously loaded by the snippet. + // + // When user calls `aa('setUserToken', 'my-user-token')` before `search-insights` is loaded, + // ['setUserToken', 'my-user-token'] gets stored in `aa.queue`. + // Whenever `search-insights` is finally loaded, it will process the queue. + // + // But here's the reason why we handle it here: + // At this point, even though `search-insights` is not loaded yet, + // we still want to read the token from the queue. + // Otherwise, the first search call will be fired without the token. + [, queuedUserToken] = + insightsClient.queue + .slice() + .reverse() + .find(([method]) => method === 'setUserToken') || []; + } + insightsClient('_get', '_userToken', (userToken: string) => { + // If user has called `aa('setUserToken', 'my-user-token')` before creating + // the `insights` middleware, we store them temporarily and + // set it later on. + // + // Otherwise, the `init` call might override it with anonymous user token. + userTokenBeforeInit = userToken; }); + insightsClient('init', { appId, apiKey }); return { onStateChange() {}, @@ -76,28 +86,19 @@ aa('setUserToken', 'your-user-token'); .getHelper()! .setQueryParameter('clickAnalytics', true); - if (hasInsightsClient) { + const anonymousUserToken = getInsightsAnonymousUserTokenInternal(); + if (hasInsightsClient && anonymousUserToken) { // When `aa('init', { ... })` is called, it creates an anonymous user token in cookie. // We can set it as userToken. - setUserTokenToSearch(getInsightsAnonymousUserTokenInternal()); + setUserTokenToSearch(anonymousUserToken); } - if (Array.isArray((insightsClient as any).queue)) { - // Context: The umd build of search-insights is asynchronously loaded by the snippet. - // - // When user calls `aa('setUserToken', 'my-user-token')` before `search-insights` is loaded, - // ['setUserToken', 'my-user-token'] gets stored in `aa.queue`. - // Whenever `search-insights` is finally loaded, it will process the queue. - // - // But here's the reason why we handle it here: - // At this point, even though `search-insights` is not loaded yet, - // we still want to read the token from the queue. - // Otherwise, the first search call will be fired without the token. - (insightsClient as any).queue.forEach(([method, firstArgument]) => { - if (method === 'setUserToken') { - setUserTokenToSearch(firstArgument); - } - }); + // We consider the `userToken` coming from a `init` call to have a higher + // importance than the one coming from the queue. + if (userTokenBeforeInit) { + insightsClient('setUserToken', userTokenBeforeInit); + } else if (queuedUserToken) { + insightsClient('setUserToken', queuedUserToken); } // This updates userToken which is set explicitly by `aa('setUserToken', userToken)` diff --git a/src/types/insights.ts b/src/types/insights.ts index 743229cfe3..eb147791f0 100644 --- a/src/types/insights.ts +++ b/src/types/insights.ts @@ -12,6 +12,11 @@ export type InsightsClientPayload = { positions?: number[]; }; +export type InsightsSetUserToken = ( + method: 'setUserToken', + userToken: string +) => void; + export type InsightsSendEvent = ( method: InsightsClientMethod, payload: InsightsClientPayload @@ -40,7 +45,10 @@ export type InsightsInit = ( export type InsightsClient = InsightsSendEvent & InsightsOnUserTokenChange & InsightsGet & - InsightsInit; + InsightsInit & + InsightsSetUserToken & { + queue?: Array<[string, any]>; + }; export type InsightsClientWrapper = ( method: InsightsClientMethod, diff --git a/test/mock/createInsightsClient.ts b/test/mock/createInsightsClient.ts index b4cff2c4d5..e4fca92cbc 100644 --- a/test/mock/createInsightsClient.ts +++ b/test/mock/createInsightsClient.ts @@ -1,5 +1,16 @@ export const ANONYMOUS_TOKEN = 'anonymous-user-id-1'; +export type AlgoliaAnalytics = { + setUserToken: (userToken: string) => void; + init: ({ appId, apiKey }) => void; + _get: (key: string, callback: Function) => void; + onUserTokenChange: ( + callback: Function, + options?: { immediate?: boolean } + ) => void; + viewedObjectIDs: Function; +}; + export function createAlgoliaAnalytics() { let values: any = {}; const setValues = obj => { @@ -26,14 +37,15 @@ export function createAlgoliaAnalytics() { callback(values._userToken); } }; + const viewedObjectIDs = jest.fn(); return { setUserToken, init, _get, onUserTokenChange, - viewedObjectIDs: jest.fn(), - }; + viewedObjectIDs, + } as AlgoliaAnalytics; } export function createInsightsClient(instance = createAlgoliaAnalytics()) { @@ -45,7 +57,9 @@ export function createInsightsClient(instance = createAlgoliaAnalytics()) { }; } -export function createInsightsUmdVersion() { +export function createInsightsUmdVersion( + algoliaAnalytics = createAlgoliaAnalytics() +) { const globalObject: any = {}; globalObject.aa = (...args) => { globalObject.aa.queue = globalObject.aa.queue || []; @@ -55,8 +69,7 @@ export function createInsightsUmdVersion() { return { insightsClient: globalObject.aa, libraryLoadedAndProcessQueue: () => { - const instance = createAlgoliaAnalytics(); - const _aa = createInsightsClient(instance); + const _aa = createInsightsClient(algoliaAnalytics); const queue = globalObject.aa.queue; queue.forEach(([methodName, ...args]) => { _aa(methodName, ...args); @@ -64,6 +77,9 @@ export function createInsightsUmdVersion() { queue.push = ([methodName, ...args]) => { _aa(methodName, ...args); }; + return { + algoliaAnalytics, + }; }, }; }