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

feat(insights): introduce insights middleware (1/4) #4446

Merged
merged 35 commits into from
Sep 2, 2020
Merged
Show file tree
Hide file tree
Changes from 22 commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
3963956
feat: update userToken automatically
eunjae-lee Jul 15, 2020
4331d24
chore: fix bug where it gets token before rendering the custom widget
eunjae-lee Jul 15, 2020
77996c9
chore: add access modifier
eunjae-lee Jul 15, 2020
467a648
chore: use custom widget and helper
eunjae-lee Jul 16, 2020
ec3f7bb
Update src/lib/InstantSearch.ts
Jul 16, 2020
3ab96a2
chore: update token to helper directly
eunjae-lee Jul 16, 2020
020381e
chore: move setupUserTokenUpdater to start()
eunjae-lee Jul 16, 2020
a0cc96f
chore: move to middleware
eunjae-lee Jul 23, 2020
c642234
Update src/middleware/insights.ts
Jul 23, 2020
03e52c7
export middleware
eunjae-lee Jul 23, 2020
0ae09e2
fix lint error
eunjae-lee Jul 23, 2020
2417c50
export for umd build
eunjae-lee Jul 23, 2020
090fe28
Update src/middleware/insights.ts
Jul 23, 2020
c262700
Merge branch 'master' into feat/automatic-user-token
Jul 23, 2020
2a49909
update token automatically
eunjae-lee Jul 23, 2020
20254cf
chore: update error message
eunjae-lee Jul 24, 2020
10ce292
inline functions
eunjae-lee Jul 24, 2020
5a1fc34
initialize insightsClient earlier
eunjae-lee Jul 24, 2020
963d305
add warning message if userToken is set before creating the middleware
eunjae-lee Jul 27, 2020
daca533
accept `false` for `insightsClient`
eunjae-lee Jul 27, 2020
000d708
clean up types
eunjae-lee Jul 29, 2020
602037a
fix wrong import
eunjae-lee Jul 29, 2020
8fd4698
Update src/middleware/insights.ts
Aug 24, 2020
b0b7add
accept null as insightsClient
eunjae-lee Aug 24, 2020
e2a2743
Merge branch 'master' into feat/automatic-user-token
Aug 24, 2020
21faadb
Update src/types/insights.ts
Aug 24, 2020
7f49f25
add test for getAppIdAndApiKey
eunjae-lee Aug 24, 2020
94e394b
Update src/types/insights.ts
Aug 24, 2020
ad0dafb
bring back exports
eunjae-lee Aug 24, 2020
f34170d
rename middleware to middlewares
eunjae-lee Aug 24, 2020
ef95a87
Merge branch 'master' into feat/automatic-user-token
eunjae-lee Aug 27, 2020
183b1ca
chore: rename files for better alignment
eunjae-lee Aug 31, 2020
e7c7e58
chore: export all types related to insights middleware
eunjae-lee Aug 31, 2020
60af9e0
feat(insights): send events from hits and refinementList (2/4) (#4456)
Sep 2, 2020
5067ec7
Merge branch 'master' into feat/automatic-user-token
Sep 2, 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
3 changes: 2 additions & 1 deletion src/lib/InstantSearch.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,10 @@ import {
Widget,
UiState,
CreateURL,
Middleware,
MiddlewareDefinition,
} from '../types';
import hasDetectedInsightsClient from './utils/detect-insights-client';
import { Middleware, MiddlewareDefinition } from '../middleware';
import { createRouter, RouterProps } from '../middleware/createRouter';

const withUsage = createDocumentationMessageGenerator({
Expand Down
2 changes: 2 additions & 0 deletions src/lib/main.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import version from './version';
import * as connectors from '../connectors/index';
import * as widgets from '../widgets/index';
import * as helpers from '../helpers/index';
import * as middleware from '../middleware/index';

import * as routers from './routers/index';
import * as stateMappings from './stateMappings/index';
Expand Down Expand Up @@ -39,5 +40,6 @@ instantsearch.createInfiniteHitsSessionStorageCache = createInfiniteHitsSessionS
instantsearch.highlight = helpers.highlight;
instantsearch.snippet = helpers.snippet;
instantsearch.insights = helpers.insights;
instantsearch.middleware = middleware;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

middleware vs middlewares

Copy link
Member

Choose a reason for hiding this comment

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

"middlewares" is not proper English.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://en.wiktionary.org/wiki/middleware
image

I don't think I've seen s at the end of middleware before, though.
So you agree it's natural to have middleware although we have routers, stateMappings, connectors, widgets, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it should be middlewares, at least for the object containing all possible middlewares. Interestingly my spell-checker only accepts singular "middleware" as correct, and not "middlewares"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

https://www.wordhippo.com/what-is/the-plural-of/middleware.html

While middleware is more common, it's also safe to use middlewares if we want to explicitly indicate it's a collection.

Copy link
Contributor Author

Choose a reason for hiding this comment

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


export default instantsearch;
3 changes: 1 addition & 2 deletions src/middleware/createRouter.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
import simpleStateMapping from '../lib/stateMappings/simple';
import historyRouter from '../lib/routers/history';
import { Index } from '../widgets/index/index';
import { Middleware } from '.';
import { Router, StateMapping, UiState } from '../types';
import { Router, StateMapping, UiState, Middleware } from '../types';

const walk = (current: Index, callback: (index: Index) => void) => {
callback(current);
Expand Down
14 changes: 1 addition & 13 deletions src/middleware/index.ts
Original file line number Diff line number Diff line change
@@ -1,13 +1 @@
import { UiState } from '../types';

export type MiddlewareDefinition = {
onStateChange(options: { uiState: UiState }): void;
subscribe(): void;
unsubscribe(): void;
};

export type Middleware = ({
instantSearchInstance: InstantSearch,
}) => MiddlewareDefinition;

export { createRouter, RouterProps } from './createRouter';
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure if anyone uses this, but this changes the API

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added them back ad0dafb
but not like before because of this
(tl;dr -> rollup failed to resolve RouterProps type from ./createRouter. It didn't happen before because this was not exported at src/lib/main.ts, but not it is.)

export { createInsightsMiddleware } from './insights';
114 changes: 114 additions & 0 deletions src/middleware/insights.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,114 @@
import { InsightsClient, Middleware } from '../types';
import { getInsightsAnonymousUserToken } from '../helpers';
import { warning, noop } from '../lib/utils';

export type InsightsProps = {
insightsClient: false | InsightsClient;
Copy link
Member

Choose a reason for hiding this comment

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

Accepting false for the Insights client is confusing because it can't be true. Why don't we use null as we're doing with other options?

Copy link
Contributor

Choose a reason for hiding this comment

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

null is a fair option, I suggested false, since that's what I did with Vue: https://github.com/algolia/vue-instantsearch/blob/master/src/mixins/widget.js#L47-L55

EDIT: it's actually true there, but null would make more sense (maybe a next major)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, null sounds good.
b0b7add

};

export type CreateInsightsMiddleware = (props: InsightsProps) => Middleware;

export const createInsightsMiddleware: CreateInsightsMiddleware = props => {
const { insightsClient: _insightsClient } = props;
if (_insightsClient !== false && !_insightsClient) {
if (__DEV__) {
throw new Error(
"The `insightsClient` option is required if you want userToken to be automatically set in search calls. If you don't want this behaviour, set it to `false`."
);
} else {
throw new Error(
'The `insightsClient` option is required. To disable, set it to `false`.'
);
}
}

const hasInsightsClient = Boolean(_insightsClient);
const insightsClient =
_insightsClient === false ? (noop as InsightsClient) : _insightsClient;

eunjae-lee marked this conversation as resolved.
Show resolved Hide resolved
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 });
}
});

return {
onStateChange() {},
subscribe() {
const setUserTokenToSearch = (userToken?: string) => {
// At the time this middleware is subscribed, `mainIndex.init()` is already called.
// It means `mainIndex.getHelper()` exists.
if (userToken) {
instantSearchInstance.mainIndex
.getHelper()!
.setQueryParameter('userToken', userToken);
}
};

instantSearchInstance.mainIndex
.getHelper()!
.setQueryParameter('clickAnalytics', true);

if (hasInsightsClient) {
// When `aa('init', { ... })` is called, it creates an anonymous user token in cookie.
// We can set it as userToken.
setUserTokenToSearch(getInsightsAnonymousUserToken());
}

if (Array.isArray((insightsClient as any).queue)) {
// Context: The umd build of search-insights is asynchronously loaded by the snippet.
//
// When user called `aa('setUserToken', 'my-user-token')` before `search-insights` is loaded,
eunjae-lee marked this conversation as resolved.
Show resolved Hide resolved
// it is stored in `aa.queue` and we are reading it to set userToken to search call.
// This queue is meant to be consumed whenever `search-insights` is loaded and when it runs `processQueue()`.
// But the reason why we handle it here is to prevent the first search API from being triggered
// without userToken because search-insights is not loaded yet.
(insightsClient as any).queue.forEach(([method, firstArgument]) => {
if (method === 'setUserToken') {
setUserTokenToSearch(firstArgument);
}
});
}

// This updates userToken which is set explicitly by `aa('setUserToken', userToken)`
insightsClient('onUserTokenChange', setUserTokenToSearch, {
immediate: true,
});
},
unsubscribe() {
insightsClient('onUserTokenChange', undefined);
},
};
};
};

function getAppIdAndApiKey(searchClient) {
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe add a unit test here. Since we run the tests with algoliasearch v3 and v4 separately, you should be able to cover both branches.

Other case we can add a dev dependency for "algoliasearch-v3": "npm:algoliasearch@3" and import algoliasearch from 'algoliasearc-v3'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TIL: "npm:algoliasearch@3" syntax. Nice! 7f49f25

if (searchClient.transporter) {
// searchClient v4
const { headers, queryParameters } = searchClient.transporter;
const APP_ID = 'x-algolia-application-id';
const API_KEY = 'x-algolia-api-key';
const appId = headers[APP_ID] || queryParameters[APP_ID];
const apiKey = headers[API_KEY] || queryParameters[API_KEY];
return [appId, apiKey];
} else {
// searchClient v3
return [searchClient.applicationID, searchClient.apiKey];
}
}
1 change: 1 addition & 0 deletions src/types/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,4 @@ export * from './connector';
export * from './widget';
export * from './insights';
export * from './algoliasearch';
export * from './middleware';
francoischalifour marked this conversation as resolved.
Show resolved Hide resolved
29 changes: 26 additions & 3 deletions src/types/insights.ts
Original file line number Diff line number Diff line change
@@ -1,20 +1,43 @@
export type InsightsClientMethod =
| 'clickedObjectIDsAfterSearch'
| 'convertedObjectIDsAfterSearch';

export type InsightsClientPayload = {
eunjae-lee marked this conversation as resolved.
Show resolved Hide resolved
eventName: string;
queryID: string;
index: string;
objectIDs: string[];
positions?: number[];
};

export type InsightsClient = (
export type InsightsSendEvent = (
eunjae-lee marked this conversation as resolved.
Show resolved Hide resolved
method: InsightsClientMethod,
payload: InsightsClientPayload
) => void;

export type InsightsOnUserTokenChange = (
method: 'onUserTokenChange',
callback?: (userToken: string) => void,
options?: { immediate?: boolean }
) => void;

export type InsightsGet = (
method: '_get',
key: string,
callback: (value: any) => void
) => void;

export type InsightsInit = (
method: 'init',
options: {
appId: string;
apiKey: string;
}
) => void;

export type InsightsClient = InsightsSendEvent &
InsightsOnUserTokenChange &
InsightsGet &
InsightsInit;

export type InsightsClientWrapper = (
method: InsightsClientMethod,
payload: Partial<InsightsClientPayload>
Expand Down
14 changes: 14 additions & 0 deletions src/types/middleware.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
import InstantSearch from '../lib/InstantSearch';
import { UiState } from './widget';

export type MiddlewareDefinition = {
onStateChange(options: { uiState: UiState }): void;
subscribe(): void;
unsubscribe(): void;
};

export type MiddlewareOptions = {
instantSearchInstance: InstantSearch;
};

export type Middleware = (options: MiddlewareOptions) => MiddlewareDefinition;