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

fix(insights): do not throw when sending event right after creating insights middleware #4575

Merged
merged 8 commits into from
Nov 12, 2020
Merged

fix(insights): do not throw when sending event right after creating insights middleware #4575

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
97244a5
test: test if insightsClient does not throw when an event is sent rig…
eunjae-lee Nov 9, 2020
9effefd
fix: restore user token which was set before creating the middleware
eunjae-lee Nov 9, 2020
1d3c977
test: make it clearer
eunjae-lee Nov 9, 2020
fa7ea26
use find instead of loop
eunjae-lee Nov 12, 2020
5ddedee
add queue to InsightsClient type
eunjae-lee Nov 12, 2020
8b10f89
clean up flow
eunjae-lee Nov 12, 2020
0add3dc
change the way to test viewedObjectIDs
eunjae-lee Nov 12, 2020
b3e0247
Merge branch 'master' into fix/insights-init
Nov 12, 2020
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
107 changes: 68 additions & 39 deletions src/middlewares/__tests__/createInsightsMiddleware.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,30 @@ describe('insights', () => {
};
};

const createUmdTestEnvironment = () => {
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 has been moved here from other describe scope with no change.

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,
};
};

beforeEach(() => {
warning.cache = {};
});
Expand Down Expand Up @@ -79,22 +103,48 @@ 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', done => {
const {
insightsClient,
libraryLoadedAndProcessQueue,
instantSearchInstance,
} = createUmdTestEnvironment();

const middleware = createInsightsMiddleware({
insightsClient,
})({ instantSearchInstance });
middleware.subscribe();

createInsightsMiddleware({ /* ... */ });
setTimeout(() => {
// When the library is loaded later, it consumes the queue and sends the event.
const { algoliaAnalytics } = libraryLoadedAndProcessQueue();
expect(algoliaAnalytics.viewedObjectIDs).toHaveBeenCalledTimes(1);
expect(algoliaAnalytics.viewedObjectIDs).toHaveBeenCalledWith({
eventName: 'Hits Viewed',
index: '',
objectIDs: ['1', '2'],
});
done();
}, 200);

expect(() => {
instantSearchInstance.sendEventToInsights({
eventType: 'view',
insightsMethod: 'viewedObjectIDs',
payload: {
eventName: 'Hits Viewed',
index: '',
objectIDs: ['1', '2'],
},
widgetType: 'ais.hits',
});
}).not.toThrow();

insightsClient('setUserToken', 'your-user-token');
// or
aa('setUserToken', 'your-user-token');`);
// The library is not loaded yet, so it stays in the queue.
expect(insightsClient.queue[insightsClient.queue.length - 1]).toEqual([
'viewedObjectIDs',
{ eventName: 'Hits Viewed', index: '', objectIDs: ['1', '2'] },
]);
});

it('applies clickAnalytics', () => {
Expand Down Expand Up @@ -153,7 +203,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 +216,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 +249,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 +264,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
74 changes: 37 additions & 37 deletions src/middlewares/createInsightsMiddleware.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,26 +38,35 @@ 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.

createInsightsMiddleware({ /* ... */ });

insightsClient('setUserToken', 'your-user-token');
// or
aa('setUserToken', 'your-user-token');
`
);
});
insightsClient('init', { appId, apiKey });
}
const [appId, apiKey] = getAppIdAndApiKey(instantSearchInstance.client);
let queuedUserToken: string | undefined = undefined;
let userTokenBeforeInit: string | undefined = undefined;
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]) => {
Copy link
Member

Choose a reason for hiding this comment

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

Why using forEach if we override the variable every time? Using find would make more sense:

find(insightsClient.queue.slice().reverse(), /* ... */)
  • slice to not mutate the queue
  • reverse to find the latest occurrence

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fa7ea26

if (method === 'setUserToken') {
queuedUserToken = firstArgument;
}
});
}
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 +85,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 (queuedUserToken) {
Copy link
Member

Choose a reason for hiding this comment

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

Why can this condition and the one below be satisfied at the same time? It makes the flow harder to understand.

If that what it means, I'd suggest changing to this:

// 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);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

8b10f89

insightsClient('setUserToken', queuedUserToken);
}

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);
}
});
if (userTokenBeforeInit) {
insightsClient('setUserToken', userTokenBeforeInit);
}

// This updates userToken which is set explicitly by `aa('setUserToken', userToken)`
Expand Down
8 changes: 7 additions & 1 deletion src/types/insights.ts
View file
Open in desktop
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,8 @@ export type InsightsInit = (
export type InsightsClient = InsightsSendEvent &
InsightsOnUserTokenChange &
InsightsGet &
InsightsInit;
InsightsInit &
InsightsSetUserToken;

export type InsightsClientWrapper = (
method: InsightsClientMethod,
Expand Down
15 changes: 14 additions & 1 deletion test/mock/createInsightsClient.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,13 +26,23 @@ export function createAlgoliaAnalytics() {
callback(values._userToken);
}
};
const sendEvent = () => {
Copy link
Member

Choose a reason for hiding this comment

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

Why do we implement another whole logic in its mock?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. I thought about it again, and I think the new test is better.
0add3dc

Copy link
Member

Choose a reason for hiding this comment

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

We probably shouldn't even mock any logic, but just spy for methods called. That's coming from before so it shouldn't block that PR though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At some point, I began to wonder if I just should've used the real search-insights client for the tests, because there are some implementations on the search-insights side that needs to be tested with the insights middleware.

if (!values._hasCredentials) {
throw new Error(
"Before calling any methods on the analytics, you first need to call the 'init' function with appId and apiKey parameters"
);
}
};
const viewedObjectIDs = jest.fn(() => {
sendEvent();
});

return {
setUserToken,
init,
_get,
onUserTokenChange,
viewedObjectIDs: jest.fn(),
viewedObjectIDs,
};
}

Expand Down Expand Up @@ -64,6 +74,9 @@ export function createInsightsUmdVersion() {
queue.push = ([methodName, ...args]) => {
_aa(methodName, ...args);
};
return {
algoliaAnalytics: instance,
};
},
};
}