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): only load site_destinations conditionally #1619

Closed
wants to merge 7 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions playground/nextjs/src/posthog.ts
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,10 @@ export const configForConsent = (): Partial<PostHogConfig> => {
export const updatePostHogConsent = (consentGiven: boolean) => {
if (consentGiven) {
localStorage.setItem('cookie_consent', 'true')
posthog.opt_in_capturing()
} else {
localStorage.removeItem('cookie_consent')
posthog.opt_out_capturing()
}

posthog.set_config(configForConsent())
Expand Down
91 changes: 61 additions & 30 deletions src/__tests__/site-apps.ts
Original file line number Diff line number Diff line change
@@ -1,13 +1,14 @@
import { mockLogger } from './helpers/mock-logger'

import { SiteApps } from '../site-apps'
import { PostHogPersistence } from '../posthog-persistence'
import { RequestRouter } from '../utils/request-router'
import { PostHog } from '../posthog-core'
import { PostHogConfig, Properties, CaptureResult, RemoteConfig } from '../types'
import { PostHogConfig, CaptureResult, RemoteConfig } from '../types'
import { assignableWindow } from '../utils/globals'
import '../entrypoints/external-scripts-loader'
import { isFunction } from '../utils/type-utils'
import { uuidv7 } from '../uuidv7'
import { logger } from '../utils/logger'
import { defaultPostHog } from './helpers/posthog-instance'

describe('SiteApps', () => {
let posthog: PostHog
Expand All @@ -21,6 +22,8 @@ describe('SiteApps', () => {
token: token,
api_host: 'https://test.com',
persistence: 'memory',
opt_in_site_apps: true,
opt_out_capturing_by_default: false,
}

beforeEach(() => {
Expand All @@ -44,33 +47,26 @@ describe('SiteApps', () => {

delete assignableWindow._POSTHOG_REMOTE_CONFIG
delete assignableWindow.POSTHOG_DEBUG
delete assignableWindow[`__$$ph_site_app_1`]
delete assignableWindow[`__$$ph_site_app_2`]

removeCaptureHook = jest.fn()

posthog = {
config: { ...defaultConfig, opt_in_site_apps: true },
persistence: new PostHogPersistence(defaultConfig as PostHogConfig),
register: (props: Properties) => posthog.persistence!.register(props),
unregister: (key: string) => posthog.persistence!.unregister(key),
get_property: (key: string) => posthog.persistence!.props[key],
capture: jest.fn(),
_addCaptureHook: jest.fn((cb) => {
emitCaptureEvent = cb
return removeCaptureHook
}),
_afterDecideResponse: jest.fn(),
get_distinct_id: jest.fn().mockImplementation(() => 'distinctid'),
_send_request: jest.fn().mockImplementation(({ callback }) => callback?.({ config: {} })),
featureFlags: {
receivedFeatureFlags: jest.fn(),
setReloadingPaused: jest.fn(),
_startReloadTimer: jest.fn(),
},
requestRouter: new RequestRouter({ config: defaultConfig } as unknown as PostHog),
_hasBootstrappedFeatureFlags: jest.fn(),
getGroups: () => ({ organization: '5' }),
on: jest.fn(),
} as unknown as PostHog
const createPostHog = (config: Partial<PostHogConfig> = {}) => {
const posthog = defaultPostHog().init('testtoken', { ...config }, uuidv7())!
posthog.debug()
return posthog
}

posthog = createPostHog(defaultConfig)
posthog._addCaptureHook = jest.fn((cb) => {
emitCaptureEvent = cb
return removeCaptureHook
})
posthog.on = jest.fn()
posthog.capture = jest.fn()
posthog._send_request = jest.fn().mockImplementation(({ callback }) => callback?.({ config: {} }))
logger.error = jest.fn()

siteAppsInstance = new SiteApps(posthog)
})
Expand Down Expand Up @@ -240,8 +236,8 @@ describe('SiteApps', () => {
posthog.config.opt_in_site_apps = false
siteAppsInstance.onRemoteConfig({
siteApps: [
{ id: '1', url: '/site_app/1' },
{ id: '2', url: '/site_app/2' },
{ id: '1', type: 'site_destination', url: '/site_app/1' },
{ id: '2', type: 'site_destination', url: '/site_app/2' },
],
} as RemoteConfig)

Expand All @@ -257,6 +253,7 @@ describe('SiteApps', () => {
siteApps: [
{
id: '1',
type: 'site_destination',
init: jest.fn(() => {
return {
processEvent: jest.fn(),
Expand All @@ -267,7 +264,7 @@ describe('SiteApps', () => {
},
} as any
siteAppsInstance.onRemoteConfig({
siteApps: [{ id: '1', url: '/site_app/1' }],
siteApps: [{ id: '1', type: 'site_destination', url: '/site_app/1' }],
} as RemoteConfig)

expect(assignableWindow.__PosthogExtensions__?.loadSiteApp).not.toHaveBeenCalled()
Expand Down Expand Up @@ -311,6 +308,7 @@ describe('SiteApps', () => {
siteApps: [
{
id: '1',
type: 'site_app',
init: jest.fn((config) => {
appConfigs.push(config)
return {
Expand All @@ -320,6 +318,7 @@ describe('SiteApps', () => {
},
{
id: '2',
type: 'site_destination',
init: jest.fn((config) => {
appConfigs.push(config)
return {
Expand Down Expand Up @@ -425,5 +424,37 @@ describe('SiteApps', () => {
)
expect(siteAppsInstance.apps).toEqual({})
})

it('does not load site destinations if consent is not given', () => {
posthog.opt_out_capturing()

siteAppsInstance.onRemoteConfig({} as RemoteConfig)

expect(appConfigs[0]).toBeDefined()
expect(appConfigs[1]).toBeUndefined()

expect(typeof assignableWindow['__$$ph_site_app_1']).toBe('object')
expect(typeof assignableWindow['__$$ph_site_app_2']).toBe('undefined')
})

it('load site destinations if consent is given at a later time', () => {
posthog.opt_out_capturing()

siteAppsInstance.onRemoteConfig({} as RemoteConfig)

expect(appConfigs[0]).toBeDefined()
expect(appConfigs[1]).toBeUndefined()

expect(typeof assignableWindow['__$$ph_site_app_1']).toBe('object')
expect(typeof assignableWindow['__$$ph_site_app_2']).toBe('undefined')

posthog.opt_in_capturing()

expect(appConfigs[0]).toBeDefined()
expect(appConfigs[1]).toBeDefined()

expect(typeof assignableWindow['__$$ph_site_app_1']).toBe('object')
expect(typeof assignableWindow['__$$ph_site_app_2']).toBe('object')
})
})
})
32 changes: 18 additions & 14 deletions src/posthog-core.ts
Original file line number Diff line number Diff line change
Expand Up @@ -312,6 +312,7 @@ export class PostHog {
this.scrollManager = new ScrollManager(this)
this.pageViewManager = new PageViewManager(this)
this.surveys = new PostHogSurveys(this)
this.siteApps = new SiteApps(this)
this.experiments = new WebExperiments(this)
this.exceptions = new PostHogExceptions(this)
this.rateLimiter = new RateLimiter(this)
Expand Down Expand Up @@ -440,8 +441,8 @@ export class PostHog {

new TracingHeaders(this).startIfEnabledOrStop()

this.siteApps = new SiteApps(this)
this.siteApps?.init()
this.siteApps?.loadIfEnabled()

if (!this.config.__preview_experimental_cookieless_mode) {
this.sessionRecording = new SessionRecording(this)
Expand Down Expand Up @@ -819,6 +820,19 @@ export class PostHog {
return
}

// The initial campaign/referrer props need to be stored in the regular persistence, as they are there to mimic
// the person-initial props. The non-initial versions are stored in the sessionPersistence, as they are sent
// with every event and used by the session table to create session-initial props.
if (this.config.store_google) {
this.sessionPersistence.update_campaign_params()
}
if (this.config.save_referrer) {
this.sessionPersistence.update_referrer_info()
}
if (this.config.store_google || this.config.save_referrer) {
this.persistence.set_initial_person_info()
}

if (this.consent.isOptedOut()) {
return
}
Expand All @@ -845,19 +859,6 @@ export class PostHog {
// update persistence
this.sessionPersistence.update_search_keyword()

// The initial campaign/referrer props need to be stored in the regular persistence, as they are there to mimic
// the person-initial props. The non-initial versions are stored in the sessionPersistence, as they are sent
// with every event and used by the session table to create session-initial props.
if (this.config.store_google) {
this.sessionPersistence.update_campaign_params()
}
if (this.config.save_referrer) {
this.sessionPersistence.update_referrer_info()
}
if (this.config.store_google || this.config.save_referrer) {
this.persistence.set_initial_person_info()
}

const systemTime = new Date()
const timestamp = options?.timestamp || systemTime

Expand Down Expand Up @@ -1855,6 +1856,7 @@ export class PostHog {
this.autocapture?.startIfEnabled()
this.heatmaps?.startIfEnabled()
this.surveys.loadIfEnabled()
this.siteApps?.loadIfEnabled()
this._sync_opt_out_with_persistence()
}
}
Expand Down Expand Up @@ -2123,6 +2125,8 @@ export class PostHog {
if (this.config.capture_pageview) {
this._captureInitialPageview()
}

this.set_config({})
}

/**
Expand Down
38 changes: 30 additions & 8 deletions src/site-apps.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,16 @@
import { PostHog } from './posthog-core'
import { CaptureResult, Properties, RemoteConfig, SiteApp, SiteAppGlobals, SiteAppLoader } from './types'
import {
CaptureResult,
DecideResponse,
Properties,
RemoteConfig,
SiteApp,
SiteAppGlobals,
SiteAppLoader,
} from './types'
import { assignableWindow } from './utils/globals'
import { createLogger } from './utils/logger'
import { isUndefined } from './utils/type-utils'

const logger = createLogger('[SiteApps]')

Expand All @@ -10,6 +19,7 @@ export class SiteApps {

private stopBuffering?: () => void
private bufferedInvocations: SiteAppGlobals[]
private _decideServerSiteAppsResponse?: DecideResponse['siteApps']

constructor(private instance: PostHog) {
// events captured between loading posthog-js and the site app; up to 1000 events
Expand Down Expand Up @@ -144,28 +154,29 @@ export class SiteApps {
}
}

onRemoteConfig(response: RemoteConfig): void {
loadIfEnabled(): void {
if (this.siteAppLoaders?.length) {
if (!this.isEnabled) {
logger.error(`PostHog site apps are disabled. Enable the "opt_in_site_apps" config to proceed.`)
return
}

for (const app of this.siteAppLoaders) {
// if consent isn't given, skip site destinations
if (this.instance.consent.isOptedOut() && app.type === 'site_destination') continue
// if the site app is already loaded, skip it
if (!isUndefined(assignableWindow[`__$$ph_site_app_${app.id}`])) continue
assignableWindow[`__$$ph_site_app_${app.id}`] = this.instance
this.setupSiteApp(app)
}

// NOTE: We could improve this to only fire if we actually have listeners for the event
this.instance.on('eventCaptured', (event) => this.onCapturedEvent(event))

return
}

// NOTE: Below his is now only the fallback for legacy site app support. Once we have fully removed to the remote config loader we can get rid of this

this.stopBuffering?.()

if (!response['siteApps']?.length) {
if (!this._decideServerSiteAppsResponse?.length) {
return
}

Expand All @@ -174,7 +185,11 @@ export class SiteApps {
return
}

for (const { id, url } of response['siteApps']) {
for (const { id, type, url } of this._decideServerSiteAppsResponse) {
// if consent isn't given, skip site destinations
if (this.instance.consent.isOptedOut() && type === 'site_destination') continue
// if the site app is already loaded, skip it
if (!isUndefined(assignableWindow[`__$$ph_site_app_${id}`])) continue
assignableWindow[`__$$ph_site_app_${id}`] = this.instance
assignableWindow.__PosthogExtensions__?.loadSiteApp?.(this.instance, url, (err) => {
if (err) {
Expand All @@ -183,4 +198,11 @@ export class SiteApps {
})
}
}

onRemoteConfig(response: RemoteConfig): void {
this._decideServerSiteAppsResponse = response['siteApps']
// NOTE: We could improve this to only fire if we actually have listeners for the event
if (this.siteAppLoaders?.length) this.instance.on('eventCaptured', (event) => this.onCapturedEvent(event))
this.loadIfEnabled()
}
}
3 changes: 2 additions & 1 deletion src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -549,7 +549,7 @@ export interface RemoteConfig {
editorParams?: ToolbarParams /** @deprecated, renamed to toolbarParams, still present on older API responses */
toolbarVersion: 'toolbar' /** @deprecated, moved to toolbarParams */
isAuthenticated: boolean
siteApps: { id: string; url: string }[]
siteApps: { id: string; type: string; url: string }[]
heatmaps?: boolean
defaultIdentifiedOnly?: boolean
captureDeadClicks?: boolean
Expand Down Expand Up @@ -579,6 +579,7 @@ export type SiteAppGlobals = {

export type SiteAppLoader = {
id: string
type: string
init: (config: { posthog: PostHog; callback: (success: boolean) => void }) => {
processEvent?: (globals: SiteAppGlobals) => void
}
Expand Down
Loading