From c0419c9b8b644b69bc7e63d1b3609f9360bfb9c1 Mon Sep 17 00:00:00 2001 From: Neil Kakkar Date: Thu, 14 Sep 2023 13:18:13 +0100 Subject: [PATCH 1/4] fix(flags): Enqueue follow up requests without dropping --- functional_tests/feature-flags.test.ts | 25 +++++++++++++++++++------ src/__tests__/compression.js | 2 +- src/__tests__/decide.js | 2 +- src/__tests__/posthog-core.js | 2 +- src/__tests__/posthog-core.loaded.js | 16 ++++++++++------ src/decide.ts | 3 ++- src/posthog-featureflags.ts | 4 ---- 7 files changed, 34 insertions(+), 20 deletions(-) diff --git a/functional_tests/feature-flags.test.ts b/functional_tests/feature-flags.test.ts index a9bf93bc6..0ca4071c4 100644 --- a/functional_tests/feature-flags.test.ts +++ b/functional_tests/feature-flags.test.ts @@ -107,10 +107,7 @@ test('person properties set in identify() with the same distinct_id are sent to }) }) -test('identify() doesnt trigger new request automatically if first request takes too long', async () => { - // TODO: Make this experience nicer, and queue requests, rather than leave - // it upto the user to call reloadFeatureFlags() manually. - +test('identify() triggers new request in queue after first request', async () => { const token = v4() const posthog = await createPosthogInstance(token, { advanced_disable_decide: false }) @@ -130,12 +127,28 @@ test('identify() doesnt trigger new request automatically if first request takes resetRequests(token) // don't wait for decide callback - posthog.identify('test-id', { - email: 'test@email.com', + email: 'test2@email.com', }) await waitFor(() => { expect(getRequests(token)['/decide/']).toEqual([]) }) + + // wait for decide callback + // eslint-disable-next-line compat/compat + await new Promise((res) => setTimeout(res, 500)) + + // now second call should've fired + await waitFor(() => { + expect(getRequests(token)['/decide/']).toEqual([ + { + $anon_distinct_id: anonymousId, + distinct_id: 'test-id', + groups: {}, + person_properties: { email: 'test2@email.com' }, + token, + }, + ]) + }) }) diff --git a/src/__tests__/compression.js b/src/__tests__/compression.js index fc0ee5572..2904fe5bd 100644 --- a/src/__tests__/compression.js +++ b/src/__tests__/compression.js @@ -92,8 +92,8 @@ describe('Payload Compression', () => { }, featureFlags: { receivedFeatureFlags: jest.fn(), - resetRequestQueue: jest.fn(), setReloadingPaused: jest.fn(), + _startReloadTimer: jest.fn(), }, _hasBootstrappedFeatureFlags: jest.fn(), get_property: (property_key) => diff --git a/src/__tests__/decide.js b/src/__tests__/decide.js index 8ee7078b9..20122a342 100644 --- a/src/__tests__/decide.js +++ b/src/__tests__/decide.js @@ -27,8 +27,8 @@ describe('Decide', () => { }, featureFlags: { receivedFeatureFlags: jest.fn(), - resetRequestQueue: jest.fn(), setReloadingPaused: jest.fn(), + _startReloadTimer: jest.fn(), }, _hasBootstrappedFeatureFlags: jest.fn(), getGroups: () => ({ organization: '5' }), diff --git a/src/__tests__/posthog-core.js b/src/__tests__/posthog-core.js index 1474c6c12..e7f329d74 100644 --- a/src/__tests__/posthog-core.js +++ b/src/__tests__/posthog-core.js @@ -892,7 +892,7 @@ describe('_loaded()', () => { capture: jest.fn(), featureFlags: { setReloadingPaused: jest.fn(), - resetRequestQueue: jest.fn(), + _startReloadTimer: jest.fn(), }, _start_queue_if_opted_in: jest.fn(), })) diff --git a/src/__tests__/posthog-core.loaded.js b/src/__tests__/posthog-core.loaded.js index 0afbd28c0..5a0b1077d 100644 --- a/src/__tests__/posthog-core.loaded.js +++ b/src/__tests__/posthog-core.loaded.js @@ -17,7 +17,7 @@ describe('loaded() with flags', () => { capture: jest.fn(), featureFlags: { setReloadingPaused: jest.fn(), - resetRequestQueue: jest.fn(), + _startReloadTimer: jest.fn(), receivedFeatureFlags: jest.fn(), }, _start_queue_if_opted_in: jest.fn(), @@ -48,24 +48,28 @@ describe('loaded() with flags', () => { beforeEach(() => { jest.spyOn(given.lib.featureFlags, 'setGroupPropertiesForFlags') jest.spyOn(given.lib.featureFlags, 'setReloadingPaused') - jest.spyOn(given.lib.featureFlags, 'resetRequestQueue') + jest.spyOn(given.lib.featureFlags, '_startReloadTimer') jest.spyOn(given.lib.featureFlags, '_reloadFeatureFlagsRequest') }) it('doesnt call flags while initial load is happening', () => { given.subject() - jest.runAllTimers() + jest.runOnlyPendingTimers() expect(given.lib.featureFlags.setGroupPropertiesForFlags).toHaveBeenCalled() // loaded ph.group() calls setGroupPropertiesForFlags expect(given.lib.featureFlags.setReloadingPaused).toHaveBeenCalledWith(true) - expect(given.lib.featureFlags.resetRequestQueue).toHaveBeenCalledTimes(1) + expect(given.lib.featureFlags._startReloadTimer).toHaveBeenCalled() expect(given.lib.featureFlags.setReloadingPaused).toHaveBeenCalledWith(false) - // even if the decide request returned late, we should not call _reloadFeatureFlagsRequest + // we should call _reloadFeatureFlagsRequest for `group` only after the initial load // because it ought to be paused until decide returns expect(given.overrides._send_request).toHaveBeenCalledTimes(1) expect(given.lib.featureFlags._reloadFeatureFlagsRequest).toHaveBeenCalledTimes(0) + + jest.runOnlyPendingTimers() + expect(given.overrides._send_request).toHaveBeenCalledTimes(2) + expect(given.lib.featureFlags._reloadFeatureFlagsRequest).toHaveBeenCalledTimes(1) }) }) @@ -74,7 +78,7 @@ describe('loaded() with flags', () => { expect(given.overrides.featureFlags.setReloadingPaused).toHaveBeenCalledWith(true) expect(given.overrides.featureFlags.setReloadingPaused).toHaveBeenCalledWith(false) - expect(given.overrides.featureFlags.resetRequestQueue).toHaveBeenCalledTimes(1) + expect(given.overrides.featureFlags._startReloadTimer).toHaveBeenCalled() expect(given.overrides.featureFlags.receivedFeatureFlags).toHaveBeenCalledTimes(1) }) }) diff --git a/src/decide.ts b/src/decide.ts index 1a79dee96..bf92c8fe1 100644 --- a/src/decide.ts +++ b/src/decide.ts @@ -39,8 +39,9 @@ export class Decide { } parseDecideResponse(response: DecideResponse): void { - this.instance.featureFlags.resetRequestQueue() this.instance.featureFlags.setReloadingPaused(false) + // :TRICKY: Reload - start another request if queued! + this.instance.featureFlags._startReloadTimer() if (response?.status === 0) { console.error('Failed to fetch feature flags from PostHog.') diff --git a/src/posthog-featureflags.ts b/src/posthog-featureflags.ts index 0e3a40a0f..176aa39e4 100644 --- a/src/posthog-featureflags.ts +++ b/src/posthog-featureflags.ts @@ -151,10 +151,6 @@ export class PostHogFeatureFlags { this.reloadFeatureFlagsInAction = isPaused } - resetRequestQueue(): void { - this.reloadFeatureFlagsQueued = false - } - _startReloadTimer(): void { if (this.reloadFeatureFlagsQueued && !this.reloadFeatureFlagsInAction) { setTimeout(() => { From c62a2541f188ddc29b6961064f70e53da0141210 Mon Sep 17 00:00:00 2001 From: Neil Kakkar Date: Thu, 14 Sep 2023 14:15:23 +0100 Subject: [PATCH 2/4] fix tests --- README.md | 2 +- functional_tests/feature-flags.test.ts | 32 ++++++++++++++++++++++++++ src/__tests__/posthog-core.js | 1 + src/__tests__/posthog-core.loaded.js | 3 +++ src/posthog-core.ts | 4 ++++ src/posthog-featureflags.ts | 4 ++++ 6 files changed, 45 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index 1c8589443..6f498757a 100644 --- a/README.md +++ b/README.md @@ -9,7 +9,7 @@ This README is intended for developing the library itself. ## Testing Unit tests: run `yarn test`. -Cypress: run `yarn serve` to have a test server running and separately `yarn cypress` to launch Cypress test engine. +Cypress: run `yarn start` to have a test server running and separately `yarn cypress` to launch Cypress test engine. ### Running TestCafe E2E tests with BrowserStack diff --git a/functional_tests/feature-flags.test.ts b/functional_tests/feature-flags.test.ts index 0ca4071c4..3b2cc6861 100644 --- a/functional_tests/feature-flags.test.ts +++ b/functional_tests/feature-flags.test.ts @@ -152,3 +152,35 @@ test('identify() triggers new request in queue after first request', async () => ]) }) }) + +test('identify() does not trigger new request in queue after first request for loaded callback', async () => { + const token = v4() + await createPosthogInstance(token, { + advanced_disable_decide: false, + bootstrap: { distinctID: 'anon-id' }, + loaded: (ph) => { + ph.identify('test-id', { email: 'test3@email.com' }) + ph.group('playlist', 'id:77', { length: 8 }) + }, + }) + + await waitFor(() => { + 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 + distinct_id: 'test-id', + groups: { playlist: 'id:77' }, + person_properties: { + email: 'test3@email.com', + }, + group_properties: { + playlist: { + length: 8, + }, + }, + token, + }, + ]) + }) +}) diff --git a/src/__tests__/posthog-core.js b/src/__tests__/posthog-core.js index e7f329d74..87a2a2ac2 100644 --- a/src/__tests__/posthog-core.js +++ b/src/__tests__/posthog-core.js @@ -892,6 +892,7 @@ describe('_loaded()', () => { capture: jest.fn(), featureFlags: { setReloadingPaused: jest.fn(), + resetRequestQueue: jest.fn(), _startReloadTimer: jest.fn(), }, _start_queue_if_opted_in: jest.fn(), diff --git a/src/__tests__/posthog-core.loaded.js b/src/__tests__/posthog-core.loaded.js index 5a0b1077d..4f7ce5e8b 100644 --- a/src/__tests__/posthog-core.loaded.js +++ b/src/__tests__/posthog-core.loaded.js @@ -17,6 +17,7 @@ describe('loaded() with flags', () => { capture: jest.fn(), featureFlags: { setReloadingPaused: jest.fn(), + resetRequestQueue: jest.fn(), _startReloadTimer: jest.fn(), receivedFeatureFlags: jest.fn(), }, @@ -49,6 +50,7 @@ describe('loaded() with flags', () => { jest.spyOn(given.lib.featureFlags, 'setGroupPropertiesForFlags') jest.spyOn(given.lib.featureFlags, 'setReloadingPaused') jest.spyOn(given.lib.featureFlags, '_startReloadTimer') + jest.spyOn(given.lib.featureFlags, 'resetRequestQueue') jest.spyOn(given.lib.featureFlags, '_reloadFeatureFlagsRequest') }) @@ -59,6 +61,7 @@ describe('loaded() with flags', () => { expect(given.lib.featureFlags.setGroupPropertiesForFlags).toHaveBeenCalled() // loaded ph.group() calls setGroupPropertiesForFlags expect(given.lib.featureFlags.setReloadingPaused).toHaveBeenCalledWith(true) + expect(given.lib.featureFlags.resetRequestQueue).toHaveBeenCalledTimes(1) expect(given.lib.featureFlags._startReloadTimer).toHaveBeenCalled() expect(given.lib.featureFlags.setReloadingPaused).toHaveBeenCalledWith(false) diff --git a/src/posthog-core.ts b/src/posthog-core.ts index 2635f8d03..f5d531e05 100644 --- a/src/posthog-core.ts +++ b/src/posthog-core.ts @@ -543,6 +543,10 @@ export class PostHog { this.capture('$pageview', { title: document.title }, { send_instantly: true }) } + // TRICKY: Reset any decide reloads queued during config.loaded because they'll be + // covered by the decide call right below + this.featureFlags.resetRequestQueue() + // 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. diff --git a/src/posthog-featureflags.ts b/src/posthog-featureflags.ts index 176aa39e4..0e3a40a0f 100644 --- a/src/posthog-featureflags.ts +++ b/src/posthog-featureflags.ts @@ -151,6 +151,10 @@ export class PostHogFeatureFlags { this.reloadFeatureFlagsInAction = isPaused } + resetRequestQueue(): void { + this.reloadFeatureFlagsQueued = false + } + _startReloadTimer(): void { if (this.reloadFeatureFlagsQueued && !this.reloadFeatureFlagsInAction) { setTimeout(() => { From 0c96a80266d30353e4f2d31d26e196583339f6ab Mon Sep 17 00:00:00 2001 From: Neil Kakkar Date: Thu, 14 Sep 2023 14:17:12 +0100 Subject: [PATCH 3/4] fix --- src/posthog-core.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/posthog-core.ts b/src/posthog-core.ts index f5d531e05..ff6efe2f9 100644 --- a/src/posthog-core.ts +++ b/src/posthog-core.ts @@ -543,15 +543,15 @@ export class PostHog { this.capture('$pageview', { title: document.title }, { send_instantly: true }) } - // TRICKY: Reset any decide reloads queued during config.loaded because they'll be - // covered by the decide call right below - this.featureFlags.resetRequestQueue() - // 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. if (!disableDecide) { new Decide(this).call() + + // TRICKY: Reset any decide reloads queued during config.loaded because they'll be + // covered by the decide call right below + this.featureFlags.resetRequestQueue() } } From 527938f2e616cf80a3fe32f65633db0602c3e61e Mon Sep 17 00:00:00 2001 From: Neil Kakkar Date: Thu, 14 Sep 2023 14:17:43 +0100 Subject: [PATCH 4/4] fix --- 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 ff6efe2f9..e52a055cd 100644 --- a/src/posthog-core.ts +++ b/src/posthog-core.ts @@ -550,7 +550,7 @@ export class PostHog { new Decide(this).call() // TRICKY: Reset any decide reloads queued during config.loaded because they'll be - // covered by the decide call right below + // covered by the decide call right above. this.featureFlags.resetRequestQueue() } }