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(insights): do not throw when sending event right after creating insights middleware #4575

Merged
merged 8 commits into from
Nov 12, 2020
106 changes: 67 additions & 39 deletions src/middlewares/__tests__/createInsightsMiddleware.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import {
createAlgoliaAnalytics,
createInsightsClient,
createInsightsUmdVersion,
AlgoliaAnalytics,
ANONYMOUS_TOKEN,
} from '../../../test/mock/createInsightsClient';
import { warning } from '../../lib/utils';
Expand Down Expand Up @@ -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 = {};
});
Expand Down Expand Up @@ -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({
Copy link
Member

Choose a reason for hiding this comment

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

We keep testing internals here, that's fragile. Again, that's how it was before these 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.

Although the method is public, in this context, we're indeed testing the internals. I'll keep it that way for now.

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', () => {
Expand Down Expand Up @@ -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,
Expand All @@ -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,
Expand Down Expand Up @@ -222,6 +248,7 @@ aa('setUserToken', 'your-user-token');`);
insightsClient,
instantSearchInstance,
getUserToken,
libraryLoadedAndProcessQueue,
} = createUmdTestEnvironment();

// call init and setUserToken even before the library is loaded.
Expand All @@ -236,6 +263,7 @@ aa('setUserToken', 'your-user-token');`);
insightsClient,
})({ instantSearchInstance });
middleware.subscribe();
libraryLoadedAndProcessQueue();
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 was missing. We need this because the library will eventually be loaded.

expect(getUserToken()).toEqual('token-from-queue');
});

Expand Down
73 changes: 37 additions & 36 deletions src/middlewares/createInsightsMiddleware.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,26 +38,36 @@ export const createInsightsMiddleware: CreateInsightsMiddleware = props => {
_insightsClient === null ? (noop as InsightsClient) : _insightsClient;
Copy link
Member

Choose a reason for hiding this comment

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

If we type it correctly here, we shouldn't have to cast insightsClient every time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean regarding (insightsClient as any).queue right?
5ddedee


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() {},
Expand All @@ -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)) {
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 part has been move to the line 44. And there it doesn't call setUserTokenToSearch(). It instead sets a variable there, and later on, at the line 95, it reads the variable and sets it.

// 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)`
Expand Down
10 changes: 9 additions & 1 deletion src/types/insights.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,11 @@ export type InsightsClientPayload = {
positions?: number[];
};

export type InsightsSetUserToken = (
method: 'setUserToken',
userToken: string
) => void;

export type InsightsSendEvent = (
method: InsightsClientMethod,
payload: InsightsClientPayload
Expand Down Expand Up @@ -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,
Expand Down
26 changes: 21 additions & 5 deletions test/mock/createInsightsClient.ts
Original file line number Diff line number Diff line change
@@ -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 => {
Expand All @@ -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()) {
Expand All @@ -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 || [];
Expand All @@ -55,15 +69,17 @@ 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);
});
queue.push = ([methodName, ...args]) => {
_aa(methodName, ...args);
};
return {
algoliaAnalytics,
};
},
};
}