From 1383a089199d18e27098f99491455978184b644a Mon Sep 17 00:00:00 2001 From: Ben White Date: Wed, 11 Dec 2024 14:37:07 +0100 Subject: [PATCH 01/13] Fix up loaders --- playground/nextjs/pages/_app.tsx | 4 +- src/__tests__/posthog-core.loaded.ts | 90 +++++++++----------------- src/__tests__/posthog-core.ts | 54 +++++----------- src/decide.ts | 75 ++++------------------ src/posthog-core.ts | 7 --- src/posthog-featureflags.ts | 94 ++++++++++++++-------------- 6 files changed, 109 insertions(+), 215 deletions(-) diff --git a/playground/nextjs/pages/_app.tsx b/playground/nextjs/pages/_app.tsx index 3d74c2fe6..55caa3b40 100644 --- a/playground/nextjs/pages/_app.tsx +++ b/playground/nextjs/pages/_app.tsx @@ -33,7 +33,9 @@ export default function App({ Component, pageProps }: AppProps) { } }, []) - const localhostDomain = process.env.NEXT_PUBLIC_CROSSDOMAIN ? 'https://localhost:8000' : 'http://localhost:8000' + const localhostDomain = process.env.NEXT_PUBLIC_CROSSDOMAIN + ? 'https://localhost:8000' + : process.env.NEXT_PUBLIC_POSTHOG_HOST return ( diff --git a/src/__tests__/posthog-core.loaded.ts b/src/__tests__/posthog-core.loaded.ts index 925087ce2..88b7b5535 100644 --- a/src/__tests__/posthog-core.loaded.ts +++ b/src/__tests__/posthog-core.loaded.ts @@ -1,86 +1,58 @@ import { createPosthogInstance } from './helpers/posthog-instance' import { uuidv7 } from '../uuidv7' import { PostHog } from '../posthog-core' +import { PostHogConfig } from '../types' jest.useFakeTimers() describe('loaded() with flags', () => { let instance: PostHog - const config = { loaded: jest.fn(), api_host: 'https://app.posthog.com' } - - const overrides = { - capture: jest.fn(), - _send_request: jest.fn(({ callback }) => callback?.({ status: 200, json: {} })), - _start_queue_if_opted_in: jest.fn(), - } beforeAll(() => { jest.unmock('../decide') }) - beforeEach(async () => { - const posthog = await createPosthogInstance(uuidv7(), config) - instance = Object.assign(posthog, { - ...overrides, - featureFlags: { - setReloadingPaused: jest.fn(), - resetRequestQueue: jest.fn(), - _startReloadTimer: jest.fn(), - receivedFeatureFlags: jest.fn(), - onFeatureFlags: jest.fn(), + const createPosthog = async (config?: Partial) => { + const posthog = await createPosthogInstance(uuidv7(), { + api_host: 'https://app.posthog.com', + ...config, + loaded: (ph) => { + ph.capture = jest.fn() + ph._send_request = jest.fn(({ callback }) => callback?.({ status: 200, json: {} })) + ph._start_queue_if_opted_in = jest.fn() + + jest.spyOn(ph.featureFlags, 'setGroupPropertiesForFlags') + jest.spyOn(ph.featureFlags, 'setReloadingPaused') + jest.spyOn(ph.featureFlags, 'reloadFeatureFlags') + jest.spyOn(ph.featureFlags, '_callDecideEndpoint') + + ph.group('org', 'bazinga', { name: 'Shelly' }) + setTimeout(() => { + ph.group('org', 'bazinga2', { name: 'Shelly' }) + }, 100) }, - _send_request: jest.fn(({ callback }) => callback?.({ status: 200, json: {} })), }) - }) - describe('toggling flag reloading', () => { - beforeEach(async () => { - const posthog = await createPosthogInstance(uuidv7(), { - ...config, - loaded: (ph) => { - ph.group('org', 'bazinga', { name: 'Shelly' }) - setTimeout(() => { - ph.group('org', 'bazinga2', { name: 'Shelly' }) - }, 100) - }, - }) - instance = Object.assign(posthog, overrides) + return posthog + } - jest.spyOn(instance.featureFlags, 'setGroupPropertiesForFlags') - jest.spyOn(instance.featureFlags, 'setReloadingPaused') - jest.spyOn(instance.featureFlags, '_startReloadTimer') - jest.spyOn(instance.featureFlags, 'resetRequestQueue') - jest.spyOn(instance.featureFlags, '_reloadFeatureFlagsRequest') - }) + beforeEach(async () => { + instance = await createPosthog() + }) + describe('toggling flag reloading', () => { it('doesnt call flags while initial load is happening', () => { - instance._loaded() - - jest.runOnlyPendingTimers() - expect(instance.featureFlags.setGroupPropertiesForFlags).toHaveBeenCalled() // loaded ph.group() calls setGroupPropertiesForFlags - expect(instance.featureFlags.setReloadingPaused).toHaveBeenCalledWith(true) - expect(instance.featureFlags.resetRequestQueue).toHaveBeenCalledTimes(1) - expect(instance.featureFlags._startReloadTimer).toHaveBeenCalled() - expect(instance.featureFlags.setReloadingPaused).toHaveBeenCalledWith(false) - - // we should call _reloadFeatureFlagsRequest for `group` only after the initial load - // because it ought to be paused until decide returns + expect(instance.featureFlags.reloadFeatureFlags).toHaveBeenCalledTimes(1) // 1 call from load + group debounced + expect(instance.featureFlags._callDecideEndpoint).toHaveBeenCalledTimes(1) expect(instance._send_request).toHaveBeenCalledTimes(1) - expect(instance.featureFlags._reloadFeatureFlagsRequest).toHaveBeenCalledTimes(0) jest.runOnlyPendingTimers() - expect(instance._send_request).toHaveBeenCalledTimes(2) - expect(instance.featureFlags._reloadFeatureFlagsRequest).toHaveBeenCalledTimes(1) - }) - }) - it('toggles feature flags on and off', () => { - instance._loaded() + expect(instance.featureFlags.reloadFeatureFlags).toHaveBeenCalledTimes(2) // Additional call for groups change + expect(instance.featureFlags._callDecideEndpoint).toHaveBeenCalledTimes(2) - expect(instance.featureFlags.setReloadingPaused).toHaveBeenCalledWith(true) - expect(instance.featureFlags.setReloadingPaused).toHaveBeenCalledWith(false) - expect(instance.featureFlags._startReloadTimer).toHaveBeenCalled() - expect(instance.featureFlags.receivedFeatureFlags).toHaveBeenCalledTimes(1) + expect(instance._send_request).toHaveBeenCalledTimes(2) + }) }) }) diff --git a/src/__tests__/posthog-core.ts b/src/__tests__/posthog-core.ts index 0579706b4..744ad5df8 100644 --- a/src/__tests__/posthog-core.ts +++ b/src/__tests__/posthog-core.ts @@ -1088,18 +1088,7 @@ describe('posthog core', () => { describe('_loaded()', () => { it('calls loaded config option', () => { - const posthog = posthogWith( - { loaded: jest.fn() }, - { - capture: jest.fn(), - featureFlags: { - setReloadingPaused: jest.fn(), - resetRequestQueue: jest.fn(), - _startReloadTimer: jest.fn(), - } as unknown as PostHogFeatureFlags, - _start_queue_if_opted_in: jest.fn(), - } - ) + const posthog = posthogWith({ loaded: jest.fn() }) posthog._loaded() @@ -1107,22 +1096,11 @@ describe('posthog core', () => { }) it('handles loaded config option throwing gracefully', () => { - const posthog = posthogWith( - { - loaded: () => { - throw Error() - }, + const posthog = posthogWith({ + loaded: () => { + throw Error() }, - { - capture: jest.fn(), - featureFlags: { - setReloadingPaused: jest.fn(), - resetRequestQueue: jest.fn(), - _startReloadTimer: jest.fn(), - } as unknown as PostHogFeatureFlags, - _start_queue_if_opted_in: jest.fn(), - } - ) + }) posthog._loaded() @@ -1131,27 +1109,27 @@ describe('posthog core', () => { describe('/decide', () => { it('is called by default', async () => { - const instance = await createPosthogInstance(uuidv7()) - instance.featureFlags.setReloadingPaused = jest.fn() - instance._send_request = jest.fn() - instance._loaded() + const sendRequestMock = jest.fn() + await createPosthogInstance(uuidv7(), { + loaded: (ph) => { + ph._send_request = sendRequestMock + }, + }) - expect(instance._send_request.mock.calls[0][0]).toMatchObject({ + expect(sendRequestMock.mock.calls[0][0]).toMatchObject({ url: 'http://localhost/decide/?v=3', }) - expect(instance.featureFlags.setReloadingPaused).toHaveBeenCalledWith(true) }) it('does not call decide if disabled', async () => { + const sendRequestMock = jest.fn() const instance = await createPosthogInstance(uuidv7(), { advanced_disable_decide: true, + loaded: (ph) => { + ph._send_request = sendRequestMock + }, }) - instance.featureFlags.setReloadingPaused = jest.fn() - instance._send_request = jest.fn() - instance._loaded() - expect(instance._send_request).not.toHaveBeenCalled() - expect(instance.featureFlags.setReloadingPaused).not.toHaveBeenCalled() }) }) }) diff --git a/src/decide.ts b/src/decide.ts index 35a1747ef..dfac7e789 100644 --- a/src/decide.ts +++ b/src/decide.ts @@ -1,6 +1,5 @@ import { PostHog } from './posthog-core' -import { Compression, DecideResponse, RemoteConfig } from './types' -import { STORED_GROUP_PROPERTIES_KEY, STORED_PERSON_PROPERTIES_KEY } from './constants' +import { RemoteConfig } from './types' import { createLogger } from './utils/logger' import { assignableWindow, document } from './utils/globals' @@ -40,12 +39,6 @@ export class Decide { // and compression will not be available. const disableRemoteCalls = !!this.instance.config.advanced_disable_decide - if (!disableRemoteCalls) { - // TRICKY: Reset any decide reloads queued during config.loaded because they'll be - // covered by the decide call right above. - this.instance.featureFlags.resetRequestQueue() - } - if (this.instance.config.__preview_remote_config) { // Attempt 1 - use the pre-loaded config if it came as part of the token-specific array.js if (assignableWindow._POSTHOG_CONFIG) { @@ -80,61 +73,18 @@ export class Decide { return } - /* - Calls /decide endpoint to fetch options for autocapture, session recording, feature flags & compression. - */ - const data = { - token: this.instance.config.token, - distinct_id: this.instance.get_distinct_id(), - groups: this.instance.getGroups(), - person_properties: this.instance.get_property(STORED_PERSON_PROPERTIES_KEY), - group_properties: this.instance.get_property(STORED_GROUP_PROPERTIES_KEY), - disable_flags: - this.instance.config.advanced_disable_feature_flags || - this.instance.config.advanced_disable_feature_flags_on_first_load || - undefined, - } - - this.instance._send_request({ - method: 'POST', - url: this.instance.requestRouter.endpointFor('api', '/decide/?v=3'), - data, - compression: this.instance.config.disable_compression ? undefined : Compression.Base64, - timeout: this.instance.config.feature_flag_request_timeout_ms, - callback: (response) => this.parseDecideResponse(response.json as DecideResponse | undefined), + this.instance.featureFlags._callDecideEndpoint({ + data: { + disable_flags: + this.instance.config.advanced_disable_feature_flags || + this.instance.config.advanced_disable_feature_flags_on_first_load || + undefined, + }, + callback: (response) => this.onRemoteConfig(response, true), }) } - parseDecideResponse(response?: DecideResponse): void { - this.instance.featureFlags.setReloadingPaused(false) - // :TRICKY: Reload - start another request if queued! - this.instance.featureFlags._startReloadTimer() - - const errorsLoading = !response - - if ( - !this.instance.config.advanced_disable_feature_flags_on_first_load && - !this.instance.config.advanced_disable_feature_flags - ) { - this.instance.featureFlags.receivedFeatureFlags(response ?? {}, errorsLoading) - } - - if (errorsLoading) { - logger.error('Failed to fetch feature flags from PostHog.') - return - } - if (!(document && document.body)) { - logger.info('document not ready yet, trying again in 500 milliseconds...') - setTimeout(() => { - this.parseDecideResponse(response) - }, 500) - return - } - - this.instance._onRemoteConfig(response) - } - - private onRemoteConfig(config?: RemoteConfig): void { + private onRemoteConfig(config?: RemoteConfig, fromDecide?: boolean): void { // NOTE: Once this is rolled out we will remove the "decide" related code above. Until then the code duplication is fine. if (!config) { logger.error('Failed to fetch remote config from PostHog.') @@ -150,9 +100,8 @@ export class Decide { this.instance._onRemoteConfig(config) - if (config.hasFeatureFlags !== false) { - // TRICKY: This is set in the parent for some reason... - this.instance.featureFlags.setReloadingPaused(false) + if (config.hasFeatureFlags !== false && !fromDecide) { + logger.info('hasFeatureFlags is true, reloading flags') // If the config has feature flags, we need to call decide to get the feature flags // This completely separates it from the config logic which is good in terms of separation of concerns this.instance.featureFlags.reloadFeatureFlags() diff --git a/src/posthog-core.ts b/src/posthog-core.ts index a310f84e5..da63cf58c 100644 --- a/src/posthog-core.ts +++ b/src/posthog-core.ts @@ -577,13 +577,6 @@ export class PostHog { } _loaded(): void { - // Pause `reloadFeatureFlags` calls in config.loaded callback. - // These feature flags are loaded in the decide call made right after - const disableDecide = this.config.advanced_disable_decide - if (!disableDecide) { - this.featureFlags.setReloadingPaused(true) - } - try { this.config.loaded(this) } catch (err) { diff --git a/src/posthog-featureflags.ts b/src/posthog-featureflags.ts index 55debed5e..ad0f1d54b 100644 --- a/src/posthog-featureflags.ts +++ b/src/posthog-featureflags.ts @@ -82,20 +82,16 @@ export const parseFeatureFlagDecideResponse = ( } export class PostHogFeatureFlags { - instance: PostHog - _override_warning: boolean + _override_warning: boolean = false featureFlagEventHandlers: FeatureFlagsCallback[] - reloadFeatureFlagsQueued: boolean - reloadFeatureFlagsInAction: boolean $anon_distinct_id: string | undefined + private _requestInFlight: boolean = false + private _reloadingDisabled: boolean = false + private _additionalReloadRequested: boolean = false + private _reloadDebouncer?: any - constructor(instance: PostHog) { - this.instance = instance - this._override_warning = false + constructor(private instance: PostHog) { this.featureFlagEventHandlers = [] - - this.reloadFeatureFlagsQueued = false - this.reloadFeatureFlagsInAction = false } getFlags(): string[] { @@ -137,13 +133,23 @@ export class PostHogFeatureFlags { * * 1. Avoid parallel requests * 2. Delay a few milliseconds after each reloadFeatureFlags call to batch subsequent changes together - * 3. Don't call this during initial load (as /decide will be called instead), see posthog-core.js */ reloadFeatureFlags(): void { - if (!this.reloadFeatureFlagsQueued) { - this.reloadFeatureFlagsQueued = true - this._startReloadTimer() + if (this._reloadingDisabled) { + // If reloading has been explicitly disabled then we don't want to do anything + return + } + + if (this._reloadDebouncer) { + // If we're already in a debounce then we don't want to do anything + return } + + // Debounce multiple calls on the same tick + this._reloadDebouncer = setTimeout(() => { + this._reloadDebouncer = undefined + this._callDecideEndpoint() + }, 5) } setAnonymousDistinctId(anon_distinct_id: string): void { @@ -151,51 +157,41 @@ export class PostHogFeatureFlags { } setReloadingPaused(isPaused: boolean): void { - this.reloadFeatureFlagsInAction = isPaused - } - - resetRequestQueue(): void { - this.reloadFeatureFlagsQueued = false - } - - _startReloadTimer(): void { - if (this.reloadFeatureFlagsQueued && !this.reloadFeatureFlagsInAction) { - setTimeout(() => { - if (!this.reloadFeatureFlagsInAction && this.reloadFeatureFlagsQueued) { - this.reloadFeatureFlagsQueued = false - this._reloadFeatureFlagsRequest() - } - }, 5) - } + this._reloadingDisabled = isPaused } - _reloadFeatureFlagsRequest(): void { - if (this.instance.config.advanced_disable_feature_flags) { + /** + * NOTE: This is used both for flags and remote config. Once the RemoteConfig is fully released this will essentially only + * be for flags and can eventually be replaced wiht the new flags endpoint + */ + _callDecideEndpoint(options?: { data: Record; callback: (response: DecideResponse) => void }): void { + if (this._requestInFlight) { + this._additionalReloadRequested = true return } - - this.setReloadingPaused(true) const token = this.instance.config.token - const personProperties = this.instance.get_property(STORED_PERSON_PROPERTIES_KEY) - const groupProperties = this.instance.get_property(STORED_GROUP_PROPERTIES_KEY) - const json_data = { + const data = { token: token, distinct_id: this.instance.get_distinct_id(), groups: this.instance.getGroups(), $anon_distinct_id: this.$anon_distinct_id, - person_properties: personProperties, - group_properties: groupProperties, + person_properties: this.instance.get_property(STORED_PERSON_PROPERTIES_KEY), + group_properties: this.instance.get_property(STORED_GROUP_PROPERTIES_KEY), disable_flags: this.instance.config.advanced_disable_feature_flags || undefined, + ...(options?.data || {}), } + this._requestInFlight = true this.instance._send_request({ method: 'POST', url: this.instance.requestRouter.endpointFor('api', '/decide/?v=3'), - data: json_data, + data, compression: this.instance.config.disable_compression ? undefined : Compression.Base64, timeout: this.instance.config.feature_flag_request_timeout_ms, callback: (response) => { - this.setReloadingPaused(false) + this._requestInFlight = false + + options?.callback?.(response.json ?? {}) let errorsLoading = true @@ -206,14 +202,18 @@ export class PostHogFeatureFlags { this.$anon_distinct_id = undefined errorsLoading = false } - // :TRICKY: We want to fire the callback even if the request fails - // and return existing flags if they exist - // This is because we don't want to block clients waiting for flags to load. - // It's possible they're waiting for the callback to render the UI, but it never occurs. + + if (data.disable_flags) { + // If flags are disabled then there is no need to call decide again (flags are the only thing that may change) + return + } + this.receivedFeatureFlags(response.json ?? {}, errorsLoading) - // :TRICKY: Reload - start another request if queued! - this._startReloadTimer() + if (this._additionalReloadRequested) { + this._additionalReloadRequested = false + this._callDecideEndpoint() + } }, }) } From 80e6e0007abea4694382dfc2a2f8c9d3ccce956a Mon Sep 17 00:00:00 2001 From: Ben White Date: Thu, 12 Dec 2024 09:43:44 +0100 Subject: [PATCH 02/13] Fixes --- src/__tests__/decide.test.ts | 221 ++++++++++++ src/__tests__/decide.ts | 329 ------------------ .../{featureflags.ts => featureflags.test.ts} | 18 +- src/decide.ts | 5 +- src/posthog-core.ts | 7 +- src/posthog-featureflags.ts | 14 +- 6 files changed, 245 insertions(+), 349 deletions(-) create mode 100644 src/__tests__/decide.test.ts delete mode 100644 src/__tests__/decide.ts rename src/__tests__/{featureflags.ts => featureflags.test.ts} (98%) diff --git a/src/__tests__/decide.test.ts b/src/__tests__/decide.test.ts new file mode 100644 index 000000000..1e9c95b7e --- /dev/null +++ b/src/__tests__/decide.test.ts @@ -0,0 +1,221 @@ +import { Decide } from '../decide' +import { PostHogPersistence } from '../posthog-persistence' +import { RequestRouter } from '../utils/request-router' +import { PostHog } from '../posthog-core' +import { PostHogConfig, Properties, RemoteConfig } from '../types' +import '../entrypoints/external-scripts-loader' +import { assignableWindow } from '../utils/globals' + +describe('Decide', () => { + let posthog: PostHog + + beforeEach(() => { + // clean the JSDOM to prevent interdependencies between tests + + const defaultConfig: Partial = { + token: 'testtoken', + api_host: 'https://test.com', + persistence: 'memory', + } + + document.body.innerHTML = '' + document.head.innerHTML = '' + jest.spyOn(window.console, 'error').mockImplementation() + + 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], + capture: jest.fn(), + _addCaptureHook: jest.fn(), + _onRemoteConfig: jest.fn(), + get_distinct_id: jest.fn().mockImplementation(() => 'distinctid'), + _send_request: jest.fn().mockImplementation(({ callback }) => callback?.({ config: {} })), + featureFlags: { + resetRequestQueue: jest.fn(), + reloadFeatureFlags: jest.fn(), + receivedFeatureFlags: jest.fn(), + setReloadingPaused: jest.fn(), + _callDecideEndpoint: jest.fn(), + }, + requestRouter: new RequestRouter({ config: defaultConfig } as unknown as PostHog), + _hasBootstrappedFeatureFlags: jest.fn(), + getGroups: () => ({ organization: '5' }), + } as unknown as PostHog + }) + + describe('constructor', () => { + it('should call _callDecideEndpoint on constructor', () => { + new Decide(posthog).call() + + expect(posthog.featureFlags._callDecideEndpoint).toHaveBeenCalledTimes(1) + expect(posthog.featureFlags._callDecideEndpoint).toHaveBeenLastCalledWith({ + data: { + disable_flags: undefined, + }, + callback: expect.any(Function), + }) + }) + + it('should not call _callDecideEndpoint on constructor if advanced_disable_decide', () => { + posthog.config.advanced_disable_decide = true + new Decide(posthog).call() + + expect(posthog.featureFlags._callDecideEndpoint).toHaveBeenCalledTimes(0) + }) + + it('should call _callDecideEndpoint with disable_flags true if advanced_disable_feature_flags is set', () => { + console.log('posthog.config', posthog.config) + posthog.config.advanced_disable_feature_flags = true + posthog.config.advanced_disable_feature_flags_on_first_load = false + + new Decide(posthog).call() + expect(posthog.featureFlags._callDecideEndpoint).toHaveBeenCalledTimes(1) + expect(posthog.featureFlags._callDecideEndpoint).toHaveBeenLastCalledWith({ + data: { + disable_flags: true, + }, + callback: expect.any(Function), + }) + }) + + it('should call _callDecideEndpoint with disable_flags true if advanced_disable_feature_flags_on_first_load is set', () => { + posthog.config.advanced_disable_feature_flags = false + posthog.config.advanced_disable_feature_flags_on_first_load = true + + new Decide(posthog).call() + expect(posthog.featureFlags._callDecideEndpoint).toHaveBeenCalledTimes(1) + expect(posthog.featureFlags._callDecideEndpoint).toHaveBeenLastCalledWith({ + data: { + disable_flags: true, + }, + callback: expect.any(Function), + }) + }) + }) + + // describe('parseDecideResponse', () => { + + // it('Make sure receivedFeatureFlags is called with errors if the decide response fails', () => { + // ;(window as any).POSTHOG_DEBUG = true + + // subject(undefined as unknown as DecideResponse) + + // expect(posthog.featureFlags.receivedFeatureFlags).toHaveBeenCalledWith({}, true) + // expect(console.error).toHaveBeenCalledWith( + // '[PostHog.js] [Decide]', + // 'Failed to fetch feature flags from PostHog.' + // ) + // }) + + // it('Make sure receivedFeatureFlags is not called if advanced_disable_feature_flags_on_first_load is set', () => { + // posthog.config = { + // api_host: 'https://test.com', + // token: 'testtoken', + // persistence: 'memory', + // advanced_disable_feature_flags_on_first_load: true, + // } as PostHogConfig + + // const decideResponse = { + // featureFlags: { 'test-flag': true }, + // } as unknown as DecideResponse + // subject(decideResponse) + + // expect(posthog._onRemoteConfig).toHaveBeenCalledWith(decideResponse) + // expect(posthog.featureFlags.receivedFeatureFlags).not.toHaveBeenCalled() + // }) + + // it('Make sure receivedFeatureFlags is not called if advanced_disable_feature_flags is set', () => { + // posthog.config = { + // api_host: 'https://test.com', + // token: 'testtoken', + // persistence: 'memory', + // advanced_disable_feature_flags: true, + // } as PostHogConfig + + // const decideResponse = { + // featureFlags: { 'test-flag': true }, + // } as unknown as DecideResponse + // subject(decideResponse) + + // expect(posthog._onRemoteConfig).toHaveBeenCalledWith(decideResponse) + // expect(posthog.featureFlags.receivedFeatureFlags).not.toHaveBeenCalled() + // }) + // }) + + describe('remote config', () => { + const config = { surveys: true } as RemoteConfig + + beforeEach(() => { + posthog.config.__preview_remote_config = true + assignableWindow._POSTHOG_CONFIG = undefined + assignableWindow.POSTHOG_DEBUG = true + + assignableWindow.__PosthogExtensions__.loadExternalDependency = jest.fn( + (_ph: PostHog, _name: string, cb: (err?: any) => void) => { + assignableWindow._POSTHOG_CONFIG = config as RemoteConfig + cb() + } + ) + + posthog._send_request = jest.fn().mockImplementation(({ callback }) => callback?.({ json: config })) + }) + + it('properly pulls from the window and uses it if set', () => { + assignableWindow._POSTHOG_CONFIG = config as RemoteConfig + new Decide(posthog).call() + + expect(assignableWindow.__PosthogExtensions__.loadExternalDependency).not.toHaveBeenCalled() + expect(posthog._send_request).not.toHaveBeenCalled() + + expect(posthog._onRemoteConfig).toHaveBeenCalledWith(config) + }) + + it('loads the script if window config not set', () => { + new Decide(posthog).call() + + expect(assignableWindow.__PosthogExtensions__.loadExternalDependency).toHaveBeenCalledWith( + posthog, + 'remote-config', + expect.any(Function) + ) + expect(posthog._send_request).not.toHaveBeenCalled() + expect(posthog._onRemoteConfig).toHaveBeenCalledWith(config) + }) + + it('loads the json if window config not set and js failed', () => { + assignableWindow.__PosthogExtensions__.loadExternalDependency = jest.fn( + (_ph: PostHog, _name: string, cb: (err?: any) => void) => { + cb() + } + ) + + new Decide(posthog).call() + + expect(assignableWindow.__PosthogExtensions__.loadExternalDependency).toHaveBeenCalled() + expect(posthog._send_request).toHaveBeenCalledWith({ + method: 'GET', + url: 'https://test.com/array/testtoken/config', + callback: expect.any(Function), + }) + expect(posthog._onRemoteConfig).toHaveBeenCalledWith(config) + }) + + it.each([ + [true, true], + [false, false], + [undefined, true], + ])('conditionally reloads feature flags - hasFlags: %s, shouldReload: %s', (hasFeatureFlags, shouldReload) => { + assignableWindow._POSTHOG_CONFIG = { hasFeatureFlags } as RemoteConfig + new Decide(posthog).call() + + if (shouldReload) { + expect(posthog.featureFlags.reloadFeatureFlags).toHaveBeenCalled() + } else { + expect(posthog.featureFlags.reloadFeatureFlags).not.toHaveBeenCalled() + } + }) + }) +}) diff --git a/src/__tests__/decide.ts b/src/__tests__/decide.ts deleted file mode 100644 index a7c8ec266..000000000 --- a/src/__tests__/decide.ts +++ /dev/null @@ -1,329 +0,0 @@ -import { Decide } from '../decide' -import { PostHogPersistence } from '../posthog-persistence' -import { RequestRouter } from '../utils/request-router' -import { PostHog } from '../posthog-core' -import { DecideResponse, PostHogConfig, Properties, RemoteConfig } from '../types' -import '../entrypoints/external-scripts-loader' -import { assignableWindow } from '../utils/globals' - -const expectDecodedSendRequest = ( - send_request: PostHog['_send_request'], - data: Record, - noCompression: boolean, - posthog: PostHog -) => { - const lastCall = jest.mocked(send_request).mock.calls[jest.mocked(send_request).mock.calls.length - 1] - - const decoded = lastCall[0].data - // Helper to give us more accurate error messages - expect(decoded).toEqual(data) - - expect(posthog._send_request).toHaveBeenCalledWith({ - url: 'https://test.com/decide/?v=3', - data, - method: 'POST', - callback: expect.any(Function), - compression: noCompression ? undefined : 'base64', - timeout: undefined, - }) -} - -describe('Decide', () => { - let posthog: PostHog - - const decide = () => new Decide(posthog) - - const defaultConfig: Partial = { - token: 'testtoken', - api_host: 'https://test.com', - persistence: 'memory', - } - - beforeEach(() => { - // clean the JSDOM to prevent interdependencies between tests - document.body.innerHTML = '' - document.head.innerHTML = '' - jest.spyOn(window.console, 'error').mockImplementation() - - 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], - capture: jest.fn(), - _addCaptureHook: jest.fn(), - _onRemoteConfig: jest.fn(), - get_distinct_id: jest.fn().mockImplementation(() => 'distinctid'), - _send_request: jest.fn().mockImplementation(({ callback }) => callback?.({ config: {} })), - featureFlags: { - resetRequestQueue: jest.fn(), - reloadFeatureFlags: jest.fn(), - 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 - }) - - describe('constructor', () => { - it('should call instance._send_request on constructor', () => { - decide().call() - - expectDecodedSendRequest( - posthog._send_request, - { - token: 'testtoken', - distinct_id: 'distinctid', - groups: { organization: '5' }, - }, - false, - posthog - ) - }) - - it('should send all stored properties with decide request', () => { - posthog.register({ - $stored_person_properties: { key: 'value' }, - $stored_group_properties: { organization: { orgName: 'orgValue' } }, - }) - - decide().call() - - expectDecodedSendRequest( - posthog._send_request, - { - token: 'testtoken', - distinct_id: 'distinctid', - groups: { organization: '5' }, - person_properties: { key: 'value' }, - group_properties: { organization: { orgName: 'orgValue' } }, - }, - false, - posthog - ) - }) - - it('should send disable flags with decide request when config is set', () => { - posthog.config = { - api_host: 'https://test.com', - token: 'testtoken', - persistence: 'memory', - advanced_disable_feature_flags: true, - } as PostHogConfig - - posthog.register({ - $stored_person_properties: { key: 'value' }, - $stored_group_properties: { organization: { orgName: 'orgValue' } }, - }) - decide().call() - - expectDecodedSendRequest( - posthog._send_request, - { - token: 'testtoken', - distinct_id: 'distinctid', - groups: { organization: '5' }, - person_properties: { key: 'value' }, - group_properties: { organization: { orgName: 'orgValue' } }, - disable_flags: true, - }, - false, - posthog - ) - }) - - it('should disable compression when config is set', () => { - posthog.config = { - api_host: 'https://test.com', - token: 'testtoken', - persistence: 'memory', - disable_compression: true, - } as PostHogConfig - - posthog.register({ - $stored_person_properties: {}, - $stored_group_properties: {}, - }) - decide().call() - - // noCompression is true - expectDecodedSendRequest( - posthog._send_request, - { - token: 'testtoken', - distinct_id: 'distinctid', - groups: { organization: '5' }, - person_properties: {}, - group_properties: {}, - }, - true, - posthog - ) - }) - - it('should send disable flags with decide request when config for advanced_disable_feature_flags_on_first_load is set', () => { - posthog.config = { - api_host: 'https://test.com', - token: 'testtoken', - persistence: 'memory', - advanced_disable_feature_flags_on_first_load: true, - } as PostHogConfig - - posthog.register({ - $stored_person_properties: { key: 'value' }, - $stored_group_properties: { organization: { orgName: 'orgValue' } }, - }) - - decide().call() - - expectDecodedSendRequest( - posthog._send_request, - { - token: 'testtoken', - distinct_id: 'distinctid', - groups: { organization: '5' }, - person_properties: { key: 'value' }, - group_properties: { organization: { orgName: 'orgValue' } }, - disable_flags: true, - }, - false, - posthog - ) - }) - }) - - describe('parseDecideResponse', () => { - const subject = (decideResponse: DecideResponse) => decide().parseDecideResponse(decideResponse) - - it('properly parses decide response', () => { - subject({} as DecideResponse) - - expect(posthog.featureFlags.receivedFeatureFlags).toHaveBeenCalledWith({}, false) - expect(posthog._onRemoteConfig).toHaveBeenCalledWith({}) - }) - - it('Make sure receivedFeatureFlags is called with errors if the decide response fails', () => { - ;(window as any).POSTHOG_DEBUG = true - - subject(undefined as unknown as DecideResponse) - - expect(posthog.featureFlags.receivedFeatureFlags).toHaveBeenCalledWith({}, true) - expect(console.error).toHaveBeenCalledWith( - '[PostHog.js] [Decide]', - 'Failed to fetch feature flags from PostHog.' - ) - }) - - it('Make sure receivedFeatureFlags is not called if advanced_disable_feature_flags_on_first_load is set', () => { - posthog.config = { - api_host: 'https://test.com', - token: 'testtoken', - persistence: 'memory', - advanced_disable_feature_flags_on_first_load: true, - } as PostHogConfig - - const decideResponse = { - featureFlags: { 'test-flag': true }, - } as unknown as DecideResponse - subject(decideResponse) - - expect(posthog._onRemoteConfig).toHaveBeenCalledWith(decideResponse) - expect(posthog.featureFlags.receivedFeatureFlags).not.toHaveBeenCalled() - }) - - it('Make sure receivedFeatureFlags is not called if advanced_disable_feature_flags is set', () => { - posthog.config = { - api_host: 'https://test.com', - token: 'testtoken', - persistence: 'memory', - advanced_disable_feature_flags: true, - } as PostHogConfig - - const decideResponse = { - featureFlags: { 'test-flag': true }, - } as unknown as DecideResponse - subject(decideResponse) - - expect(posthog._onRemoteConfig).toHaveBeenCalledWith(decideResponse) - expect(posthog.featureFlags.receivedFeatureFlags).not.toHaveBeenCalled() - }) - }) - - describe('remote config', () => { - const config = { surveys: true } as RemoteConfig - - beforeEach(() => { - posthog.config.__preview_remote_config = true - assignableWindow._POSTHOG_CONFIG = undefined - assignableWindow.POSTHOG_DEBUG = true - - assignableWindow.__PosthogExtensions__.loadExternalDependency = jest.fn( - (_ph: PostHog, _name: string, cb: (err?: any) => void) => { - assignableWindow._POSTHOG_CONFIG = config as RemoteConfig - cb() - } - ) - - posthog._send_request = jest.fn().mockImplementation(({ callback }) => callback?.({ json: config })) - }) - - it('properly pulls from the window and uses it if set', () => { - assignableWindow._POSTHOG_CONFIG = config as RemoteConfig - decide().call() - - expect(assignableWindow.__PosthogExtensions__.loadExternalDependency).not.toHaveBeenCalled() - expect(posthog._send_request).not.toHaveBeenCalled() - - expect(posthog._onRemoteConfig).toHaveBeenCalledWith(config) - }) - - it('loads the script if window config not set', () => { - decide().call() - - expect(assignableWindow.__PosthogExtensions__.loadExternalDependency).toHaveBeenCalledWith( - posthog, - 'remote-config', - expect.any(Function) - ) - expect(posthog._send_request).not.toHaveBeenCalled() - expect(posthog._onRemoteConfig).toHaveBeenCalledWith(config) - }) - - it('loads the json if window config not set and js failed', () => { - assignableWindow.__PosthogExtensions__.loadExternalDependency = jest.fn( - (_ph: PostHog, _name: string, cb: (err?: any) => void) => { - cb() - } - ) - - decide().call() - - expect(assignableWindow.__PosthogExtensions__.loadExternalDependency).toHaveBeenCalled() - expect(posthog._send_request).toHaveBeenCalledWith({ - method: 'GET', - url: 'https://test.com/array/testtoken/config', - callback: expect.any(Function), - }) - expect(posthog._onRemoteConfig).toHaveBeenCalledWith(config) - }) - - it.each([ - [true, true], - [false, false], - [undefined, true], - ])('conditionally reloads feature flags - hasFlags: %s, shouldReload: %s', (hasFeatureFlags, shouldReload) => { - assignableWindow._POSTHOG_CONFIG = { hasFeatureFlags } as RemoteConfig - decide().call() - - if (shouldReload) { - expect(posthog.featureFlags.reloadFeatureFlags).toHaveBeenCalled() - } else { - expect(posthog.featureFlags.reloadFeatureFlags).not.toHaveBeenCalled() - } - }) - }) -}) diff --git a/src/__tests__/featureflags.ts b/src/__tests__/featureflags.test.ts similarity index 98% rename from src/__tests__/featureflags.ts rename to src/__tests__/featureflags.test.ts index 8dd585b94..ccb3ed071 100644 --- a/src/__tests__/featureflags.ts +++ b/src/__tests__/featureflags.test.ts @@ -67,7 +67,7 @@ describe('featureflags', () => { }) it('should return flags from persistence even if decide endpoint was not hit', () => { - featureFlags.instance.decideEndpointWasHit = false + featureFlags._hasLoadedFlags = false expect(featureFlags.getFlags()).toEqual([ 'beta-feature', @@ -80,7 +80,7 @@ describe('featureflags', () => { it('should warn if decide endpoint was not hit and no flags exist', () => { ;(window as any).POSTHOG_DEBUG = true - featureFlags.instance.decideEndpointWasHit = false + featureFlags._hasLoadedFlags = false instance.persistence.unregister('$enabled_feature_flags') instance.persistence.unregister('$active_feature_flags') @@ -101,7 +101,7 @@ describe('featureflags', () => { }) it('should return the right feature flag and call capture', () => { - featureFlags.instance.decideEndpointWasHit = false + featureFlags._hasLoadedFlags = false expect(featureFlags.getFlags()).toEqual([ 'beta-feature', @@ -132,7 +132,7 @@ describe('featureflags', () => { }) it('should call capture for every different flag response', () => { - featureFlags.instance.decideEndpointWasHit = true + featureFlags._hasLoadedFlags = true instance.persistence.register({ $enabled_feature_flags: { @@ -156,13 +156,13 @@ describe('featureflags', () => { instance.persistence.register({ $enabled_feature_flags: {}, }) - featureFlags.instance.decideEndpointWasHit = false + featureFlags._hasLoadedFlags = false expect(featureFlags.getFlagVariants()).toEqual({}) expect(featureFlags.isFeatureEnabled('beta-feature')).toEqual(undefined) // no extra capture call because flags haven't loaded yet. expect(instance.capture).toHaveBeenCalledTimes(1) - featureFlags.instance.decideEndpointWasHit = true + featureFlags._hasLoadedFlags = true instance.persistence.register({ $enabled_feature_flags: { x: 'y' }, }) @@ -185,7 +185,7 @@ describe('featureflags', () => { }) it('should return the right feature flag and not call capture', () => { - featureFlags.instance.decideEndpointWasHit = true + featureFlags._hasLoadedFlags = true expect(featureFlags.isFeatureEnabled('beta-feature', { send_event: false })).toEqual(true) expect(instance.capture).not.toHaveBeenCalled() @@ -315,7 +315,7 @@ describe('featureflags', () => { }) it('onFeatureFlags callback should be called immediately if feature flags were loaded', () => { - featureFlags.instance.decideEndpointWasHit = true + featureFlags._hasLoadedFlags = true let called = false featureFlags.onFeatureFlags(() => (called = true)) expect(called).toEqual(true) @@ -324,7 +324,7 @@ describe('featureflags', () => { }) it('onFeatureFlags should not return flags that are off', () => { - featureFlags.instance.decideEndpointWasHit = true + featureFlags._hasLoadedFlags = true let _flags = [] let _variants = {} featureFlags.onFeatureFlags((flags, variants) => { diff --git a/src/decide.ts b/src/decide.ts index dfac7e789..a8212c031 100644 --- a/src/decide.ts +++ b/src/decide.ts @@ -7,10 +7,7 @@ import { assignableWindow, document } from './utils/globals' const logger = createLogger('[Decide]') export class Decide { - constructor(private readonly instance: PostHog) { - // don't need to wait for `decide` to return if flags were provided on initialisation - this.instance.decideEndpointWasHit = this.instance._hasBootstrappedFeatureFlags() - } + constructor(private readonly instance: PostHog) {} private _loadRemoteConfigJs(cb: (config?: RemoteConfig) => void): void { if (assignableWindow.__PosthogExtensions__?.loadExternalDependency) { diff --git a/src/posthog-core.ts b/src/posthog-core.ts index da63cf58c..31b9b13ce 100644 --- a/src/posthog-core.ts +++ b/src/posthog-core.ts @@ -275,7 +275,6 @@ export class PostHog { _triggered_notifs: any compression?: Compression __request_queue: QueuedRequestOptions[] - decideEndpointWasHit: boolean analyticsDefaultEndpoint: string version = Config.LIB_VERSION _initialPersonProfilesConfig: 'always' | 'never' | 'identified_only' | null @@ -285,6 +284,11 @@ export class PostHog { private _internalEventEmitter = new SimpleEventEmitter() + // Legacy property to support existing usage + public get decideEndpointWasHit(): boolean { + return this.featureFlags._decideEndpointWasHit + } + /** DEPRECATED: We keep this to support existing usage but now one should just call .setPersonProperties */ people: { set: (prop: string | Properties, to?: string, callback?: RequestCallback) => void @@ -294,7 +298,6 @@ export class PostHog { constructor() { this.config = defaultConfig() - this.decideEndpointWasHit = false this.SentryIntegration = SentryIntegration this.sentryIntegration = (options?: SentryIntegrationOptions) => sentryIntegration(this, options) this.__request_queue = [] diff --git a/src/posthog-featureflags.ts b/src/posthog-featureflags.ts index ad0f1d54b..8a9e48f76 100644 --- a/src/posthog-featureflags.ts +++ b/src/posthog-featureflags.ts @@ -85,10 +85,12 @@ export class PostHogFeatureFlags { _override_warning: boolean = false featureFlagEventHandlers: FeatureFlagsCallback[] $anon_distinct_id: string | undefined + _decideEndpointWasHit: boolean = false private _requestInFlight: boolean = false private _reloadingDisabled: boolean = false private _additionalReloadRequested: boolean = false private _reloadDebouncer?: any + private _hasLoadedFlags: boolean = false constructor(private instance: PostHog) { this.featureFlagEventHandlers = [] @@ -135,8 +137,9 @@ export class PostHogFeatureFlags { * 2. Delay a few milliseconds after each reloadFeatureFlags call to batch subsequent changes together */ reloadFeatureFlags(): void { - if (this._reloadingDisabled) { + if (this._reloadingDisabled || this.instance.config.advanced_disable_feature_flags) { // If reloading has been explicitly disabled then we don't want to do anything + // Or if feature flags are disabled return } @@ -229,7 +232,7 @@ export class PostHogFeatureFlags { * @param {Object|String} options (optional) If {send_event: false}, we won't send an $feature_flag_call event to PostHog. */ getFeatureFlag(key: string, options: { send_event?: boolean } = {}): boolean | string | undefined { - if (!this.instance.decideEndpointWasHit && !(this.getFlags() && this.getFlags().length > 0)) { + if (!this._hasLoadedFlags && !(this.getFlags() && this.getFlags().length > 0)) { logger.warn('getFeatureFlag for key "' + key + '" failed. Feature flags didn\'t load in time.') return undefined } @@ -268,7 +271,7 @@ export class PostHogFeatureFlags { * @param {Object|String} options (optional) If {send_event: false}, we won't send an $feature_flag_call event to PostHog. */ isFeatureEnabled(key: string, options: { send_event?: boolean } = {}): boolean | undefined { - if (!this.instance.decideEndpointWasHit && !(this.getFlags() && this.getFlags().length > 0)) { + if (!this._hasLoadedFlags && !(this.getFlags() && this.getFlags().length > 0)) { logger.warn('isFeatureEnabled for key "' + key + '" failed. Feature flags didn\'t load in time.') return undefined } @@ -287,7 +290,8 @@ export class PostHogFeatureFlags { if (!this.instance.persistence) { return } - this.instance.decideEndpointWasHit = true + this._hasLoadedFlags = true + const currentFlags = this.getFlagVariants() const currentFlagPayloads = this.getFlagPayloads() parseFeatureFlagDecideResponse(response, this.instance.persistence, currentFlags, currentFlagPayloads) @@ -341,7 +345,7 @@ export class PostHogFeatureFlags { */ onFeatureFlags(callback: FeatureFlagsCallback): () => void { this.addFeatureFlagsHandler(callback) - if (this.instance.decideEndpointWasHit) { + if (this._hasLoadedFlags) { const { flags, flagVariants } = this._prepareFeatureFlagsForCallbacks() callback(flags, flagVariants) } From 7812a43584ce5ae11bee7a2cebf08f28c6b3281a Mon Sep 17 00:00:00 2001 From: Ben White Date: Thu, 12 Dec 2024 09:46:05 +0100 Subject: [PATCH 03/13] Fix --- src/decide.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/decide.ts b/src/decide.ts index a8212c031..cd8d6bb6e 100644 --- a/src/decide.ts +++ b/src/decide.ts @@ -98,6 +98,8 @@ export class Decide { this.instance._onRemoteConfig(config) if (config.hasFeatureFlags !== false && !fromDecide) { + // TODO: It's possible that some other code path called reload flags - we should check if this is the case + logger.info('hasFeatureFlags is true, reloading flags') // If the config has feature flags, we need to call decide to get the feature flags // This completely separates it from the config logic which is good in terms of separation of concerns From b636d87910ca0c12514de224d07329e55f261d97 Mon Sep 17 00:00:00 2001 From: Ben White Date: Thu, 12 Dec 2024 09:54:24 +0100 Subject: [PATCH 04/13] Fix --- src/__tests__/posthog-core.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/src/__tests__/posthog-core.ts b/src/__tests__/posthog-core.ts index 744ad5df8..2ef22b6ee 100644 --- a/src/__tests__/posthog-core.ts +++ b/src/__tests__/posthog-core.ts @@ -12,7 +12,6 @@ import { PostHogPersistence } from '../posthog-persistence' import { SessionIdManager } from '../sessionid' import { RequestQueue } from '../request-queue' import { SessionRecording } from '../extensions/replay/sessionrecording' -import { PostHogFeatureFlags } from '../posthog-featureflags' describe('posthog core', () => { const baseUTCDateTime = new Date(Date.UTC(2020, 0, 1, 0, 0, 0)) From e6df1514b17ba2bd6478c6b90901b4467a334559 Mon Sep 17 00:00:00 2001 From: Ben White Date: Thu, 12 Dec 2024 10:18:35 +0100 Subject: [PATCH 05/13] Fix tests --- src/__tests__/decide.test.ts | 6 +++--- src/decide.ts | 12 +++++------- src/posthog-core.ts | 4 ++-- src/posthog-featureflags.ts | 20 ++++++++++++++++++-- 4 files changed, 28 insertions(+), 14 deletions(-) diff --git a/src/__tests__/decide.test.ts b/src/__tests__/decide.test.ts index 1e9c95b7e..7f0d71c63 100644 --- a/src/__tests__/decide.test.ts +++ b/src/__tests__/decide.test.ts @@ -35,7 +35,7 @@ describe('Decide', () => { _send_request: jest.fn().mockImplementation(({ callback }) => callback?.({ config: {} })), featureFlags: { resetRequestQueue: jest.fn(), - reloadFeatureFlags: jest.fn(), + ensureFlagsLoaded: jest.fn(), receivedFeatureFlags: jest.fn(), setReloadingPaused: jest.fn(), _callDecideEndpoint: jest.fn(), @@ -212,9 +212,9 @@ describe('Decide', () => { new Decide(posthog).call() if (shouldReload) { - expect(posthog.featureFlags.reloadFeatureFlags).toHaveBeenCalled() + expect(posthog.featureFlags.ensureFlagsLoaded).toHaveBeenCalled() } else { - expect(posthog.featureFlags.reloadFeatureFlags).not.toHaveBeenCalled() + expect(posthog.featureFlags.ensureFlagsLoaded).not.toHaveBeenCalled() } }) }) diff --git a/src/decide.ts b/src/decide.ts index cd8d6bb6e..eaee69972 100644 --- a/src/decide.ts +++ b/src/decide.ts @@ -77,11 +77,11 @@ export class Decide { this.instance.config.advanced_disable_feature_flags_on_first_load || undefined, }, - callback: (response) => this.onRemoteConfig(response, true), + callback: (response) => this.onRemoteConfig(response), }) } - private onRemoteConfig(config?: RemoteConfig, fromDecide?: boolean): void { + private onRemoteConfig(config?: RemoteConfig): void { // NOTE: Once this is rolled out we will remove the "decide" related code above. Until then the code duplication is fine. if (!config) { logger.error('Failed to fetch remote config from PostHog.') @@ -97,13 +97,11 @@ export class Decide { this.instance._onRemoteConfig(config) - if (config.hasFeatureFlags !== false && !fromDecide) { - // TODO: It's possible that some other code path called reload flags - we should check if this is the case - - logger.info('hasFeatureFlags is true, reloading flags') + // We only need to reload if we haven't already loaded the flags or if the request is in flight + if (config.hasFeatureFlags !== false) { // If the config has feature flags, we need to call decide to get the feature flags // This completely separates it from the config logic which is good in terms of separation of concerns - this.instance.featureFlags.reloadFeatureFlags() + this.instance.featureFlags.ensureFlagsLoaded() } } } diff --git a/src/posthog-core.ts b/src/posthog-core.ts index 31b9b13ce..cbcc70e63 100644 --- a/src/posthog-core.ts +++ b/src/posthog-core.ts @@ -284,9 +284,9 @@ export class PostHog { private _internalEventEmitter = new SimpleEventEmitter() - // Legacy property to support existing usage + // Legacy property to support existing usage - this isn't technically correct but it's what it has always been - a proxy for flags being loaded public get decideEndpointWasHit(): boolean { - return this.featureFlags._decideEndpointWasHit + return this.featureFlags.hasLoadedFlags } /** DEPRECATED: We keep this to support existing usage but now one should just call .setPersonProperties */ diff --git a/src/posthog-featureflags.ts b/src/posthog-featureflags.ts index 8a9e48f76..fbfdb5acf 100644 --- a/src/posthog-featureflags.ts +++ b/src/posthog-featureflags.ts @@ -85,17 +85,20 @@ export class PostHogFeatureFlags { _override_warning: boolean = false featureFlagEventHandlers: FeatureFlagsCallback[] $anon_distinct_id: string | undefined - _decideEndpointWasHit: boolean = false + private _hasLoadedFlags: boolean = false private _requestInFlight: boolean = false private _reloadingDisabled: boolean = false private _additionalReloadRequested: boolean = false private _reloadDebouncer?: any - private _hasLoadedFlags: boolean = false constructor(private instance: PostHog) { this.featureFlagEventHandlers = [] } + get hasLoadedFlags(): boolean { + return this._hasLoadedFlags + } + getFlags(): string[] { return Object.keys(this.getFlagVariants()) } @@ -155,6 +158,15 @@ export class PostHogFeatureFlags { }, 5) } + ensureFlagsLoaded(): void { + if (this._hasLoadedFlags || this._requestInFlight) { + // If we are or have already loaded the flags then we don't want to do anything + return + } + + this.reloadFeatureFlags() + } + setAnonymousDistinctId(anon_distinct_id: string): void { this.$anon_distinct_id = anon_distinct_id } @@ -168,6 +180,10 @@ export class PostHogFeatureFlags { * be for flags and can eventually be replaced wiht the new flags endpoint */ _callDecideEndpoint(options?: { data: Record; callback: (response: DecideResponse) => void }): void { + if (this.instance.config.advanced_disable_decide) { + // The way this is documented is essentially used to refuse to ever call the decide endpoint. + return + } if (this._requestInFlight) { this._additionalReloadRequested = true return From c49d42f12df4944f4f5dd9408c37560bdfd751ed Mon Sep 17 00:00:00 2001 From: Ben White Date: Thu, 12 Dec 2024 12:53:53 +0100 Subject: [PATCH 06/13] Fixes all round --- src/__tests__/decide.test.ts | 221 -------------------- src/__tests__/featureflags.test.ts | 94 ++++++++- src/__tests__/helpers/posthog-instance.ts | 1 + src/__tests__/personProcessing.test.ts | 1 + src/__tests__/posthog-core.identify.test.ts | 3 +- src/__tests__/posthog-core.loaded.ts | 93 ++++++-- src/__tests__/remote-config.test.ts | 108 ++++++++++ src/posthog-core.ts | 13 +- src/posthog-featureflags.ts | 44 +++- src/{decide.ts => remote-config.ts} | 72 +++---- 10 files changed, 352 insertions(+), 298 deletions(-) delete mode 100644 src/__tests__/decide.test.ts create mode 100644 src/__tests__/remote-config.test.ts rename src/{decide.ts => remote-config.ts} (52%) diff --git a/src/__tests__/decide.test.ts b/src/__tests__/decide.test.ts deleted file mode 100644 index 7f0d71c63..000000000 --- a/src/__tests__/decide.test.ts +++ /dev/null @@ -1,221 +0,0 @@ -import { Decide } from '../decide' -import { PostHogPersistence } from '../posthog-persistence' -import { RequestRouter } from '../utils/request-router' -import { PostHog } from '../posthog-core' -import { PostHogConfig, Properties, RemoteConfig } from '../types' -import '../entrypoints/external-scripts-loader' -import { assignableWindow } from '../utils/globals' - -describe('Decide', () => { - let posthog: PostHog - - beforeEach(() => { - // clean the JSDOM to prevent interdependencies between tests - - const defaultConfig: Partial = { - token: 'testtoken', - api_host: 'https://test.com', - persistence: 'memory', - } - - document.body.innerHTML = '' - document.head.innerHTML = '' - jest.spyOn(window.console, 'error').mockImplementation() - - 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], - capture: jest.fn(), - _addCaptureHook: jest.fn(), - _onRemoteConfig: jest.fn(), - get_distinct_id: jest.fn().mockImplementation(() => 'distinctid'), - _send_request: jest.fn().mockImplementation(({ callback }) => callback?.({ config: {} })), - featureFlags: { - resetRequestQueue: jest.fn(), - ensureFlagsLoaded: jest.fn(), - receivedFeatureFlags: jest.fn(), - setReloadingPaused: jest.fn(), - _callDecideEndpoint: jest.fn(), - }, - requestRouter: new RequestRouter({ config: defaultConfig } as unknown as PostHog), - _hasBootstrappedFeatureFlags: jest.fn(), - getGroups: () => ({ organization: '5' }), - } as unknown as PostHog - }) - - describe('constructor', () => { - it('should call _callDecideEndpoint on constructor', () => { - new Decide(posthog).call() - - expect(posthog.featureFlags._callDecideEndpoint).toHaveBeenCalledTimes(1) - expect(posthog.featureFlags._callDecideEndpoint).toHaveBeenLastCalledWith({ - data: { - disable_flags: undefined, - }, - callback: expect.any(Function), - }) - }) - - it('should not call _callDecideEndpoint on constructor if advanced_disable_decide', () => { - posthog.config.advanced_disable_decide = true - new Decide(posthog).call() - - expect(posthog.featureFlags._callDecideEndpoint).toHaveBeenCalledTimes(0) - }) - - it('should call _callDecideEndpoint with disable_flags true if advanced_disable_feature_flags is set', () => { - console.log('posthog.config', posthog.config) - posthog.config.advanced_disable_feature_flags = true - posthog.config.advanced_disable_feature_flags_on_first_load = false - - new Decide(posthog).call() - expect(posthog.featureFlags._callDecideEndpoint).toHaveBeenCalledTimes(1) - expect(posthog.featureFlags._callDecideEndpoint).toHaveBeenLastCalledWith({ - data: { - disable_flags: true, - }, - callback: expect.any(Function), - }) - }) - - it('should call _callDecideEndpoint with disable_flags true if advanced_disable_feature_flags_on_first_load is set', () => { - posthog.config.advanced_disable_feature_flags = false - posthog.config.advanced_disable_feature_flags_on_first_load = true - - new Decide(posthog).call() - expect(posthog.featureFlags._callDecideEndpoint).toHaveBeenCalledTimes(1) - expect(posthog.featureFlags._callDecideEndpoint).toHaveBeenLastCalledWith({ - data: { - disable_flags: true, - }, - callback: expect.any(Function), - }) - }) - }) - - // describe('parseDecideResponse', () => { - - // it('Make sure receivedFeatureFlags is called with errors if the decide response fails', () => { - // ;(window as any).POSTHOG_DEBUG = true - - // subject(undefined as unknown as DecideResponse) - - // expect(posthog.featureFlags.receivedFeatureFlags).toHaveBeenCalledWith({}, true) - // expect(console.error).toHaveBeenCalledWith( - // '[PostHog.js] [Decide]', - // 'Failed to fetch feature flags from PostHog.' - // ) - // }) - - // it('Make sure receivedFeatureFlags is not called if advanced_disable_feature_flags_on_first_load is set', () => { - // posthog.config = { - // api_host: 'https://test.com', - // token: 'testtoken', - // persistence: 'memory', - // advanced_disable_feature_flags_on_first_load: true, - // } as PostHogConfig - - // const decideResponse = { - // featureFlags: { 'test-flag': true }, - // } as unknown as DecideResponse - // subject(decideResponse) - - // expect(posthog._onRemoteConfig).toHaveBeenCalledWith(decideResponse) - // expect(posthog.featureFlags.receivedFeatureFlags).not.toHaveBeenCalled() - // }) - - // it('Make sure receivedFeatureFlags is not called if advanced_disable_feature_flags is set', () => { - // posthog.config = { - // api_host: 'https://test.com', - // token: 'testtoken', - // persistence: 'memory', - // advanced_disable_feature_flags: true, - // } as PostHogConfig - - // const decideResponse = { - // featureFlags: { 'test-flag': true }, - // } as unknown as DecideResponse - // subject(decideResponse) - - // expect(posthog._onRemoteConfig).toHaveBeenCalledWith(decideResponse) - // expect(posthog.featureFlags.receivedFeatureFlags).not.toHaveBeenCalled() - // }) - // }) - - describe('remote config', () => { - const config = { surveys: true } as RemoteConfig - - beforeEach(() => { - posthog.config.__preview_remote_config = true - assignableWindow._POSTHOG_CONFIG = undefined - assignableWindow.POSTHOG_DEBUG = true - - assignableWindow.__PosthogExtensions__.loadExternalDependency = jest.fn( - (_ph: PostHog, _name: string, cb: (err?: any) => void) => { - assignableWindow._POSTHOG_CONFIG = config as RemoteConfig - cb() - } - ) - - posthog._send_request = jest.fn().mockImplementation(({ callback }) => callback?.({ json: config })) - }) - - it('properly pulls from the window and uses it if set', () => { - assignableWindow._POSTHOG_CONFIG = config as RemoteConfig - new Decide(posthog).call() - - expect(assignableWindow.__PosthogExtensions__.loadExternalDependency).not.toHaveBeenCalled() - expect(posthog._send_request).not.toHaveBeenCalled() - - expect(posthog._onRemoteConfig).toHaveBeenCalledWith(config) - }) - - it('loads the script if window config not set', () => { - new Decide(posthog).call() - - expect(assignableWindow.__PosthogExtensions__.loadExternalDependency).toHaveBeenCalledWith( - posthog, - 'remote-config', - expect.any(Function) - ) - expect(posthog._send_request).not.toHaveBeenCalled() - expect(posthog._onRemoteConfig).toHaveBeenCalledWith(config) - }) - - it('loads the json if window config not set and js failed', () => { - assignableWindow.__PosthogExtensions__.loadExternalDependency = jest.fn( - (_ph: PostHog, _name: string, cb: (err?: any) => void) => { - cb() - } - ) - - new Decide(posthog).call() - - expect(assignableWindow.__PosthogExtensions__.loadExternalDependency).toHaveBeenCalled() - expect(posthog._send_request).toHaveBeenCalledWith({ - method: 'GET', - url: 'https://test.com/array/testtoken/config', - callback: expect.any(Function), - }) - expect(posthog._onRemoteConfig).toHaveBeenCalledWith(config) - }) - - it.each([ - [true, true], - [false, false], - [undefined, true], - ])('conditionally reloads feature flags - hasFlags: %s, shouldReload: %s', (hasFeatureFlags, shouldReload) => { - assignableWindow._POSTHOG_CONFIG = { hasFeatureFlags } as RemoteConfig - new Decide(posthog).call() - - if (shouldReload) { - expect(posthog.featureFlags.ensureFlagsLoaded).toHaveBeenCalled() - } else { - expect(posthog.featureFlags.ensureFlagsLoaded).not.toHaveBeenCalled() - } - }) - }) -}) diff --git a/src/__tests__/featureflags.test.ts b/src/__tests__/featureflags.test.ts index ccb3ed071..6fb0a09dd 100644 --- a/src/__tests__/featureflags.test.ts +++ b/src/__tests__/featureflags.test.ts @@ -22,7 +22,7 @@ describe('featureflags', () => { beforeEach(() => { instance = { - config, + config: { ...config }, get_distinct_id: () => 'blah id', getGroups: () => {}, persistence: new PostHogPersistence(config), @@ -38,6 +38,7 @@ describe('featureflags', () => { json: {}, }) ), + _onRemoteConfig: jest.fn(), reloadFeatureFlags: () => featureFlags.reloadFeatureFlags(), } @@ -271,6 +272,97 @@ describe('featureflags', () => { }) }) + describe('decide()', () => { + it('should not call decide if advanced_disable_decide is true', () => { + instance.config.advanced_disable_decide = true + featureFlags.decide() + + expect(instance._send_request).toHaveBeenCalledTimes(0) + }) + + it('should call decide', () => { + featureFlags.decide() + + expect(instance._send_request).toHaveBeenCalledTimes(1) + expect(instance._send_request.mock.calls[0][0]).toMatchObject({ + data: { + disable_flags: undefined, + }, + }) + + jest.runOnlyPendingTimers() + expect(instance._send_request).toHaveBeenCalledTimes(1) + }) + + it('should call decide with flags disabled if set', () => { + instance.config.advanced_disable_feature_flags_on_first_load = true + featureFlags.decide() + + expect(instance._send_request).toHaveBeenCalledTimes(1) + expect(instance._send_request.mock.calls[0][0]).toMatchObject({ + data: { + disable_flags: true, + }, + }) + }) + + it('should call decide with flags disabled if set generally', () => { + instance.config.advanced_disable_feature_flags = true + featureFlags.decide() + + expect(instance._send_request).toHaveBeenCalledTimes(1) + expect(instance._send_request.mock.calls[0][0]).toMatchObject({ + data: { + disable_flags: true, + }, + }) + }) + + it('should call decide once even if reload called before', () => { + featureFlags.reloadFeatureFlags() + featureFlags.decide() + + expect(instance._send_request).toHaveBeenCalledTimes(1) + expect(instance._send_request.mock.calls[0][0]).toMatchObject({ + data: { + disable_flags: false, + }, + }) + + jest.runOnlyPendingTimers() + expect(instance._send_request).toHaveBeenCalledTimes(1) + }) + + it('should not disable flags if reload was called on decide', () => { + instance.config.advanced_disable_feature_flags_on_first_load = true + featureFlags.reloadFeatureFlags() + featureFlags.decide() + + expect(instance._send_request).toHaveBeenCalledTimes(1) + expect(instance._send_request.mock.calls[0][0]).toMatchObject({ + data: { + disable_flags: false, + }, + }) + + jest.runOnlyPendingTimers() + expect(instance._send_request).toHaveBeenCalledTimes(1) + }) + + it('should always disable flags if set', () => { + instance.config.advanced_disable_feature_flags = true + featureFlags.reloadFeatureFlags() + featureFlags.decide() + + expect(instance._send_request).toHaveBeenCalledTimes(1) + expect(instance._send_request.mock.calls[0][0]).toMatchObject({ + data: { + disable_flags: true, + }, + }) + }) + }) + describe('onFeatureFlags', () => { beforeEach(() => { instance._send_request = jest.fn().mockImplementation(({ callback }) => diff --git a/src/__tests__/helpers/posthog-instance.ts b/src/__tests__/helpers/posthog-instance.ts index 5f74a7c59..7c119f5db 100644 --- a/src/__tests__/helpers/posthog-instance.ts +++ b/src/__tests__/helpers/posthog-instance.ts @@ -13,6 +13,7 @@ export const createPosthogInstance = async ( // written, we first create an instance, then call init on it which then // creates another instance. const posthog = new PostHog() + // eslint-disable-next-line compat/compat return await new Promise((resolve) => posthog.init( diff --git a/src/__tests__/personProcessing.test.ts b/src/__tests__/personProcessing.test.ts index 058dedb42..a384029b9 100644 --- a/src/__tests__/personProcessing.test.ts +++ b/src/__tests__/personProcessing.test.ts @@ -44,6 +44,7 @@ jest.mock('../utils/globals', () => { document: { ...orig.document, createElement: (...args: any[]) => orig.document.createElement(...args), + body: {}, get referrer() { return mockReferrerGetter() }, diff --git a/src/__tests__/posthog-core.identify.test.ts b/src/__tests__/posthog-core.identify.test.ts index 96beace88..885ec25f9 100644 --- a/src/__tests__/posthog-core.identify.test.ts +++ b/src/__tests__/posthog-core.identify.test.ts @@ -3,8 +3,6 @@ import { uuidv7 } from '../uuidv7' import { createPosthogInstance, defaultPostHog } from './helpers/posthog-instance' import { PostHog } from '../posthog-core' -jest.mock('../decide') - describe('identify()', () => { let instance: PostHog let beforeSendMock: jest.Mock @@ -19,6 +17,7 @@ describe('identify()', () => { }, uuidv7() ) + instance = Object.assign(posthog, { register: jest.fn(), featureFlags: { diff --git a/src/__tests__/posthog-core.loaded.ts b/src/__tests__/posthog-core.loaded.ts index 88b7b5535..d866558bc 100644 --- a/src/__tests__/posthog-core.loaded.ts +++ b/src/__tests__/posthog-core.loaded.ts @@ -8,13 +8,10 @@ jest.useFakeTimers() describe('loaded() with flags', () => { let instance: PostHog - beforeAll(() => { - jest.unmock('../decide') - }) - const createPosthog = async (config?: Partial) => { const posthog = await createPosthogInstance(uuidv7(), { api_host: 'https://app.posthog.com', + disable_compression: true, ...config, loaded: (ph) => { ph.capture = jest.fn() @@ -26,33 +23,93 @@ describe('loaded() with flags', () => { jest.spyOn(ph.featureFlags, 'reloadFeatureFlags') jest.spyOn(ph.featureFlags, '_callDecideEndpoint') - ph.group('org', 'bazinga', { name: 'Shelly' }) - setTimeout(() => { - ph.group('org', 'bazinga2', { name: 'Shelly' }) - }, 100) + config?.loaded?.(ph) }, }) return posthog } - beforeEach(async () => { - instance = await createPosthog() - }) + describe('flag reloading', () => { + it('only calls decide once whilst loading', async () => { + instance = await createPosthog({ + loaded: (ph) => { + ph.group('org', 'bazinga', { name: 'Shelly' }) + }, + }) - describe('toggling flag reloading', () => { - it('doesnt call flags while initial load is happening', () => { - expect(instance.featureFlags.setGroupPropertiesForFlags).toHaveBeenCalled() // loaded ph.group() calls setGroupPropertiesForFlags - expect(instance.featureFlags.reloadFeatureFlags).toHaveBeenCalledTimes(1) // 1 call from load + group debounced + expect(instance._send_request).toHaveBeenCalledTimes(1) + + expect(instance._send_request.mock.calls[0][0]).toMatchObject({ + url: 'https://us.i.posthog.com/decide/?v=3', + data: { + groups: { org: 'bazinga' }, + }, + }) + jest.runOnlyPendingTimers() // Once for callback + jest.runOnlyPendingTimers() // Once for potential debounce + expect(instance._send_request).toHaveBeenCalledTimes(1) + }) + + it('does add follow up call due to group change', async () => { + instance = await createPosthog({ + loaded: (ph) => { + ph.group('org', 'bazinga', { name: 'Shelly' }) + setTimeout(() => { + ph.group('org', 'bazinga2', { name: 'Shelly' }) + }, 100) + }, + }) expect(instance.featureFlags._callDecideEndpoint).toHaveBeenCalledTimes(1) expect(instance._send_request).toHaveBeenCalledTimes(1) - jest.runOnlyPendingTimers() + expect(instance._send_request.mock.calls[0][0]).toMatchObject({ + url: 'https://us.i.posthog.com/decide/?v=3', + data: { + groups: { org: 'bazinga' }, + }, + }) - expect(instance.featureFlags.reloadFeatureFlags).toHaveBeenCalledTimes(2) // Additional call for groups change - expect(instance.featureFlags._callDecideEndpoint).toHaveBeenCalledTimes(2) + jest.runOnlyPendingTimers() // Once for callback + jest.runOnlyPendingTimers() // Once for potential debounce + expect(instance.featureFlags._callDecideEndpoint).toHaveBeenCalledTimes(2) expect(instance._send_request).toHaveBeenCalledTimes(2) + + expect(instance._send_request.mock.calls[1][0]).toMatchObject({ + url: 'https://us.i.posthog.com/decide/?v=3', + data: { + groups: { org: 'bazinga2' }, + }, + }) + }) + + it('does call decide with a request for flags if called directly (via groups) even if disabled for first load', async () => { + instance = await createPosthog({ + advanced_disable_feature_flags_on_first_load: true, + loaded: (ph) => { + ph.group('org', 'bazinga', { name: 'Shelly' }) + }, + }) + + expect(instance.config.advanced_disable_feature_flags_on_first_load).toBe(true) + + expect(instance.featureFlags._callDecideEndpoint).toHaveBeenCalledTimes(1) + expect(instance._send_request).toHaveBeenCalledTimes(1) + + expect(instance._send_request.mock.calls[0][0]).toMatchObject({ + url: 'https://us.i.posthog.com/decide/?v=3', + data: { + groups: { org: 'bazinga' }, + disable_flags: false, + }, + }) + + jest.runOnlyPendingTimers() // Once for callback + jest.runOnlyPendingTimers() // Once for potential debounce + + expect(instance.featureFlags._callDecideEndpoint).toHaveBeenCalledTimes(1) + expect(instance._send_request).toHaveBeenCalledTimes(1) }) }) }) diff --git a/src/__tests__/remote-config.test.ts b/src/__tests__/remote-config.test.ts new file mode 100644 index 000000000..473bb1d0a --- /dev/null +++ b/src/__tests__/remote-config.test.ts @@ -0,0 +1,108 @@ +import { RemoteConfigLoader } from '../remote-config' +import { RequestRouter } from '../utils/request-router' +import { PostHog } from '../posthog-core' +import { PostHogConfig, RemoteConfig } from '../types' +import '../entrypoints/external-scripts-loader' +import { assignableWindow } from '../utils/globals' + +describe('RemoteConfigLoader', () => { + let posthog: PostHog + + beforeEach(() => { + // clean the JSDOM to prevent interdependencies between tests + + const defaultConfig: Partial = { + token: 'testtoken', + api_host: 'https://test.com', + persistence: 'memory', + } + + document.body.innerHTML = '' + document.head.innerHTML = '' + jest.spyOn(window.console, 'error').mockImplementation() + + posthog = { + config: { ...defaultConfig }, + _onRemoteConfig: jest.fn(), + _send_request: jest.fn().mockImplementation(({ callback }) => callback?.({ config: {} })), + featureFlags: { + ensureFlagsLoaded: jest.fn(), + }, + requestRouter: new RequestRouter({ config: defaultConfig } as unknown as PostHog), + } as unknown as PostHog + }) + + describe('remote config', () => { + const config = { surveys: true } as RemoteConfig + + beforeEach(() => { + posthog.config.__preview_remote_config = true + assignableWindow._POSTHOG_CONFIG = undefined + assignableWindow.POSTHOG_DEBUG = true + + assignableWindow.__PosthogExtensions__.loadExternalDependency = jest.fn( + (_ph: PostHog, _name: string, cb: (err?: any) => void) => { + assignableWindow._POSTHOG_CONFIG = config as RemoteConfig + cb() + } + ) + + posthog._send_request = jest.fn().mockImplementation(({ callback }) => callback?.({ json: config })) + }) + + it('properly pulls from the window and uses it if set', () => { + assignableWindow._POSTHOG_CONFIG = config as RemoteConfig + new RemoteConfigLoader(posthog).load() + + expect(assignableWindow.__PosthogExtensions__.loadExternalDependency).not.toHaveBeenCalled() + expect(posthog._send_request).not.toHaveBeenCalled() + + expect(posthog._onRemoteConfig).toHaveBeenCalledWith(config) + }) + + it('loads the script if window config not set', () => { + new RemoteConfigLoader(posthog).load() + + expect(assignableWindow.__PosthogExtensions__.loadExternalDependency).toHaveBeenCalledWith( + posthog, + 'remote-config', + expect.any(Function) + ) + expect(posthog._send_request).not.toHaveBeenCalled() + expect(posthog._onRemoteConfig).toHaveBeenCalledWith(config) + }) + + it('loads the json if window config not set and js failed', () => { + assignableWindow.__PosthogExtensions__.loadExternalDependency = jest.fn( + (_ph: PostHog, _name: string, cb: (err?: any) => void) => { + cb() + } + ) + + new RemoteConfigLoader(posthog).load() + + expect(assignableWindow.__PosthogExtensions__.loadExternalDependency).toHaveBeenCalled() + expect(posthog._send_request).toHaveBeenCalledWith({ + method: 'GET', + url: 'https://test.com/array/testtoken/config', + callback: expect.any(Function), + }) + expect(posthog._onRemoteConfig).toHaveBeenCalledWith(config) + }) + + it.each([ + [true, true], + [false, false], + [undefined, true], + ])('conditionally reloads feature flags - hasFlags: %s, shouldReload: %s', (hasFeatureFlags, shouldReload) => { + assignableWindow._POSTHOG_CONFIG = { hasFeatureFlags } as RemoteConfig + new RemoteConfigLoader(posthog).load() + + if (shouldReload) { + expect(posthog.featureFlags.ensureFlagsLoaded).toHaveBeenCalled() + } else { + expect(posthog.featureFlags.ensureFlagsLoaded).not.toHaveBeenCalled() + } + }) + }) +}) diff --git a/src/posthog-core.ts b/src/posthog-core.ts index cbcc70e63..34d523e4d 100644 --- a/src/posthog-core.ts +++ b/src/posthog-core.ts @@ -21,7 +21,7 @@ import { ENABLE_PERSON_PROCESSING, } from './constants' import { SessionRecording } from './extensions/replay/sessionrecording' -import { Decide } from './decide' +import { RemoteConfigLoader } from './remote-config' import { Toolbar } from './extensions/toolbar' import { localStore } from './storage' import { RequestQueue } from './request-queue' @@ -548,6 +548,14 @@ export class PostHog { } _onRemoteConfig(config: RemoteConfig) { + if (!(document && document.body)) { + logger.info('document not ready yet, trying again in 500 milliseconds...') + setTimeout(() => { + this._onRemoteConfig(config) + }, 500) + return + } + this.compression = undefined if (config.supportedCompression && !this.config.disable_compression) { this.compression = includes(config['supportedCompression'], Compression.GZipJS) @@ -599,7 +607,8 @@ export class PostHog { }, 1) } - new Decide(this).call() + new RemoteConfigLoader(this).load() + this.featureFlags.decide() } _start_queue_if_opted_in(): void { diff --git a/src/posthog-featureflags.ts b/src/posthog-featureflags.ts index fbfdb5acf..9c801fa01 100644 --- a/src/posthog-featureflags.ts +++ b/src/posthog-featureflags.ts @@ -90,11 +90,32 @@ export class PostHogFeatureFlags { private _reloadingDisabled: boolean = false private _additionalReloadRequested: boolean = false private _reloadDebouncer?: any + private _decideCalled: boolean = false constructor(private instance: PostHog) { this.featureFlagEventHandlers = [] } + decide(): void { + if (this.instance.config.__preview_remote_config) { + // If remote config is enabled we don't call decide and we mark it as called so that we don't simulate it + this._decideCalled = true + return + } + + // TRICKY: We want to disable flags if we don't have a queued reload, and one of the settings exist for disabling on first load + const disableFlags = + !this._reloadDebouncer && + (this.instance.config.advanced_disable_feature_flags || + this.instance.config.advanced_disable_feature_flags_on_first_load) + + this._callDecideEndpoint({ + data: { + disable_flags: disableFlags, + }, + }) + } + get hasLoadedFlags(): boolean { return this._hasLoadedFlags } @@ -153,13 +174,17 @@ export class PostHogFeatureFlags { // Debounce multiple calls on the same tick this._reloadDebouncer = setTimeout(() => { - this._reloadDebouncer = undefined this._callDecideEndpoint() }, 5) } + private clearDebouncer(): void { + clearTimeout(this._reloadDebouncer) + this._reloadDebouncer = undefined + } + ensureFlagsLoaded(): void { - if (this._hasLoadedFlags || this._requestInFlight) { + if (this._hasLoadedFlags || this._requestInFlight || this._reloadDebouncer) { // If we are or have already loaded the flags then we don't want to do anything return } @@ -179,7 +204,9 @@ export class PostHogFeatureFlags { * NOTE: This is used both for flags and remote config. Once the RemoteConfig is fully released this will essentially only * be for flags and can eventually be replaced wiht the new flags endpoint */ - _callDecideEndpoint(options?: { data: Record; callback: (response: DecideResponse) => void }): void { + _callDecideEndpoint(options?: { data: Record }): void { + // Ensure we don't have double queued decide requests + this.clearDebouncer() if (this.instance.config.advanced_disable_decide) { // The way this is documented is essentially used to refuse to ever call the decide endpoint. return @@ -208,10 +235,6 @@ export class PostHogFeatureFlags { compression: this.instance.config.disable_compression ? undefined : Compression.Base64, timeout: this.instance.config.feature_flag_request_timeout_ms, callback: (response) => { - this._requestInFlight = false - - options?.callback?.(response.json ?? {}) - let errorsLoading = true if (response.statusCode === 200) { @@ -222,6 +245,13 @@ export class PostHogFeatureFlags { errorsLoading = false } + this._requestInFlight = false + + if (!this._decideCalled) { + this._decideCalled = true + this.instance._onRemoteConfig(response.json ?? {}) + } + if (data.disable_flags) { // If flags are disabled then there is no need to call decide again (flags are the only thing that may change) return diff --git a/src/decide.ts b/src/remote-config.ts similarity index 52% rename from src/decide.ts rename to src/remote-config.ts index eaee69972..11c2a676e 100644 --- a/src/decide.ts +++ b/src/remote-config.ts @@ -2,11 +2,11 @@ import { PostHog } from './posthog-core' import { RemoteConfig } from './types' import { createLogger } from './utils/logger' -import { assignableWindow, document } from './utils/globals' +import { assignableWindow } from './utils/globals' const logger = createLogger('[Decide]') -export class Decide { +export class RemoteConfigLoader { constructor(private readonly instance: PostHog) {} private _loadRemoteConfigJs(cb: (config?: RemoteConfig) => void): void { @@ -30,54 +30,40 @@ export class Decide { }) } - call(): void { + load(): void { // Call decide to get what features are enabled and other settings. // As a reminder, if the /decide endpoint is disabled, feature flags, toolbar, session recording, autocapture, // and compression will not be available. - const disableRemoteCalls = !!this.instance.config.advanced_disable_decide - if (this.instance.config.__preview_remote_config) { - // Attempt 1 - use the pre-loaded config if it came as part of the token-specific array.js - if (assignableWindow._POSTHOG_CONFIG) { - logger.info('Using preloaded remote config', assignableWindow._POSTHOG_CONFIG) - this.onRemoteConfig(assignableWindow._POSTHOG_CONFIG) - return - } - - if (disableRemoteCalls) { - logger.warn('Remote config is disabled. Falling back to local config.') - return - } - - // Attempt 2 - if we have the external deps loader then lets load the script version of the config that includes site apps - this._loadRemoteConfigJs((config) => { - if (!config) { - logger.info('No config found after loading remote JS config. Falling back to JSON.') - // Attempt 3 Load the config json instead of the script - we won't get site apps etc. but we will get the config - this._loadRemoteConfigJSON((config) => { - this.onRemoteConfig(config) - }) - return - } - - this.onRemoteConfig(config) - }) + if (!this.instance.config.__preview_remote_config) { + return + } + // Attempt 1 - use the pre-loaded config if it came as part of the token-specific array.js + if (assignableWindow._POSTHOG_CONFIG) { + logger.info('Using preloaded remote config', assignableWindow._POSTHOG_CONFIG) + this.onRemoteConfig(assignableWindow._POSTHOG_CONFIG) return } - if (disableRemoteCalls) { + if (this.instance.config.advanced_disable_decide) { + // This setting is essentially saying "dont call external APIs" hence we respect it here + logger.warn('Remote config is disabled. Falling back to local config.') return } - this.instance.featureFlags._callDecideEndpoint({ - data: { - disable_flags: - this.instance.config.advanced_disable_feature_flags || - this.instance.config.advanced_disable_feature_flags_on_first_load || - undefined, - }, - callback: (response) => this.onRemoteConfig(response), + // Attempt 2 - if we have the external deps loader then lets load the script version of the config that includes site apps + this._loadRemoteConfigJs((config) => { + if (!config) { + logger.info('No config found after loading remote JS config. Falling back to JSON.') + // Attempt 3 Load the config json instead of the script - we won't get site apps etc. but we will get the config + this._loadRemoteConfigJSON((config) => { + this.onRemoteConfig(config) + }) + return + } + + this.onRemoteConfig(config) }) } @@ -87,14 +73,6 @@ export class Decide { logger.error('Failed to fetch remote config from PostHog.') return } - if (!(document && document.body)) { - logger.info('document not ready yet, trying again in 500 milliseconds...') - setTimeout(() => { - this.onRemoteConfig(config) - }, 500) - return - } - this.instance._onRemoteConfig(config) // We only need to reload if we haven't already loaded the flags or if the request is in flight From 04e5d554a06c3de77de6883d40e9243d91daf2c3 Mon Sep 17 00:00:00 2001 From: Ben White Date: Thu, 12 Dec 2024 12:59:41 +0100 Subject: [PATCH 07/13] fix most tests --- src/__tests__/featureflags.test.ts | 30 +++++----------------------- src/__tests__/posthog-core.loaded.ts | 8 +------- src/posthog-featureflags.ts | 10 ++++++---- 3 files changed, 12 insertions(+), 36 deletions(-) diff --git a/src/__tests__/featureflags.test.ts b/src/__tests__/featureflags.test.ts index 6fb0a09dd..30d89c301 100644 --- a/src/__tests__/featureflags.test.ts +++ b/src/__tests__/featureflags.test.ts @@ -299,11 +299,7 @@ describe('featureflags', () => { featureFlags.decide() expect(instance._send_request).toHaveBeenCalledTimes(1) - expect(instance._send_request.mock.calls[0][0]).toMatchObject({ - data: { - disable_flags: true, - }, - }) + expect(instance._send_request.mock.calls[0][0].data.disable_flags).toBe(true) }) it('should call decide with flags disabled if set generally', () => { @@ -311,11 +307,7 @@ describe('featureflags', () => { featureFlags.decide() expect(instance._send_request).toHaveBeenCalledTimes(1) - expect(instance._send_request.mock.calls[0][0]).toMatchObject({ - data: { - disable_flags: true, - }, - }) + expect(instance._send_request.mock.calls[0][0].data.disable_flags).toBe(true) }) it('should call decide once even if reload called before', () => { @@ -323,11 +315,7 @@ describe('featureflags', () => { featureFlags.decide() expect(instance._send_request).toHaveBeenCalledTimes(1) - expect(instance._send_request.mock.calls[0][0]).toMatchObject({ - data: { - disable_flags: false, - }, - }) + expect(instance._send_request.mock.calls[0][0].data.disable_flags).toBe(undefined) jest.runOnlyPendingTimers() expect(instance._send_request).toHaveBeenCalledTimes(1) @@ -339,11 +327,7 @@ describe('featureflags', () => { featureFlags.decide() expect(instance._send_request).toHaveBeenCalledTimes(1) - expect(instance._send_request.mock.calls[0][0]).toMatchObject({ - data: { - disable_flags: false, - }, - }) + expect(instance._send_request.mock.calls[0][0].data.disable_flags).toBe(undefined) jest.runOnlyPendingTimers() expect(instance._send_request).toHaveBeenCalledTimes(1) @@ -355,11 +339,7 @@ describe('featureflags', () => { featureFlags.decide() expect(instance._send_request).toHaveBeenCalledTimes(1) - expect(instance._send_request.mock.calls[0][0]).toMatchObject({ - data: { - disable_flags: true, - }, - }) + expect(instance._send_request.mock.calls[0][0].data.disable_flags).toBe(true) }) }) diff --git a/src/__tests__/posthog-core.loaded.ts b/src/__tests__/posthog-core.loaded.ts index d866558bc..3ef6c6805 100644 --- a/src/__tests__/posthog-core.loaded.ts +++ b/src/__tests__/posthog-core.loaded.ts @@ -97,13 +97,7 @@ describe('loaded() with flags', () => { expect(instance.featureFlags._callDecideEndpoint).toHaveBeenCalledTimes(1) expect(instance._send_request).toHaveBeenCalledTimes(1) - expect(instance._send_request.mock.calls[0][0]).toMatchObject({ - url: 'https://us.i.posthog.com/decide/?v=3', - data: { - groups: { org: 'bazinga' }, - disable_flags: false, - }, - }) + expect(instance._send_request.mock.calls[0][0].data.disable_flags).toEqual(undefined) jest.runOnlyPendingTimers() // Once for callback jest.runOnlyPendingTimers() // Once for potential debounce diff --git a/src/posthog-featureflags.ts b/src/posthog-featureflags.ts index 9c801fa01..f0f317002 100644 --- a/src/posthog-featureflags.ts +++ b/src/posthog-featureflags.ts @@ -204,7 +204,7 @@ export class PostHogFeatureFlags { * NOTE: This is used both for flags and remote config. Once the RemoteConfig is fully released this will essentially only * be for flags and can eventually be replaced wiht the new flags endpoint */ - _callDecideEndpoint(options?: { data: Record }): void { + _callDecideEndpoint(options?: { disableFlags?: boolean }): void { // Ensure we don't have double queued decide requests this.clearDebouncer() if (this.instance.config.advanced_disable_decide) { @@ -216,15 +216,17 @@ export class PostHogFeatureFlags { return } const token = this.instance.config.token - const data = { + const data: Record = { token: token, distinct_id: this.instance.get_distinct_id(), groups: this.instance.getGroups(), $anon_distinct_id: this.$anon_distinct_id, person_properties: this.instance.get_property(STORED_PERSON_PROPERTIES_KEY), group_properties: this.instance.get_property(STORED_GROUP_PROPERTIES_KEY), - disable_flags: this.instance.config.advanced_disable_feature_flags || undefined, - ...(options?.data || {}), + } + + if (options?.disableFlags || this.instance.config.advanced_disable_feature_flags) { + data.disable_flags = true } this._requestInFlight = true From ba6c722e08d0533999e77fd38d93fa8281b7bb0b Mon Sep 17 00:00:00 2001 From: Ben White Date: Thu, 12 Dec 2024 13:06:40 +0100 Subject: [PATCH 08/13] Fix --- src/__tests__/featureflags.test.ts | 6 +----- src/posthog-featureflags.ts | 4 +--- 2 files changed, 2 insertions(+), 8 deletions(-) diff --git a/src/__tests__/featureflags.test.ts b/src/__tests__/featureflags.test.ts index 30d89c301..a54ada8fd 100644 --- a/src/__tests__/featureflags.test.ts +++ b/src/__tests__/featureflags.test.ts @@ -284,11 +284,7 @@ describe('featureflags', () => { featureFlags.decide() expect(instance._send_request).toHaveBeenCalledTimes(1) - expect(instance._send_request.mock.calls[0][0]).toMatchObject({ - data: { - disable_flags: undefined, - }, - }) + expect(instance._send_request.mock.calls[0][0].data.disable_flags).toBe(undefined) jest.runOnlyPendingTimers() expect(instance._send_request).toHaveBeenCalledTimes(1) diff --git a/src/posthog-featureflags.ts b/src/posthog-featureflags.ts index f0f317002..78eee1341 100644 --- a/src/posthog-featureflags.ts +++ b/src/posthog-featureflags.ts @@ -110,9 +110,7 @@ export class PostHogFeatureFlags { this.instance.config.advanced_disable_feature_flags_on_first_load) this._callDecideEndpoint({ - data: { - disable_flags: disableFlags, - }, + disableFlags, }) } From f3d237ac6fac9f9ef5009d55ca2781d8b626f1de Mon Sep 17 00:00:00 2001 From: Ben White Date: Thu, 12 Dec 2024 13:08:01 +0100 Subject: [PATCH 09/13] Fixes --- functional_tests/feature-flags.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/functional_tests/feature-flags.test.ts b/functional_tests/feature-flags.test.ts index d2fd05fbf..6e4fe65f5 100644 --- a/functional_tests/feature-flags.test.ts +++ b/functional_tests/feature-flags.test.ts @@ -178,7 +178,7 @@ describe('FunctionalTests / Feature Flags', () => { expect(getRequests(token)['/decide/']).toEqual([ // This is the initial call to the decide endpoint on PostHog init, with all info added from `loaded`. { - // $anon_distinct_id: 'anon-id', // no anonymous ID is sent because this was overridden on load + $anon_distinct_id: 'anon-id', distinct_id: 'test-id', groups: { playlist: 'id:77' }, person_properties: { From dcd5e8ad8129ea3940226cb3885dcc4cee6ccd6c Mon Sep 17 00:00:00 2001 From: Ben White Date: Thu, 12 Dec 2024 13:23:35 +0100 Subject: [PATCH 10/13] Fixes --- cypress/e2e/capture.cy.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/cypress/e2e/capture.cy.ts b/cypress/e2e/capture.cy.ts index 3516e35e7..d4faf8155 100644 --- a/cypress/e2e/capture.cy.ts +++ b/cypress/e2e/capture.cy.ts @@ -339,6 +339,7 @@ describe('Event capture', () => { token: 'test_token', distinct_id: 'new-id', person_properties: {}, + $anon_distinct_id: payload.$anon_distinct_id, groups: { company: 'id:5', playlist: 'id:77', From d3508763b2df0fad86181b81b0bc02e2f7b1ae86 Mon Sep 17 00:00:00 2001 From: Ben White Date: Fri, 13 Dec 2024 09:11:45 +0100 Subject: [PATCH 11/13] Update src/posthog-featureflags.ts Co-authored-by: Dylan Martin --- src/posthog-featureflags.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/posthog-featureflags.ts b/src/posthog-featureflags.ts index 78eee1341..3271a8c55 100644 --- a/src/posthog-featureflags.ts +++ b/src/posthog-featureflags.ts @@ -200,7 +200,7 @@ export class PostHogFeatureFlags { /** * NOTE: This is used both for flags and remote config. Once the RemoteConfig is fully released this will essentially only - * be for flags and can eventually be replaced wiht the new flags endpoint + * be for flags and can eventually be replaced with the new flags endpoint */ _callDecideEndpoint(options?: { disableFlags?: boolean }): void { // Ensure we don't have double queued decide requests From 2fb2d2028f7c94fe1e221916f040409eb47870fe Mon Sep 17 00:00:00 2001 From: Ben White Date: Fri, 13 Dec 2024 13:36:04 +0100 Subject: [PATCH 12/13] fix --- src/__tests__/remote-config.test.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/__tests__/remote-config.test.ts b/src/__tests__/remote-config.test.ts index 473bb1d0a..6fb265281 100644 --- a/src/__tests__/remote-config.test.ts +++ b/src/__tests__/remote-config.test.ts @@ -9,8 +9,6 @@ describe('RemoteConfigLoader', () => { let posthog: PostHog beforeEach(() => { - // clean the JSDOM to prevent interdependencies between tests - const defaultConfig: Partial = { token: 'testtoken', api_host: 'https://test.com', From 5040141c7f25355484facbbe4b84fbcf00c055e9 Mon Sep 17 00:00:00 2001 From: Ben White Date: Fri, 13 Dec 2024 14:12:01 +0100 Subject: [PATCH 13/13] Fixed up loading of flag --- src/posthog-featureflags.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/posthog-featureflags.ts b/src/posthog-featureflags.ts index b0f5279db..e8d2e9c3e 100644 --- a/src/posthog-featureflags.ts +++ b/src/posthog-featureflags.ts @@ -91,6 +91,7 @@ export class PostHogFeatureFlags { private _additionalReloadRequested: boolean = false private _reloadDebouncer?: any private _decideCalled: boolean = false + private _flagsLoadedFromRemote: boolean = false constructor(private instance: PostHog) { this.featureFlagEventHandlers = [] @@ -257,6 +258,7 @@ export class PostHogFeatureFlags { return } + this._flagsLoadedFromRemote = !errorsLoading this.receivedFeatureFlags(response.json ?? {}, errorsLoading) if (this._additionalReloadRequested) { @@ -303,7 +305,7 @@ export class PostHogFeatureFlags { $feature_flag_bootstrapped_payload: this.instance.config.bootstrap?.featureFlagPayloads?.[key] || null, // If we haven't yet received a response from the /decide endpoint, we must have used the bootstrapped value - $used_bootstrap_value: !this.instance.decideEndpointWasHit, + $used_bootstrap_value: !this._flagsLoadedFromRemote, }) } }