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(cdp): allow disabling site destinations #1563

Closed
wants to merge 21 commits into from

Conversation

MarconLP
Copy link
Member

@MarconLP MarconLP commented Nov 27, 2024

Changes

  • Only load site_destination's if isOptedOut is false
  • site destinations will be loaded if .opt_in_capturing() is being called at a later point.
  • Updated the nextjs example to call opt_in_capturing & opt_out_capturing based on the cookie banner (link)

2024-11-27 at 15 18 40

TODOs:

  • add tests

Checklist

  • Tests for new code (see advice on the tests we use)
  • Accounted for the impact of any changes across different browsers
  • Accounted for backwards compatibility of any changes (no breaking changes in posthog-js!)

Copy link

vercel bot commented Nov 27, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
posthog-js ✅ Ready (Inspect) Visit Preview Dec 5, 2024 11:26pm

@MarconLP MarconLP changed the title feat(cdp): site apps handle consent #1555 feat(cdp): site apps handle consent Nov 27, 2024
Copy link

github-actions bot commented Nov 27, 2024

Size Change: +3.99 kB (+0.13%)

Total Size: 3.19 MB

Filename Size Change
dist/array.full.es5.js 258 kB +418 B (+0.16%)
dist/array.full.js 361 kB +398 B (+0.11%)
dist/array.full.no-external.js 360 kB +398 B (+0.11%)
dist/array.js 176 kB +396 B (+0.23%)
dist/array.no-external.js 175 kB +396 B (+0.23%)
dist/main.js 176 kB +396 B (+0.22%)
dist/module.full.js 362 kB +398 B (+0.11%)
dist/module.full.no-external.js 361 kB +398 B (+0.11%)
dist/module.js 176 kB +396 B (+0.23%)
dist/module.no-external.js 175 kB +396 B (+0.23%)
ℹ️ View Unchanged
Filename Size
dist/all-external-dependencies.js 206 kB
dist/customizations.full.js 12.5 kB
dist/dead-clicks-autocapture.js 14.3 kB
dist/exception-autocapture.js 9.37 kB
dist/external-scripts-loader.js 2.29 kB
dist/recorder-v2.js 115 kB
dist/recorder.js 115 kB
dist/surveys-preview.js 57.6 kB
dist/surveys.js 63.2 kB
dist/tracing-headers.js 1.75 kB
dist/web-vitals.js 10.3 kB

compressed-size-action

@MarconLP MarconLP changed the title feat(cdp): site apps handle consent feat(cdp): allow disabling site destinations Nov 27, 2024
playground/nextjs/src/posthog.ts Outdated Show resolved Hide resolved
src/__tests__/site-apps.ts Show resolved Hide resolved
src/__tests__/site-apps.ts Outdated Show resolved Hide resolved
src/site-apps.ts Outdated Show resolved Hide resolved
src/site-apps.ts Outdated
}
}

// TODO: opting out of stuff should disable this
afterDecideResponse(response: DecideResponse): void {
this._decideServerResponse = response['siteApps']
Copy link
Collaborator

Choose a reason for hiding this comment

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

The variable _decideServerResponse makes me think this contains the whole response, not just the siteApps key

Copy link
Member Author

Choose a reason for hiding this comment

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

renamed to "_decideServerSiteAppsResponse"

@mariusandra
Copy link
Collaborator

So... I finally had a look, and I think we're most of the way there.

One more thing: there's a chance if you don't opt in, that the event collector will collect forever. Is there ever a "definitely opted out" event to hook into, which would stop it?

@MarconLP
Copy link
Member Author

One more thing: there's a chance if you don't opt in, that the event collector will collect forever. Is there ever a "definitely opted out" event to hook into, which would stop it?

That is not a thing. It's either user is opted in/out by default and consent has been toggled.

Currently, we'll clear the missedInvocations queue after we've loaded all of the site_app's and set loaded to true (this will include cases where consent wasn't given and site_destination's haven't been loaded yet), which will disable the eventCollector.

We could reuse the missedInvocations queue to only clear after all site_destination's have been loaded and consentIsGiven, since it's already configured to only hold 1000 events at a time?

if (this.missedInvocations.length > 1000) {
    this.missedInvocations = this.missedInvocations.slice(10)
}

@mariusandra
Copy link
Collaborator

Wait... so what happens if:

  • User clicks on an ad and lands on a site
  • Waits 30seconds before clicking "agree to cookies and stuff" (all JS loads within the first 5sec)
  • what happens now?

They way I understand the last comment, is that they won't count as a converted user (we drop all events captured, as when site apps finished loading, we didn't have consent yet). Is that how it works, or did I misunderstand something again? If that is how it works though, it's definitely far from ideal and needs fixing.

…sedInvocations after site_destinations have been loaded
…lear missedInvocations after site_destinations have been loaded"

This reverts commit 19d0a7a.
@benjackwhite
Copy link
Collaborator

Sorry to be a pain but I will really want to get this in first

@MarconLP
Copy link
Member Author

MarconLP commented Dec 6, 2024

I updated the code to save initial campaign/referrer props parameters to this.storage.

  1. persistence: 'memory' & isOptedOut: true
  2. User visits posthog.com?gclid=ad_click_1
    • We won't capture a $pageview event, but we'll store gclid: 'ad_click_1' in memory
  3. User visits the pricing page
  4. User accepts the cookie banner (posthog.opt_in_capturing() & posthog.set_config({ persistence: 'localStorage+cookie' }))
    • We'll send an initial $pageview event and since the gclid is stored in this.storage we'll append that as an event property. Will also be included as an $initial_ person property.
  5. User visits the signup page - we capture a $pageview event
    • This event will be captured normally

If the user reloads the page between step 2 - 4 we'll lose the gclid property.

@posthog-bot
Copy link
Collaborator

This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the stale label – otherwise this will be closed in another week.

Copy link
Collaborator

@benjackwhite benjackwhite left a comment

Choose a reason for hiding this comment

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

This likely needs a rebase on top of the current changes

@posthog-bot posthog-bot removed the stale label Dec 17, 2024
@MarconLP
Copy link
Member Author

closing in favor of #1619

@MarconLP MarconLP closed this Dec 18, 2024
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