fix(insights): user-defined send/bindEvent overrides internal click #5527

Haroenv · 2023-03-03T16:21:35Z

in sendEvent, we mark internal events (click and conversion), and prevent them being sent if there was a custom click right before. We also move bindEvent so that it happens before sendEvent

This prevents automatic click when:

there's a bindEvent child
there's a sendEvent click before

This doesn't prevent when:

there's a sendEvent convert before
there's a view before
there's two internal clicks in a row

FX-2246

codesandbox-ci · 2023-03-03T16:25:07Z

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 1be6e7a:

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

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

…arent

packages/instantsearch.js/src/widgets/infinite-hits/__tests__/infinite-hits-integration-test.ts

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

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

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.

dhayab

Awesome ✨ , I just have 2 nitpicky notes, otherwise it's good for me!

dhayab · 2023-03-15T09:48:07Z

packages/instantsearch.js/src/components/Hits/Hits.tsx

-              onClick: () => {
-                sendEvent('click', hit, 'Hit Clicked');
+              onClick: (event: MouseEvent) => {
+                handleInsightsClick(event);


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

packages/instantsearch.js/src/lib/insights/listener.tsx

this prevents a custom blocking an internal in a different widget

packages/instantsearch.js/src/lib/utils/createSendEventForHits.ts

Haroenv · 2023-03-15T16:32:55Z

packages/vue-instantsearch/src/__tests__/common.test.js

+  fakeAct,
+  {
+    // Vue InstantSearch doesn't support modern insights with sendEvent + default events
+    insights: true,
+  }


solved by #5549

dhayab · 2023-03-15T17:35:50Z

tests/common/widgets/hits/insights.ts

This and the infinite hits insights test look really similar. Do you think we could go one step further and rely on a reusable test suite for both?

I tried it, but didn't get to a suitable shape for the tests. The common tests shape will become clearer when we have more of them I think

…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

Haroenv changed the title ~~fix(insights): user-defined click overrides~~ fix(insights): user-defined click overrides internal click Mar 3, 2023

Haroenv changed the title ~~fix(insights): user-defined click overrides internal click~~ fix(insights): user-defined send/bindEvent overrides internal click Mar 6, 2023

Haroenv mentioned this pull request Mar 8, 2023

refactor(bindEvent): move listener inside components #5528

Merged

Haroenv force-pushed the feat/automatic-insights branch from b4e9504 to 7f415a8 Compare March 8, 2023 16:00

Haroenv added 2 commits March 14, 2023 11:00

fix(insights): prevent duplicate events

16c096f

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 p…

35028d0

…arent

Haroenv force-pushed the fix/prevent-dupe-events-click branch from ecf5ec7 to 35028d0 Compare March 14, 2023 10:03

Haroenv added 3 commits March 14, 2023 11:19

test

d78effe

fix: only prevent next clicks after a regular click

0fba3f7

bundlesize

54ae54f

Haroenv marked this pull request as ready for review March 14, 2023 10:32

Haroenv requested review from sarahdayan and dhayab March 14, 2023 10:34

sarahdayan reviewed Mar 14, 2023

View reviewed changes

packages/instantsearch.js/src/widgets/infinite-hits/__tests__/infinite-hits-integration-test.ts Outdated Show resolved Hide resolved

packages/instantsearch.js/src/widgets/infinite-hits/__tests__/infinite-hits-integration-test.ts Show resolved Hide resolved

test(infiniteHits): add sendEvent tests

6ad9ebb

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

Haroenv requested a review from sarahdayan March 14, 2023 13:40

Haroenv added 3 commits March 14, 2023 14:59

wip infinite hits

44747c2

move bindEvent click listener

d2671bb

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

dhayab approved these changes Mar 15, 2023

View reviewed changes

sarahdayan reviewed Mar 15, 2023

View reviewed changes

packages/instantsearch.js/src/lib/insights/listener.tsx Outdated Show resolved Hide resolved

Haroenv added 3 commits March 15, 2023 11:35

fix(events): move back inside connector

c5180bf

this prevents a custom blocking an internal in a different widget

get rid of global cruft

bcd1155

refactor: use timers instead of order-based ignoring

81ceff4

sarahdayan reviewed Mar 15, 2023

View reviewed changes

packages/instantsearch.js/src/lib/utils/createSendEventForHits.ts Outdated Show resolved Hide resolved

Haroenv added 3 commits March 15, 2023 17:21

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

525e3ba

fix(react): send an internal event

5b1d7ed

PR feedback

cf1ee9a

Haroenv requested a review from sarahdayan March 15, 2023 16:28

Haroenv requested a review from dhayab March 15, 2023 16:29

Haroenv commented Mar 15, 2023

View reviewed changes

internal

1be6e7a

dhayab approved these changes Mar 15, 2023

View reviewed changes

Haroenv merged commit 94c385b into feat/automatic-insights Mar 16, 2023

Haroenv deleted the fix/prevent-dupe-events-click branch March 16, 2023 09:10

Haroenv mentioned this pull request Mar 16, 2023

feat(insights): add "insights" option for automatic events #5490

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(insights): user-defined send/bindEvent overrides internal click #5527

fix(insights): user-defined send/bindEvent overrides internal click #5527

Haroenv commented Mar 3, 2023 •

edited

Loading

codesandbox-ci bot commented Mar 3, 2023 •

edited

Loading

dhayab left a comment

dhayab Mar 15, 2023

Haroenv Mar 15, 2023

dhayab Mar 15, 2023

Haroenv Mar 16, 2023

fix(insights): user-defined send/bindEvent overrides internal click #5527

fix(insights): user-defined send/bindEvent overrides internal click #5527

Conversation

Haroenv commented Mar 3, 2023 • edited Loading

codesandbox-ci bot commented Mar 3, 2023 • edited Loading

dhayab left a comment

Choose a reason for hiding this comment

dhayab Mar 15, 2023

Choose a reason for hiding this comment

Haroenv Mar 15, 2023

Choose a reason for hiding this comment

dhayab Mar 15, 2023

Choose a reason for hiding this comment

Haroenv Mar 16, 2023

Choose a reason for hiding this comment

Haroenv commented Mar 3, 2023 •

edited

Loading

codesandbox-ci bot commented Mar 3, 2023 •

edited

Loading