From f32896d5fa1a8c97703b0d7e0ff97d5a58e8a4cf Mon Sep 17 00:00:00 2001 From: MarconLP <13001502+MarconLP@users.noreply.github.com> Date: Wed, 27 Nov 2024 12:09:01 +0100 Subject: [PATCH 01/21] rebase --- playground/nextjs/src/posthog.ts | 2 ++ src/posthog-core.ts | 4 +++- src/site-apps.ts | 39 ++++++++++++++++++++------------ src/types.ts | 3 ++- 4 files changed, 31 insertions(+), 17 deletions(-) diff --git a/playground/nextjs/src/posthog.ts b/playground/nextjs/src/posthog.ts index 6a4062a34..342904b9a 100644 --- a/playground/nextjs/src/posthog.ts +++ b/playground/nextjs/src/posthog.ts @@ -28,6 +28,7 @@ export const configForConsent = (): Partial => { return { persistence: consentGiven ? 'localStorage+cookie' : 'memory', disable_surveys: !consentGiven, + disable_site_apps_destinations: !consentGiven, autocapture: consentGiven, disable_session_recording: !consentGiven, } @@ -49,6 +50,7 @@ if (typeof window !== 'undefined') { session_recording: { recordCrossOriginIframes: true, }, + opt_in_site_apps: true, debug: true, disable_web_experiments: false, scroll_root_selector: ['#scroll_element', 'html'], diff --git a/src/posthog-core.ts b/src/posthog-core.ts index 49e21a25f..1e2617f63 100644 --- a/src/posthog-core.ts +++ b/src/posthog-core.ts @@ -145,6 +145,7 @@ export const defaultConfig = (): PostHogConfig => ({ disable_persistence: false, disable_web_experiments: true, // disabled in beta. disable_surveys: false, + disable_site_apps_destinations: false, enable_recording_console_log: undefined, // When undefined, it falls back to the server-side setting secure_cookie: window?.location?.protocol === 'https:', ip: true, @@ -308,6 +309,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) @@ -434,7 +436,6 @@ export class PostHog { new TracingHeaders(this).startIfEnabledOrStop() - this.siteApps = new SiteApps(this) this.siteApps?.init() this.sessionRecording = new SessionRecording(this) @@ -1828,6 +1829,7 @@ export class PostHog { this.autocapture?.startIfEnabled() this.heatmaps?.startIfEnabled() this.surveys.loadIfEnabled() + this.siteApps?.loadIfEnabled() this._sync_opt_out_with_persistence() } } diff --git a/src/site-apps.ts b/src/site-apps.ts index 73414ace1..394cccf65 100644 --- a/src/site-apps.ts +++ b/src/site-apps.ts @@ -5,16 +5,12 @@ import { logger } from './utils/logger' import { isArray } from './utils/type-utils' export class SiteApps { - instance: PostHog - enabled: boolean + private _decideServerResponse?: DecideResponse['siteApps'] missedInvocations: Record[] loaded: boolean appsLoading: Set - constructor(instance: PostHog) { - this.instance = instance - // can't use if site apps are disabled, or if we're not asking /decide for site apps - this.enabled = !!this.instance.config.opt_in_site_apps && !this.instance.config.advanced_disable_decide + constructor(private readonly instance: PostHog) { // events captured between loading posthog-js and the site app; up to 1000 events this.missedInvocations = [] // capture events until loaded @@ -23,7 +19,9 @@ export class SiteApps { } eventCollector(_eventName: string, eventPayload?: CaptureResult | undefined) { - if (!this.enabled) { + // can't use if site apps are disabled, or if we're not asking /decide for site apps + const enabled = this.instance.config.opt_in_site_apps && !this.instance.config.advanced_disable_decide + if (!enabled) { return } if (!this.loaded && eventPayload) { @@ -74,9 +72,15 @@ export class SiteApps { return globals } - afterDecideResponse(response?: DecideResponse): void { - if (isArray(response?.siteApps) && response.siteApps.length > 0) { - if (this.enabled && this.instance.config.opt_in_site_apps) { + loadIfEnabled() { + if ( + this._decideServerResponse && + isArray(this._decideServerResponse) && + this._decideServerResponse.length > 0 + ) { + // can't use if site apps are disabled, or if we're not asking /decide for site apps + const enabled = this.instance.config.opt_in_site_apps && !this.instance.config.advanced_disable_decide + if (enabled) { const checkIfAllLoaded = () => { // Stop collecting events once all site apps are loaded if (this.appsLoading.size === 0) { @@ -84,8 +88,11 @@ export class SiteApps { this.missedInvocations = [] } } - for (const { id, url } of response['siteApps']) { - // TODO: if we have opted out and "type" is "site_destination", ignore it... but do include "site_app" types + for (const { id, type, url } of this._decideServerResponse) { + if (this.instance.config.disable_site_apps_destinations && type === 'site_destination') continue + // if the site app is already loaded, skip it + // eslint-disable-next-line no-restricted-globals + if (`__$$ph_site_app_${id}_posthog` in window) continue this.appsLoading.add(id) assignableWindow[`__$$ph_site_app_${id}_posthog`] = this.instance assignableWindow[`__$$ph_site_app_${id}_missed_invocations`] = () => this.missedInvocations @@ -101,7 +108,7 @@ export class SiteApps { } }) } - } else if (response['siteApps'].length > 0) { + } else if (this._decideServerResponse.length > 0) { logger.error('PostHog site apps are disabled. Enable the "opt_in_site_apps" config to proceed.') this.loaded = true } else { @@ -109,9 +116,11 @@ export class SiteApps { } } else { this.loaded = true - this.enabled = false } } - // TODO: opting out of stuff should disable this + afterDecideResponse(response: DecideResponse): void { + this._decideServerResponse = response['siteApps'] + this.loadIfEnabled() + } } diff --git a/src/types.ts b/src/types.ts index 3044e10df..24a379791 100644 --- a/src/types.ts +++ b/src/types.ts @@ -248,6 +248,7 @@ export interface PostHogConfig { /** @deprecated - use `disable_persistence` instead */ disable_cookie?: boolean disable_surveys: boolean + disable_site_apps_destinations: boolean disable_web_experiments: boolean /** If set, posthog-js will never load external scripts such as those needed for Session Replay or Surveys. */ disable_external_dependency_loading?: boolean @@ -523,7 +524,7 @@ export interface DecideResponse { 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 From e8cffbf7661fe42ecf787773f42eb875610f83c7 Mon Sep 17 00:00:00 2001 From: MarconLP <13001502+MarconLP@users.noreply.github.com> Date: Wed, 27 Nov 2024 13:23:03 +0100 Subject: [PATCH 02/21] remove disable site destination properties and depend on isOptedOut --- playground/nextjs/src/posthog.ts | 3 ++- src/posthog-core.ts | 1 - src/site-apps.ts | 2 +- src/types.ts | 1 - 4 files changed, 3 insertions(+), 4 deletions(-) diff --git a/playground/nextjs/src/posthog.ts b/playground/nextjs/src/posthog.ts index 342904b9a..eb1882589 100644 --- a/playground/nextjs/src/posthog.ts +++ b/playground/nextjs/src/posthog.ts @@ -28,7 +28,6 @@ export const configForConsent = (): Partial => { return { persistence: consentGiven ? 'localStorage+cookie' : 'memory', disable_surveys: !consentGiven, - disable_site_apps_destinations: !consentGiven, autocapture: consentGiven, disable_session_recording: !consentGiven, } @@ -37,8 +36,10 @@ export const configForConsent = (): Partial => { 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()) diff --git a/src/posthog-core.ts b/src/posthog-core.ts index 1e2617f63..6f6aed21d 100644 --- a/src/posthog-core.ts +++ b/src/posthog-core.ts @@ -145,7 +145,6 @@ export const defaultConfig = (): PostHogConfig => ({ disable_persistence: false, disable_web_experiments: true, // disabled in beta. disable_surveys: false, - disable_site_apps_destinations: false, enable_recording_console_log: undefined, // When undefined, it falls back to the server-side setting secure_cookie: window?.location?.protocol === 'https:', ip: true, diff --git a/src/site-apps.ts b/src/site-apps.ts index 394cccf65..43cd3fd49 100644 --- a/src/site-apps.ts +++ b/src/site-apps.ts @@ -89,7 +89,7 @@ export class SiteApps { } } for (const { id, type, url } of this._decideServerResponse) { - if (this.instance.config.disable_site_apps_destinations && type === 'site_destination') continue + if (this.instance.consent.isOptedOut() && type === 'site_destination') continue // if the site app is already loaded, skip it // eslint-disable-next-line no-restricted-globals if (`__$$ph_site_app_${id}_posthog` in window) continue diff --git a/src/types.ts b/src/types.ts index 24a379791..a6cb9f7d4 100644 --- a/src/types.ts +++ b/src/types.ts @@ -248,7 +248,6 @@ export interface PostHogConfig { /** @deprecated - use `disable_persistence` instead */ disable_cookie?: boolean disable_surveys: boolean - disable_site_apps_destinations: boolean disable_web_experiments: boolean /** If set, posthog-js will never load external scripts such as those needed for Session Replay or Surveys. */ disable_external_dependency_loading?: boolean From 88a95a2ddd8a59ca89f191d6a84c0080db91e423 Mon Sep 17 00:00:00 2001 From: MarconLP <13001502+MarconLP@users.noreply.github.com> Date: Wed, 27 Nov 2024 13:32:08 +0100 Subject: [PATCH 03/21] use assignableWindow instead --- src/site-apps.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/site-apps.ts b/src/site-apps.ts index 43cd3fd49..46695b66b 100644 --- a/src/site-apps.ts +++ b/src/site-apps.ts @@ -2,7 +2,7 @@ import { PostHog } from './posthog-core' import { CaptureResult, DecideResponse } from './types' import { assignableWindow } from './utils/globals' import { logger } from './utils/logger' -import { isArray } from './utils/type-utils' +import { isArray, isUndefined } from './utils/type-utils' export class SiteApps { private _decideServerResponse?: DecideResponse['siteApps'] @@ -89,10 +89,10 @@ export class SiteApps { } } for (const { id, type, url } of this._decideServerResponse) { + // 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 - // eslint-disable-next-line no-restricted-globals - if (`__$$ph_site_app_${id}_posthog` in window) continue + if (!isUndefined(assignableWindow[`__$$ph_site_app_${id}_posthog`])) continue this.appsLoading.add(id) assignableWindow[`__$$ph_site_app_${id}_posthog`] = this.instance assignableWindow[`__$$ph_site_app_${id}_missed_invocations`] = () => this.missedInvocations From b07184f38872e9acb98b7ef2e3cc71229f31f2ad Mon Sep 17 00:00:00 2001 From: MarconLP <13001502+MarconLP@users.noreply.github.com> Date: Wed, 27 Nov 2024 14:00:58 +0100 Subject: [PATCH 04/21] expose posthog on window.posthog --- playground/nextjs/src/posthog.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/playground/nextjs/src/posthog.ts b/playground/nextjs/src/posthog.ts index eb1882589..f3b24104f 100644 --- a/playground/nextjs/src/posthog.ts +++ b/playground/nextjs/src/posthog.ts @@ -60,7 +60,7 @@ if (typeof window !== 'undefined') { persistence_name: `${process.env.NEXT_PUBLIC_POSTHOG_KEY}_nextjs`, ...configForConsent(), }) - + ;(window as any).posthog = posthog // Help with debugging(window as any).posthog = posthog } From 5fdf633e342cb8b6938900da42f838bb6dbb8a80 Mon Sep 17 00:00:00 2001 From: MarconLP <13001502+MarconLP@users.noreply.github.com> Date: Wed, 27 Nov 2024 15:12:24 +0100 Subject: [PATCH 05/21] update tests --- src/__tests__/site-apps.ts | 105 +++++++++++++++---------------------- src/posthog-core.ts | 2 +- src/site-apps.ts | 12 +++-- 3 files changed, 53 insertions(+), 66 deletions(-) diff --git a/src/__tests__/site-apps.ts b/src/__tests__/site-apps.ts index 6c670eeb4..1aec6b1e1 100644 --- a/src/__tests__/site-apps.ts +++ b/src/__tests__/site-apps.ts @@ -7,6 +7,7 @@ import { assignableWindow } from '../utils/globals' import { logger } from '../utils/logger' import '../entrypoints/external-scripts-loader' import { isFunction } from '../utils/type-utils' +import { ConsentManager } from '../consent' jest.mock('../utils/logger', () => ({ logger: { @@ -50,6 +51,11 @@ describe('SiteApps', () => { register: (props: Properties) => posthog.persistence!.register(props), unregister: (key: string) => posthog.persistence!.unregister(key), get_property: (key: string) => posthog.persistence!.props[key], + consent: { + isOptedOut(): boolean { + return false + }, + } as unknown as ConsentManager, capture: jest.fn(), _addCaptureHook: jest.fn(), _afterDecideResponse: jest.fn(), @@ -73,42 +79,6 @@ describe('SiteApps', () => { }) describe('constructor', () => { - it('sets enabled to true when opt_in_site_apps is true and advanced_disable_decide is false', () => { - posthog.config = { - ...defaultConfig, - opt_in_site_apps: true, - advanced_disable_decide: false, - } as PostHogConfig - - siteAppsInstance = new SiteApps(posthog) - - expect(siteAppsInstance.enabled).toBe(true) - }) - - it('sets enabled to false when opt_in_site_apps is false', () => { - posthog.config = { - ...defaultConfig, - opt_in_site_apps: false, - advanced_disable_decide: false, - } as PostHogConfig - - siteAppsInstance = new SiteApps(posthog) - - expect(siteAppsInstance.enabled).toBe(false) - }) - - it('sets enabled to false when advanced_disable_decide is true', () => { - posthog.config = { - ...defaultConfig, - opt_in_site_apps: true, - advanced_disable_decide: true, - } as PostHogConfig - - siteAppsInstance = new SiteApps(posthog) - - expect(siteAppsInstance.enabled).toBe(false) - }) - it('initializes missedInvocations, loaded, appsLoading correctly', () => { expect(siteAppsInstance.missedInvocations).toEqual([]) expect(siteAppsInstance.loaded).toBe(false) @@ -126,17 +96,17 @@ describe('SiteApps', () => { describe('eventCollector', () => { it('does nothing if enabled is false', () => { - siteAppsInstance.enabled = false + posthog.config.opt_in_site_apps = false siteAppsInstance.eventCollector('event_name', {} as CaptureResult) expect(siteAppsInstance.missedInvocations.length).toBe(0) }) it('collects event if enabled and loaded is false', () => { - siteAppsInstance.enabled = true + posthog.config.opt_in_site_apps = true siteAppsInstance.loaded = false - const eventPayload = { event: 'test_event', properties: { prop1: 'value1' } } as CaptureResult + const eventPayload = { event: 'test_event', properties: { prop1: 'value1' } } as unknown as CaptureResult jest.spyOn(siteAppsInstance, 'globalsForEvent').mockReturnValue({ some: 'globals' }) @@ -147,12 +117,12 @@ describe('SiteApps', () => { }) it('trims missedInvocations to last 990 when exceeding 1000', () => { - siteAppsInstance.enabled = true + posthog.config.opt_in_site_apps = true siteAppsInstance.loaded = false siteAppsInstance.missedInvocations = new Array(1000).fill({}) - const eventPayload = { event: 'test_event', properties: { prop1: 'value1' } } as CaptureResult + const eventPayload = { event: 'test_event', properties: { prop1: 'value1' } } as unknown as CaptureResult jest.spyOn(siteAppsInstance, 'globalsForEvent').mockReturnValue({ some: 'globals' }) @@ -223,20 +193,22 @@ describe('SiteApps', () => { }) describe('afterDecideResponse', () => { - it('sets loaded to true and enabled to false when response is undefined', () => { - siteAppsInstance.afterDecideResponse(undefined) + it('sets loaded to true response is undefined', () => { + const response = { + siteApps: [], + } as DecideResponse + siteAppsInstance.afterDecideResponse(response) expect(siteAppsInstance.loaded).toBe(true) - expect(siteAppsInstance.enabled).toBe(false) + expect(siteAppsInstance._decideServerResponse.length).toBe(0) }) it('loads site apps when enabled and opt_in_site_apps is true', (done) => { posthog.config.opt_in_site_apps = true - siteAppsInstance.enabled = true const response = { siteApps: [ - { id: '1', url: '/site_app/1' }, - { id: '2', url: '/site_app/2' }, + { id: '1', type: 'site_app', url: '/site_app/1' }, + { id: '2', type: 'site_app', url: '/site_app/2' }, ], } as DecideResponse @@ -255,25 +227,22 @@ describe('SiteApps', () => { }) it('does not load site apps when enabled is false', () => { - siteAppsInstance.enabled = false posthog.config.opt_in_site_apps = false const response = { - siteApps: [{ id: '1', url: '/site_app/1' }], + siteApps: [{ id: '1', type: 'site_app', url: '/site_app/1' }], } as DecideResponse siteAppsInstance.afterDecideResponse(response) expect(siteAppsInstance.loaded).toBe(true) - expect(siteAppsInstance.enabled).toBe(false) expect(assignableWindow.__PosthogExtensions__?.loadSiteApp).not.toHaveBeenCalled() }) it('clears missedInvocations when all apps are loaded', (done) => { posthog.config.opt_in_site_apps = true - siteAppsInstance.enabled = true siteAppsInstance.missedInvocations = [{ some: 'data' }] const response = { - siteApps: [{ id: '1', url: '/site_app/1' }], + siteApps: [{ id: '1', type: 'site_app', url: '/site_app/1' }], } as DecideResponse siteAppsInstance.afterDecideResponse(response) @@ -288,28 +257,26 @@ describe('SiteApps', () => { it('sets assignableWindow properties for each site app', () => { posthog.config.opt_in_site_apps = true - siteAppsInstance.enabled = true const response = { - siteApps: [{ id: '1', url: '/site_app/1' }], + siteApps: [{ id: '8', type: 'site_app', url: '/site_app/8' }], } as DecideResponse siteAppsInstance.afterDecideResponse(response) - expect(assignableWindow['__$$ph_site_app_1_posthog']).toBe(posthog) - expect(typeof assignableWindow['__$$ph_site_app_1_missed_invocations']).toBe('function') - expect(typeof assignableWindow['__$$ph_site_app_1_callback']).toBe('function') + expect(assignableWindow['__$$ph_site_app_8_posthog']).toBe(posthog) + expect(typeof assignableWindow['__$$ph_site_app_8_missed_invocations']).toBe('function') + expect(typeof assignableWindow['__$$ph_site_app_8_callback']).toBe('function') expect(assignableWindow.__PosthogExtensions__?.loadSiteApp).toHaveBeenCalledWith( posthog, - '/site_app/1', + '/site_app/8', expect.any(Function) ) }) it('logs error if site apps are disabled but response contains site apps', () => { posthog.config.opt_in_site_apps = false - siteAppsInstance.enabled = false const response = { - siteApps: [{ id: '1', url: '/site_app/1' }], + siteApps: [{ id: '1', type: 'site_app', url: '/site_app/1' }], } as DecideResponse siteAppsInstance.afterDecideResponse(response) @@ -321,7 +288,6 @@ describe('SiteApps', () => { }) it('sets loaded to true if response.siteApps is empty', () => { - siteAppsInstance.enabled = true posthog.config.opt_in_site_apps = true const response = { siteApps: [], @@ -330,7 +296,22 @@ describe('SiteApps', () => { siteAppsInstance.afterDecideResponse(response) expect(siteAppsInstance.loaded).toBe(true) - expect(siteAppsInstance.enabled).toBe(false) + }) + + it('does not load site destinations if consent is not given', () => { + posthog.config.opt_in_site_apps = true + posthog.consent.isOptedOut = () => true + const response = { + siteApps: [ + { id: '5', type: 'site_app', url: '/site_app/5' }, + { id: '6', type: 'site_destination', url: '/site_app/6' }, + ], + } as DecideResponse + + siteAppsInstance.afterDecideResponse(response) + + expect(typeof assignableWindow['__$$ph_site_app_5_callback']).toBe('function') + expect(typeof assignableWindow['__$$ph_site_app_6_callback']).toBe('undefined') }) }) }) diff --git a/src/posthog-core.ts b/src/posthog-core.ts index 6f6aed21d..38c437a25 100644 --- a/src/posthog-core.ts +++ b/src/posthog-core.ts @@ -1828,7 +1828,7 @@ export class PostHog { this.autocapture?.startIfEnabled() this.heatmaps?.startIfEnabled() this.surveys.loadIfEnabled() - this.siteApps?.loadIfEnabled() + // this.siteApps?.loadIfEnabled() this._sync_opt_out_with_persistence() } } diff --git a/src/site-apps.ts b/src/site-apps.ts index 46695b66b..975c1a5fb 100644 --- a/src/site-apps.ts +++ b/src/site-apps.ts @@ -5,7 +5,7 @@ import { logger } from './utils/logger' import { isArray, isUndefined } from './utils/type-utils' export class SiteApps { - private _decideServerResponse?: DecideResponse['siteApps'] + _decideServerResponse?: DecideResponse['siteApps'] missedInvocations: Record[] loaded: boolean appsLoading: Set @@ -90,9 +90,15 @@ export class SiteApps { } for (const { id, type, url } of this._decideServerResponse) { // if consent isn't given, skip site destinations - if (this.instance.consent.isOptedOut() && type === 'site_destination') continue + if (this.instance.consent.isOptedOut() && type === 'site_destination') { + checkIfAllLoaded() + continue + } // if the site app is already loaded, skip it - if (!isUndefined(assignableWindow[`__$$ph_site_app_${id}_posthog`])) continue + if (!isUndefined(assignableWindow[`__$$ph_site_app_${id}_posthog`])) { + checkIfAllLoaded() + continue + } this.appsLoading.add(id) assignableWindow[`__$$ph_site_app_${id}_posthog`] = this.instance assignableWindow[`__$$ph_site_app_${id}_missed_invocations`] = () => this.missedInvocations From 1a13c12007770220452bc737fd698f0343b0568b Mon Sep 17 00:00:00 2001 From: MarconLP <13001502+MarconLP@users.noreply.github.com> Date: Wed, 27 Nov 2024 15:15:42 +0100 Subject: [PATCH 06/21] uncomment loadIfEnabled in the set_config callback --- src/posthog-core.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/posthog-core.ts b/src/posthog-core.ts index 38c437a25..6f6aed21d 100644 --- a/src/posthog-core.ts +++ b/src/posthog-core.ts @@ -1828,7 +1828,7 @@ export class PostHog { this.autocapture?.startIfEnabled() this.heatmaps?.startIfEnabled() this.surveys.loadIfEnabled() - // this.siteApps?.loadIfEnabled() + this.siteApps?.loadIfEnabled() this._sync_opt_out_with_persistence() } } From acd394669519500e9c5e238613b35b9d1938f0b3 Mon Sep 17 00:00:00 2001 From: MarconLP <13001502+MarconLP@users.noreply.github.com> Date: Wed, 27 Nov 2024 15:45:34 +0100 Subject: [PATCH 07/21] add test for consent given at a later time --- src/__tests__/site-apps.ts | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/src/__tests__/site-apps.ts b/src/__tests__/site-apps.ts index 1aec6b1e1..5ad816a1f 100644 --- a/src/__tests__/site-apps.ts +++ b/src/__tests__/site-apps.ts @@ -313,5 +313,25 @@ describe('SiteApps', () => { expect(typeof assignableWindow['__$$ph_site_app_5_callback']).toBe('function') expect(typeof assignableWindow['__$$ph_site_app_6_callback']).toBe('undefined') }) + + it('load site destinations if consent is given at a later time', () => { + posthog.config.opt_in_site_apps = true + posthog.consent.isOptedOut = () => true + const response = { + siteApps: [ + { id: '5', type: 'site_app', url: '/site_app/5' }, + { id: '6', type: 'site_destination', url: '/site_app/6' }, + ], + } as DecideResponse + + siteAppsInstance.afterDecideResponse(response) + + posthog.consent.isOptedOut = () => false + + siteAppsInstance.loadIfEnabled() + + expect(typeof assignableWindow['__$$ph_site_app_5_callback']).toBe('function') + expect(typeof assignableWindow['__$$ph_site_app_6_callback']).toBe('function') + }) }) }) From c7eb17db3d6a230b3d1669435fa7a12927fa896a Mon Sep 17 00:00:00 2001 From: MarconLP <13001502+MarconLP@users.noreply.github.com> Date: Thu, 28 Nov 2024 22:59:52 +0100 Subject: [PATCH 08/21] remove code to expose posthog to the window --- playground/nextjs/src/posthog.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/playground/nextjs/src/posthog.ts b/playground/nextjs/src/posthog.ts index f3b24104f..fbf4eed3f 100644 --- a/playground/nextjs/src/posthog.ts +++ b/playground/nextjs/src/posthog.ts @@ -60,7 +60,6 @@ if (typeof window !== 'undefined') { persistence_name: `${process.env.NEXT_PUBLIC_POSTHOG_KEY}_nextjs`, ...configForConsent(), }) - ;(window as any).posthog = posthog // Help with debugging(window as any).posthog = posthog } From 6d166c2b9fbe17a76247041e6a3afc2218156d22 Mon Sep 17 00:00:00 2001 From: MarconLP <13001502+MarconLP@users.noreply.github.com> Date: Thu, 28 Nov 2024 23:02:02 +0100 Subject: [PATCH 09/21] rename variable to "_decideServerSiteAppsResponse" --- src/site-apps.ts | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/site-apps.ts b/src/site-apps.ts index 975c1a5fb..d4cc7a5f1 100644 --- a/src/site-apps.ts +++ b/src/site-apps.ts @@ -5,7 +5,7 @@ import { logger } from './utils/logger' import { isArray, isUndefined } from './utils/type-utils' export class SiteApps { - _decideServerResponse?: DecideResponse['siteApps'] + _decideServerSiteAppsResponse?: DecideResponse['siteApps'] missedInvocations: Record[] loaded: boolean appsLoading: Set @@ -74,9 +74,9 @@ export class SiteApps { loadIfEnabled() { if ( - this._decideServerResponse && - isArray(this._decideServerResponse) && - this._decideServerResponse.length > 0 + this._decideServerSiteAppsResponse && + isArray(this._decideServerSiteAppsResponse) && + this._decideServerSiteAppsResponse.length > 0 ) { // can't use if site apps are disabled, or if we're not asking /decide for site apps const enabled = this.instance.config.opt_in_site_apps && !this.instance.config.advanced_disable_decide @@ -88,7 +88,7 @@ export class SiteApps { this.missedInvocations = [] } } - for (const { id, type, url } of this._decideServerResponse) { + for (const { id, type, url } of this._decideServerSiteAppsResponse) { // if consent isn't given, skip site destinations if (this.instance.consent.isOptedOut() && type === 'site_destination') { checkIfAllLoaded() @@ -114,7 +114,7 @@ export class SiteApps { } }) } - } else if (this._decideServerResponse.length > 0) { + } else if (this._decideServerSiteAppsResponse.length > 0) { logger.error('PostHog site apps are disabled. Enable the "opt_in_site_apps" config to proceed.') this.loaded = true } else { @@ -126,7 +126,7 @@ export class SiteApps { } afterDecideResponse(response: DecideResponse): void { - this._decideServerResponse = response['siteApps'] + this._decideServerSiteAppsResponse = response['siteApps'] this.loadIfEnabled() } } From 7bd4b2ff6b4407c7800f75a7fadf54766943ba36 Mon Sep 17 00:00:00 2001 From: MarconLP <13001502+MarconLP@users.noreply.github.com> Date: Thu, 28 Nov 2024 23:16:44 +0100 Subject: [PATCH 10/21] fix test --- src/__tests__/site-apps.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/__tests__/site-apps.ts b/src/__tests__/site-apps.ts index 5ad816a1f..d7dc47786 100644 --- a/src/__tests__/site-apps.ts +++ b/src/__tests__/site-apps.ts @@ -200,7 +200,7 @@ describe('SiteApps', () => { siteAppsInstance.afterDecideResponse(response) expect(siteAppsInstance.loaded).toBe(true) - expect(siteAppsInstance._decideServerResponse.length).toBe(0) + expect(siteAppsInstance._decideServerSiteAppsResponse.length).toBe(0) }) it('loads site apps when enabled and opt_in_site_apps is true', (done) => { From bc81d02c9d806570ebaf24e625500e68e5659b00 Mon Sep 17 00:00:00 2001 From: MarconLP <13001502+MarconLP@users.noreply.github.com> Date: Thu, 28 Nov 2024 23:17:02 +0100 Subject: [PATCH 11/21] code cleanup --- src/site-apps.ts | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/src/site-apps.ts b/src/site-apps.ts index d4cc7a5f1..f57a95c6a 100644 --- a/src/site-apps.ts +++ b/src/site-apps.ts @@ -90,15 +90,9 @@ export class 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') { - checkIfAllLoaded() - continue - } + 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}_posthog`])) { - checkIfAllLoaded() - continue - } + if (!isUndefined(assignableWindow[`__$$ph_site_app_${id}_posthog`])) continue this.appsLoading.add(id) assignableWindow[`__$$ph_site_app_${id}_posthog`] = this.instance assignableWindow[`__$$ph_site_app_${id}_missed_invocations`] = () => this.missedInvocations @@ -114,6 +108,7 @@ export class SiteApps { } }) } + checkIfAllLoaded() } else if (this._decideServerSiteAppsResponse.length > 0) { logger.error('PostHog site apps are disabled. Enable the "opt_in_site_apps" config to proceed.') this.loaded = true From 52c352214860acc303594a56643298f40d417ccd Mon Sep 17 00:00:00 2001 From: MarconLP <13001502+MarconLP@users.noreply.github.com> Date: Thu, 28 Nov 2024 23:34:51 +0100 Subject: [PATCH 12/21] add test for the advanced_disable_decide config --- src/__tests__/site-apps.ts | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/__tests__/site-apps.ts b/src/__tests__/site-apps.ts index d7dc47786..f900b4aaa 100644 --- a/src/__tests__/site-apps.ts +++ b/src/__tests__/site-apps.ts @@ -95,13 +95,21 @@ describe('SiteApps', () => { }) describe('eventCollector', () => { - it('does nothing if enabled is false', () => { + it('does nothing if opt_in_site_apps is false', () => { posthog.config.opt_in_site_apps = false siteAppsInstance.eventCollector('event_name', {} as CaptureResult) expect(siteAppsInstance.missedInvocations.length).toBe(0) }) + it('does nothing if decide is disabled', () => { + posthog.config.opt_in_site_apps = true + posthog.config.advanced_disable_decide = true + siteAppsInstance.eventCollector('event_name', {} as CaptureResult) + + expect(siteAppsInstance.missedInvocations.length).toBe(0) + }) + it('collects event if enabled and loaded is false', () => { posthog.config.opt_in_site_apps = true siteAppsInstance.loaded = false From 79023d87bc650b66dc7076a9dcaf8b26b6bd251d Mon Sep 17 00:00:00 2001 From: MarconLP <13001502+MarconLP@users.noreply.github.com> Date: Fri, 29 Nov 2024 00:21:51 +0100 Subject: [PATCH 13/21] adjust to previous code --- playground/nextjs/src/posthog.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/playground/nextjs/src/posthog.ts b/playground/nextjs/src/posthog.ts index fbf4eed3f..eb1882589 100644 --- a/playground/nextjs/src/posthog.ts +++ b/playground/nextjs/src/posthog.ts @@ -60,6 +60,7 @@ if (typeof window !== 'undefined') { persistence_name: `${process.env.NEXT_PUBLIC_POSTHOG_KEY}_nextjs`, ...configForConsent(), }) + // Help with debugging(window as any).posthog = posthog } From a8d88bab11f29a3fb46eaa6a9043e609b4a933f9 Mon Sep 17 00:00:00 2001 From: MarconLP <13001502+MarconLP@users.noreply.github.com> Date: Fri, 29 Nov 2024 00:42:42 +0100 Subject: [PATCH 14/21] use `opt_out_capturing` instead of calling `loadIfEnabled` directly --- src/__tests__/site-apps.ts | 53 ++++++++++++-------------------------- 1 file changed, 16 insertions(+), 37 deletions(-) diff --git a/src/__tests__/site-apps.ts b/src/__tests__/site-apps.ts index f900b4aaa..4980fd25d 100644 --- a/src/__tests__/site-apps.ts +++ b/src/__tests__/site-apps.ts @@ -1,19 +1,12 @@ import { SiteApps } from '../site-apps' -import { PostHogPersistence } from '../posthog-persistence' -import { RequestRouter } from '../utils/request-router' import { PostHog } from '../posthog-core' -import { DecideResponse, PostHogConfig, Properties, CaptureResult } from '../types' +import { DecideResponse, PostHogConfig, CaptureResult } from '../types' import { assignableWindow } from '../utils/globals' import { logger } from '../utils/logger' import '../entrypoints/external-scripts-loader' import { isFunction } from '../utils/type-utils' -import { ConsentManager } from '../consent' - -jest.mock('../utils/logger', () => ({ - logger: { - error: jest.fn(), - }, -})) +import { uuidv7 } from '../uuidv7' +import { defaultPostHog } from './helpers/posthog-instance' describe('SiteApps', () => { let posthog: PostHog @@ -45,31 +38,17 @@ describe('SiteApps', () => { }), } - posthog = { - config: { ...defaultConfig }, - 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], - consent: { - isOptedOut(): boolean { - return false - }, - } as unknown as ConsentManager, - capture: jest.fn(), - _addCaptureHook: jest.fn(), - _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' }), - } as unknown as PostHog + const createPostHog = (config: Partial = {}) => { + const posthog = defaultPostHog().init('testtoken', { ...config }, uuidv7())! + posthog.debug() + return posthog + } + + posthog = createPostHog(defaultConfig) + posthog._addCaptureHook = jest.fn() + posthog.capture = jest.fn() + posthog._send_request = jest.fn().mockImplementation(({ callback }) => callback?.({ config: {} })) + logger.error = jest.fn() siteAppsInstance = new SiteApps(posthog) }) @@ -324,7 +303,7 @@ describe('SiteApps', () => { it('load site destinations if consent is given at a later time', () => { posthog.config.opt_in_site_apps = true - posthog.consent.isOptedOut = () => true + posthog.opt_out_capturing() const response = { siteApps: [ { id: '5', type: 'site_app', url: '/site_app/5' }, @@ -334,7 +313,7 @@ describe('SiteApps', () => { siteAppsInstance.afterDecideResponse(response) - posthog.consent.isOptedOut = () => false + posthog.opt_in_capturing() siteAppsInstance.loadIfEnabled() From dafacf47a470d7436755500682fcfa08ccdc06c2 Mon Sep 17 00:00:00 2001 From: MarconLP <13001502+MarconLP@users.noreply.github.com> Date: Mon, 2 Dec 2024 15:44:13 +0100 Subject: [PATCH 15/21] use previous site-app variables --- src/site-apps.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/site-apps.ts b/src/site-apps.ts index f57a95c6a..63d70922a 100644 --- a/src/site-apps.ts +++ b/src/site-apps.ts @@ -94,7 +94,7 @@ export class SiteApps { // if the site app is already loaded, skip it if (!isUndefined(assignableWindow[`__$$ph_site_app_${id}_posthog`])) continue this.appsLoading.add(id) - assignableWindow[`__$$ph_site_app_${id}_posthog`] = this.instance + assignableWindow[`__$$ph_site_app_${id}`] = this.instance assignableWindow[`__$$ph_site_app_${id}_missed_invocations`] = () => this.missedInvocations assignableWindow[`__$$ph_site_app_${id}_callback`] = () => { this.appsLoading.delete(id) From 3f09b4835c5384a338cd562091eb2384284f1077 Mon Sep 17 00:00:00 2001 From: MarconLP <13001502+MarconLP@users.noreply.github.com> Date: Mon, 2 Dec 2024 15:45:37 +0100 Subject: [PATCH 16/21] use previous site-app variables --- src/__tests__/site-apps.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/__tests__/site-apps.ts b/src/__tests__/site-apps.ts index 4980fd25d..382c14d8d 100644 --- a/src/__tests__/site-apps.ts +++ b/src/__tests__/site-apps.ts @@ -250,7 +250,7 @@ describe('SiteApps', () => { siteAppsInstance.afterDecideResponse(response) - expect(assignableWindow['__$$ph_site_app_8_posthog']).toBe(posthog) + expect(assignableWindow['__$$ph_site_app_8']).toBe(posthog) expect(typeof assignableWindow['__$$ph_site_app_8_missed_invocations']).toBe('function') expect(typeof assignableWindow['__$$ph_site_app_8_callback']).toBe('function') expect(assignableWindow.__PosthogExtensions__?.loadSiteApp).toHaveBeenCalledWith( From b7d742492da6d0c90a790434d70bf6768aee5c99 Mon Sep 17 00:00:00 2001 From: MarconLP <13001502+MarconLP@users.noreply.github.com> Date: Mon, 2 Dec 2024 16:29:44 +0100 Subject: [PATCH 17/21] use previous site-app variables --- src/site-apps.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/site-apps.ts b/src/site-apps.ts index 63d70922a..7a6c3408f 100644 --- a/src/site-apps.ts +++ b/src/site-apps.ts @@ -92,7 +92,7 @@ export class SiteApps { // 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}_posthog`])) continue + if (!isUndefined(assignableWindow[`__$$ph_site_app_${id}`])) continue this.appsLoading.add(id) assignableWindow[`__$$ph_site_app_${id}`] = this.instance assignableWindow[`__$$ph_site_app_${id}_missed_invocations`] = () => this.missedInvocations From 19d0a7a7fd738ee7b6d76ea845be326d3aabb632 Mon Sep 17 00:00:00 2001 From: MarconLP <13001502+MarconLP@users.noreply.github.com> Date: Tue, 3 Dec 2024 11:24:48 +0100 Subject: [PATCH 18/21] do not set loaded to true if decideResponse is empty & only clear missedInvocations after site_destinations have been loaded --- src/site-apps.ts | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/site-apps.ts b/src/site-apps.ts index 7a6c3408f..617372ac7 100644 --- a/src/site-apps.ts +++ b/src/site-apps.ts @@ -85,7 +85,10 @@ export class SiteApps { // Stop collecting events once all site apps are loaded if (this.appsLoading.size === 0) { this.loaded = true - this.missedInvocations = [] + // only clear missedInvocations after site_destinations have been loaded + if (!this.instance.consent.isOptedOut()) { + this.missedInvocations = [] + } } } for (const { id, type, url } of this._decideServerSiteAppsResponse) { @@ -115,8 +118,6 @@ export class SiteApps { } else { this.loaded = true } - } else { - this.loaded = true } } From c20b71c369b323e73907fa2c8dc0b91dd0e11da7 Mon Sep 17 00:00:00 2001 From: MarconLP <13001502+MarconLP@users.noreply.github.com> Date: Tue, 3 Dec 2024 12:12:52 +0100 Subject: [PATCH 19/21] Revert "do not set loaded to true if decideResponse is empty & only clear missedInvocations after site_destinations have been loaded" This reverts commit 19d0a7a7fd738ee7b6d76ea845be326d3aabb632. --- src/site-apps.ts | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/site-apps.ts b/src/site-apps.ts index 617372ac7..7a6c3408f 100644 --- a/src/site-apps.ts +++ b/src/site-apps.ts @@ -85,10 +85,7 @@ export class SiteApps { // Stop collecting events once all site apps are loaded if (this.appsLoading.size === 0) { this.loaded = true - // only clear missedInvocations after site_destinations have been loaded - if (!this.instance.consent.isOptedOut()) { - this.missedInvocations = [] - } + this.missedInvocations = [] } } for (const { id, type, url } of this._decideServerSiteAppsResponse) { @@ -118,6 +115,8 @@ export class SiteApps { } else { this.loaded = true } + } else { + this.loaded = true } } From 454b8992bc38653a1dabec937867c9e72a68a58f Mon Sep 17 00:00:00 2001 From: MarconLP <13001502+MarconLP@users.noreply.github.com> Date: Thu, 5 Dec 2024 15:24:20 +0100 Subject: [PATCH 20/21] save campaign params in storage before isOptedIn --- src/posthog-core.ts | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/posthog-core.ts b/src/posthog-core.ts index 6f6aed21d..4e92bb21c 100644 --- a/src/posthog-core.ts +++ b/src/posthog-core.ts @@ -808,6 +808,13 @@ 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.consent.isOptedOut()) { return } @@ -837,9 +844,6 @@ export class PostHog { // 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() } From d91ade818e32b5c7cfacde41776a9e139662a01b Mon Sep 17 00:00:00 2001 From: MarconLP <13001502+MarconLP@users.noreply.github.com> Date: Fri, 6 Dec 2024 00:22:55 +0100 Subject: [PATCH 21/21] save initial person properties in memory as well --- src/posthog-core.ts | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/src/posthog-core.ts b/src/posthog-core.ts index 4e92bb21c..c40e4e506 100644 --- a/src/posthog-core.ts +++ b/src/posthog-core.ts @@ -814,6 +814,12 @@ export class PostHog { 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 @@ -841,16 +847,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.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