fix(insights): send extra parameters only for applicable versions #5558
Conversation
still to do: update tests
|
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 f2cac54:
|
packages/instantsearch.js/src/middlewares/createInsightsMiddleware.ts
Outdated
Show resolved
Hide resolved
Haroenv
requested review from
dhayab,
a team and
aymeric-giraudet
and removed request for
a team
| @@ -138,7 +138,7 @@ export function _buildEventPayloadsForHits({ | |||
| widgetType, | |||
| eventType, | |||
| payload: { | |||
| eventName, | |||
| eventName: eventName || 'Hit Converted', | |||
There was a problem hiding this comment.
alternatively eventName could be required in the type when it's click or convert, but not sure if that can easily be done
There was a problem hiding this comment.
We could maybe have sendEventForHits satisfy both a custom payload signature and type, hit, eventName? For now though, having those defaults works for me.
There was a problem hiding this comment.
Yes, I prefer defaults too, as typescript types aren't enforced at runtime, meaning there will be more valid events.
| @@ -138,7 +138,7 @@ export function _buildEventPayloadsForHits({ | |||
| widgetType, | |||
| eventType, | |||
| payload: { | |||
| eventName, | |||
| eventName: eventName || 'Hit Converted', | |||
There was a problem hiding this comment.
We could maybe have sendEventForHits satisfy both a custom payload signature and type, hit, eventName? For now though, having those defaults works for me.
packages/instantsearch.js/src/middlewares/__tests__/createInsightsMiddleware.ts
Show resolved
Hide resolved
Summary
In v2, pre 2.4.0 sendEvent is variadic, meaning that the "extra parameters" were sent as an extra network request.
This wasn't clear in the tests, as we were asserting we were sending two arguments to search-insights, but possibly we could have asserted what actual network request was sent. That would have required a whole set of search-insights versions as dependency, which seems overly complex to me.
Result