-
Notifications
You must be signed in to change notification settings - Fork 332
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): pass clickAnalytics automatically #1080
Conversation
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 68a9008:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
things that could be changed
params: expect.objectContaining({ clickAnalytics: true }), | ||
}), | ||
expect.objectContaining({ | ||
params: expect.objectContaining({ clickAnalytics: true }), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this behaviour expected (facet search with clickAnalytics: true)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like no queryID is returned anyway, it's likely ignored by the engine
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to filter it out to avoid sending an unnecessary larger payload?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to prevent wrong clickAnalytics, you don't want to say the search is tracked, when there's no clicks happen
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We discussed and this is unlikely, and we'll deal with it when there ever is a queryID in sffv
packages/autocomplete-plugin-algolia-insights/src/createAlgoliaInsightsPlugin.ts
Outdated
Show resolved
Hide resolved
packages/autocomplete-plugin-algolia-insights/src/__tests__/createAlgoliaInsightsPlugin.test.ts
Outdated
Show resolved
Hide resolved
params: expect.objectContaining({ clickAnalytics: true }), | ||
}), | ||
expect.objectContaining({ | ||
params: expect.objectContaining({ clickAnalytics: true }), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to filter it out to avoid sending an unnecessary larger payload?
Summary
adds a new mechanism in preResolve to check the context for any extra query parameters to set. The insights plugin will set
clickAnalytics: true
as one of those parameters to apply by default.To debate: should clickAnalytics be true for facet searches? We don't send the same automatic events if the items don't have objectID (as click is different). I'm guessing we can ignore that, as searchForFacetValues doesn't appear in analytics.
To debate: should the key be
queryParameters
,searchParameters
orquery.params
?Results
We automatically pass
clickAnalytics: true
parameter if the insights plugin is used.FX-2191