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): add "insights" option for automatic events #5490

Merged
merged 22 commits into from
Apr 24, 2023

Conversation

Haroenv
Copy link
Contributor

@Haroenv Haroenv commented Feb 21, 2023

Note: this pull request must be rebased, not squashed 🚨

@codesandbox-ci
Copy link

codesandbox-ci bot commented Feb 21, 2023

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 fd74f9e:

Sandbox Source
InstantSearch.js Configuration
react-instantsearch-app Configuration
example-react-instantsearch-hooks-default-theme Configuration
example-vue-instantsearch-default-theme Configuration

@Haroenv Haroenv changed the title [insights]: automatically load insights and middleware feat(insights): automatic insights Feb 21, 2023
@Haroenv Haroenv force-pushed the feat/automatic-insights branch 3 times, most recently from b4e9504 to 7f415a8 Compare March 8, 2023 16:00
@Haroenv Haroenv mentioned this pull request Apr 6, 2023
Haroenv and others added 18 commits April 18, 2023 17:08
* feat(insights): automatically load search-insights if not passed

FX-2244

* different wording

* fix(insights): notify of error when insights fails to load

* prettier snippet
To better prepare for the insights middleware being automatically added, we are making the following changes:

- useCookie by default true so userToken does not need to be provided
- still call `insightsClient` if no userToken is found (userToken can be applied using useCookie, thus not known)
- error on unknown algolia credentials converted to warning
FX-2250 

Co-authored-by: Sarah Dayan <5370675+sarahdayan@users.noreply.github.com>
This makes sure when the same userToken is set multiple times, it doesn't cause multiple search requests, as could be the case if something would call setUserToken without condition.

Discovered in #5500, but should be merged separately.

Co-authored-by: Fabien Motte <fabien.motte@algolia.com>
* refactor(bindEvent): move listener inside components

This was needlessly abstracted with a HOC, not allowing us to use this same function inside onClick of an individual hit, which could help with deduplicating sendEvent + bindEvent

I didn't remove the `withInsightsListener` HOC, as it could (in theory) be used externally.

* remove unused data
…5527)

* fix(insights): prevent duplicate events

in sendEvent, we mark internal events (click and conversion), and prevent them being sent if there was a custom click right before.

Note that this doesn't yet work with bindEvent, as it's higher in the bubbling tree

* fix(internal): bail out of default click if element has a bindEvent parent

* test

* fix: only prevent next clicks after a regular click

* bundlesize

* test(infiniteHits): add sendEvent tests

same idea as this test in hits-integration-test.ts

* wip infinite hits

* move bindEvent click listener

people can't bind events higher anyway, so this is safe to do

* refactor: move dedupe into middleware

this allows the dedupe to work for both bindEvent and sendEvent in a single codebase.

Tiny aside: this means that the "next internal click" is now global, meaning if you click on geo, then on hits, we'll see that as an internal click to prevent sending. This is an extreme edge case, as almost nowhere sends click events.

* fix(events): move back inside connector

this prevents a custom blocking an internal in a different widget

* get rid of global cruft

* refactor: use timers instead of order-based ignoring

* test(common): insights clicks + view for hits + infinitehits

* fix(react): send an internal event

* PR feedback

* internal
…vent to slot (#5549)

* feat(hits/infinite-hits): send a default click event and expose sendEvent to slot

it's like FX-2271, but for Vue

* fix(vue3): improve renderCompat

- scopedSlots -> children
- on.method -> onMethod
This allows passing all insights middleware options to the instantsearch constructor as well, simplifying the guide on "how to use insights", even if we choose to change the default of the insights option to `false`
* fix(insights): keep track of script loading in global

This is needed for example in StrictMode with React InstantSearch Hooks, where there are two InstantSearch instances, but the first one never gets started.

Before this PR, the "should load script" was linked to "should add window.aa", but that doesn't necessarily correspond if `start` is never called on that instance.

* tests

* whoops!

* whoops

* updates
)

* fix(insights): send extra parameters only for applicable versions

still to do: update tests

* test: impact of requiring version on tests

* move type
* fix(insights): only load script if we write window.aa

* Update packages/instantsearch.js/src/middlewares/__tests__/createInsightsMiddleware.ts

* Update packages/instantsearch.js/src/middlewares/createInsightsMiddleware.ts

* fix lint
* feat(insights): optin to insights option

FX-2300

* tests: update assertions
* feat(insights): annotate events with algoliaSource

- algoliaSource is an array
- default it always has "instantsearch"
- if it's an "internal" event, it also has "instantsearch-internal"

To be able to make this work, all internal events were changed to "xxx:internal", meaning the "skip click if two internal ones in a row" feature in createSendEventForHits was refined to click only (view for example will be sent multiple times in a tick if >20 hitsPerPage)

FX-2287

* chore: bundlesize
Co-authored-by: Dhaya <154633+dhayab@users.noreply.github.com>
@Haroenv Haroenv force-pushed the feat/automatic-insights branch from e489236 to fa98e21 Compare April 18, 2023 15:16
@Haroenv Haroenv changed the title feat(insights): automatic insights feat(insights): add "insights" option for automatic events Apr 21, 2023
Haroenv added a commit to algolia/doc-code-samples that referenced this pull request Apr 21, 2023
Not done for React Native, nor React InstantSearch Core-based examples.

FX-2317

Still to do:
- [ ] update instantsearch version on release of algolia/instantsearch#5490
- [ ] update autocomplete on release of algolia/autocomplete#1118
@Haroenv Haroenv marked this pull request as ready for review April 21, 2023 15:14
Only examples not touched are react-instantsearch-core based, or react native.
Copy link
Member

@dhayab dhayab left a comment

Choose a reason for hiding this comment

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

🚀

* refactor(sendEventForHits): remove indirection

At some point earlier there was more information than "payloads", but that was refactored out, meaning that the payloads indirection can be removed

* fix test
Copy link
Contributor Author

@Haroenv Haroenv left a comment

Choose a reason for hiding this comment

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

can't approve, but I've checked every file and it seems good to be released

@Haroenv Haroenv merged commit b279395 into master Apr 24, 2023
@Haroenv Haroenv deleted the feat/automatic-insights branch April 24, 2023 09:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants