feat(insights): implement sendEvent in all the connectors (3/4) #4463
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced Jul 31, 2020
|
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 27f3125:
|
eunjae-lee
commented
Jul 31, 2020
| export { | ||
| createSendEventForHits, | ||
| createBindEventForHits, | ||
| SendEventForHits, |
There was a problem hiding this comment.
This had an error coming from rollup (probably).
40: export {
41: createSendEventForFacet,
42: SendEventForFacet,
^
43: } from './createSendEventForFacet';
44: export * from './createSendEventForHits';
Error: 'SendEventForFacet' is not exported by src/lib/utils/createSendEventForFacet.ts
at error (/Users/eunjaelee/workspace/instantsearch.js/node_modules/rollup/dist/rollup.js:5330:30)
at Module.error (/Users/eunjaelee/workspace/instantsearch.js/node_modules/rollup/dist/rollup.js:9733:16)
at handleMissingExport (/Users/eunjaelee/workspace/instantsearch.js/node_modules/rollup/dist/rollup.js:9634:28)
at Module.getVariableForExportName (/Users/eunjaelee/workspace/instantsearch.js/node_modules/rollup/dist/rollup.js:9868:24)
at /Users/eunjaelee/workspace/instantsearch.js/node_modules/rollup/dist/rollup.js:9850:39
at Array.map (<anonymous>)
at Module.getTransitiveDependencies (/Users/eunjaelee/workspace/instantsearch.js/node_modules/rollup/dist/rollup.js:9850:14)
at Chunk$1.link (/Users/eunjaelee/workspace/instantsearch.js/node_modules/rollup/dist/rollup.js:10894:48)
at /Users/eunjaelee/workspace/instantsearch.js/node_modules/rollup/dist/rollup.js:13205:23
at runMicrotasks (<anonymous>)
related to: rollup/plugins#71
couldn't find a fix other than changing it to like here.
edit: namedExports didn't work.
There was a problem hiding this comment.
the possible solution would be export * from .... Did you do that?
There was a problem hiding this comment.
Yeah, export * from ... is the current status in this branch, and it's the only solution at the moment.
7 tasks
Haroenv
reviewed
Aug 25, 2020
Haroenv
reviewed
Aug 25, 2020
Haroenv
reviewed
Aug 25, 2020
Haroenv
reviewed
Aug 25, 2020
| @@ -126,25 +133,64 @@ const connectNumericMenu: NumericMenuConnector = function connectNumericMenu( | |||
| })); | |||
|
|
|||
| const connectorState: ConnectorState = {}; | |||
| let sendEvent; | |||
There was a problem hiding this comment.
if you put it in connectorState higher, you probably don't need the variable
There was a problem hiding this comment.
Do you mean connectorState.sendEvent?
There was a problem hiding this comment.
yes, or is that something else?
There was a problem hiding this comment.
ce4f5f0#diff-5a213002ed9abd35556c3f655cc88ef6R162
Now that connectNumericMenu already has connectorState, I put sendEvent inside it for consistency.
Haroenv
reviewed
Aug 25, 2020
|
|
||
| return { | ||
| $$type: 'ais.numericMenu', | ||
|
|
||
| init({ helper, createURL, instantSearchInstance }) { | ||
| sendEvent = (...args) => { |
There was a problem hiding this comment.
this function should probably be in utils as well like createSendEventForFacet
There was a problem hiding this comment.
They all have slightly different implementation due to their own logic. So I just put them inside. I can extract them to util folder. It might improve the readability of the connectors, but won't be re-used somewhere else.
What do you think?
There was a problem hiding this comment.
I think for single-use ones, it might make sense to define this top-level in the connector's file, and once reused it can move to utils?
Haroenv
reviewed
Aug 25, 2020
Haroenv
reviewed
Aug 25, 2020
Haroenv
reviewed
Aug 25, 2020
Haroenv
approved these changes
Aug 25, 2020
eunjae-lee
changed the title
* add tests for userToken * fix import paths * fix test cases to accept null instead of false as insightsClient * update bundlesize * fix lint errors * test sendEventToInsights * use $$type instead of hard-code * remove sendEvent from connectBreadcrumb * moved createSendEvent to the top level in the files * log insights event from storybook * add test for connectors * add tests for createSendEvent helpers * add integration tests for hits and infinite-hits widgets with bindEvent in templates * update bundlesize * Update src/connectors/hits/__tests__/connectHits-test.ts Co-authored-by: Haroen Viaene <[email protected]> * use factory function instead of globally exposed variables * clean up * update comment * use runAllMicroTasks instead of nextTick * fix: type errors * fix wrong import paths * fix: pass insightsClient to onEvent as the second parameter * update titles of describe blocks Co-authored-by: Haroen Viaene <[email protected]>
eunjae-lee
pushed a commit
that referenced
this pull request
Sep 2, 2020
* chore: dummy commit * add sendEvent method * send event from refinementList * accept custom payload at sendEvent * delegate to onEvent or actually send event * store _insightsMiddleware in instantSearchInstance * add clickedFilters to InsightsClientMethod * rename $$type of middlewares * chore: remove ! * add sendEvent for hits * update for consistency * send view event when rendering hits * expose bindEvent to templates * handle insightsEvent from templates * allow single hit for bind/sendEventForHits * assign sendEventToInsights by the middleware * Update src/lib/insights/listener.tsx Co-authored-by: Haroen Viaene <[email protected]> * require eventName for click and conversion in hits * fix type errors * fix test cases * reuse $$type from connectors * remove $$type from middleware * Apply suggestions from code review Co-authored-by: François Chalifour <[email protected]> * fix: allow only one way for sending events * feat(insights): implement sendEvent in all the connectors (3/4) (#4463) * update infiniteHits * modify createSendEventForHits to accept index instead of helper * add sendEvent to connectAutocomplete * add sendEvent to connectGeoSearch * add sendEvent to connectHierarchicalMenu * add sendEvent to connectBreadcrumb * add sendEvent to connectMenu * add sendEvent to connectNumericMenu * extract getRefinedState in connectRange * add sendEvent to connectRange * add sendEvent to connectRatingMenu * add sendEvent to connectToggleRefinement * fix test error * fix export * fix type error * use $$type instead of hard-code * remove sendEvent from connectBreadcrumb * moved createSendEvent to the top level in the files * feat(insights): add tests (4/4) (#4464) * add tests for userToken * fix import paths * fix test cases to accept null instead of false as insightsClient * update bundlesize * fix lint errors * test sendEventToInsights * use $$type instead of hard-code * remove sendEvent from connectBreadcrumb * moved createSendEvent to the top level in the files * log insights event from storybook * add test for connectors * add tests for createSendEvent helpers * add integration tests for hits and infinite-hits widgets with bindEvent in templates * update bundlesize * Update src/connectors/hits/__tests__/connectHits-test.ts Co-authored-by: Haroen Viaene <[email protected]> * use factory function instead of globally exposed variables * clean up * update comment * use runAllMicroTasks instead of nextTick * fix: type errors * fix wrong import paths * fix: pass insightsClient to onEvent as the second parameter * update titles of describe blocks Co-authored-by: Haroen Viaene <[email protected]> Co-authored-by: Haroen Viaene <[email protected]> Co-authored-by: Haroen Viaene <[email protected]> Co-authored-by: François Chalifour <[email protected]>
eunjae-lee
pushed a commit
that referenced
this pull request
Sep 2, 2020
* feat: update userToken automatically * chore: fix bug where it gets token before rendering the custom widget * chore: add access modifier * chore: use custom widget and helper * Update src/lib/InstantSearch.ts Co-authored-by: Haroen Viaene <[email protected]> * chore: update token to helper directly * chore: move setupUserTokenUpdater to start() * chore: move to middleware * Update src/middleware/insights.ts Co-authored-by: Haroen Viaene <[email protected]> * export middleware * fix lint error * export for umd build * Update src/middleware/insights.ts Co-authored-by: François Chalifour <[email protected]> * update token automatically * chore: update error message * inline functions * initialize insightsClient earlier * add warning message if userToken is set before creating the middleware * accept `false` for `insightsClient` * clean up types * fix wrong import * Update src/middleware/insights.ts Co-authored-by: François Chalifour <[email protected]> * accept null as insightsClient * Update src/types/insights.ts Co-authored-by: Haroen Viaene <[email protected]> * add test for getAppIdAndApiKey * Update src/types/insights.ts Co-authored-by: Haroen Viaene <[email protected]> * bring back exports * rename middleware to middlewares * chore: rename files for better alignment * chore: export all types related to insights middleware * feat(insights): send events from hits and refinementList (2/4) (#4456) * chore: dummy commit * add sendEvent method * send event from refinementList * accept custom payload at sendEvent * delegate to onEvent or actually send event * store _insightsMiddleware in instantSearchInstance * add clickedFilters to InsightsClientMethod * rename $$type of middlewares * chore: remove ! * add sendEvent for hits * update for consistency * send view event when rendering hits * expose bindEvent to templates * handle insightsEvent from templates * allow single hit for bind/sendEventForHits * assign sendEventToInsights by the middleware * Update src/lib/insights/listener.tsx Co-authored-by: Haroen Viaene <[email protected]> * require eventName for click and conversion in hits * fix type errors * fix test cases * reuse $$type from connectors * remove $$type from middleware * Apply suggestions from code review Co-authored-by: François Chalifour <[email protected]> * fix: allow only one way for sending events * feat(insights): implement sendEvent in all the connectors (3/4) (#4463) * update infiniteHits * modify createSendEventForHits to accept index instead of helper * add sendEvent to connectAutocomplete * add sendEvent to connectGeoSearch * add sendEvent to connectHierarchicalMenu * add sendEvent to connectBreadcrumb * add sendEvent to connectMenu * add sendEvent to connectNumericMenu * extract getRefinedState in connectRange * add sendEvent to connectRange * add sendEvent to connectRatingMenu * add sendEvent to connectToggleRefinement * fix test error * fix export * fix type error * use $$type instead of hard-code * remove sendEvent from connectBreadcrumb * moved createSendEvent to the top level in the files * feat(insights): add tests (4/4) (#4464) * add tests for userToken * fix import paths * fix test cases to accept null instead of false as insightsClient * update bundlesize * fix lint errors * test sendEventToInsights * use $$type instead of hard-code * remove sendEvent from connectBreadcrumb * moved createSendEvent to the top level in the files * log insights event from storybook * add test for connectors * add tests for createSendEvent helpers * add integration tests for hits and infinite-hits widgets with bindEvent in templates * update bundlesize * Update src/connectors/hits/__tests__/connectHits-test.ts Co-authored-by: Haroen Viaene <[email protected]> * use factory function instead of globally exposed variables * clean up * update comment * use runAllMicroTasks instead of nextTick * fix: type errors * fix wrong import paths * fix: pass insightsClient to onEvent as the second parameter * update titles of describe blocks Co-authored-by: Haroen Viaene <[email protected]> Co-authored-by: Haroen Viaene <[email protected]> Co-authored-by: Haroen Viaene <[email protected]> Co-authored-by: François Chalifour <[email protected]> Co-authored-by: Haroen Viaene <[email protected]> Co-authored-by: François Chalifour <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This PR implements
sendEventin the following connectors and updates the existing test cases accordingly:Stacked PRs
insightsmiddlewareResult
Now the connectors send event when refining, and expose
sendEventatinitandrendermethod.