From 60f7ed91dfb43f749dc8b51432b0d79402497ed5 Mon Sep 17 00:00:00 2001 From: Neil Kakkar Date: Thu, 9 Jun 2022 13:44:55 +0100 Subject: [PATCH 1/7] feat(feature-flags): Enable experience continuity --- src/posthog-core.js | 2 ++ src/posthog-featureflags.js | 12 ++++++++++++ 2 files changed, 14 insertions(+) diff --git a/src/posthog-core.js b/src/posthog-core.js index 86d424904..e9de90432 100644 --- a/src/posthog-core.js +++ b/src/posthog-core.js @@ -892,6 +892,8 @@ PostHogLib.prototype.identify = function (new_distinct_id, userPropertiesToSet, // Reload active feature flags if the user identity changes. // Note we don't reload this on property changes as these get processed async if (new_distinct_id !== previous_distinct_id) { + // let the reload feature flag request know to send the previous distinct id + this.featureFlags.sendAnonymousDistinctId(previous_distinct_id) this.reloadFeatureFlags() } } diff --git a/src/posthog-featureflags.js b/src/posthog-featureflags.js index 444d817e1..df55f1277 100644 --- a/src/posthog-featureflags.js +++ b/src/posthog-featureflags.js @@ -90,6 +90,10 @@ export class PostHogFeatureFlags { } } + sendAnonymousDistinctId(anon_distinct_id) { + this.$anon_distinct_id = anon_distinct_id + } + setReloadingPaused(isPaused) { this.reloadFeatureFlagsInAction = isPaused } @@ -116,7 +120,15 @@ export class PostHogFeatureFlags { token: token, distinct_id: this.instance.get_distinct_id(), groups: this.instance.getGroups(), + $anon_distinct_id: this.$anon_distinct_id, }) + + // console.log('json data: ', json_data) + + // reset anon_distinct_id after a single request with it + // makes it through + this.$anon_distinct_id = undefined + const encoded_data = _.base64Encode(json_data) this.instance._send_request( this.instance.get_config('api_host') + '/decide/?v=2', From ffd67f11e31791488beac0a2512b55c8a76ec7cc Mon Sep 17 00:00:00 2001 From: Neil Kakkar Date: Fri, 10 Jun 2022 17:13:41 +0100 Subject: [PATCH 2/7] fix tests --- src/__tests__/featureflags.js | 118 ++++++++++++++++++++++++++++++---- src/__tests__/posthog-core.js | 13 +++- src/posthog-core.js | 5 +- src/posthog-featureflags.js | 2 - 4 files changed, 120 insertions(+), 18 deletions(-) diff --git a/src/__tests__/featureflags.js b/src/__tests__/featureflags.js index 2ec28cfd0..32f8d14bf 100644 --- a/src/__tests__/featureflags.js +++ b/src/__tests__/featureflags.js @@ -1,12 +1,32 @@ import { PostHogFeatureFlags, parseFeatureFlagDecideResponse } from '../posthog-featureflags' +jest.useFakeTimers() +jest.spyOn(global, 'setTimeout') describe('featureflags', () => { given('decideEndpointWasHit', () => false) given('instance', () => ({ get_config: jest.fn().mockImplementation((key) => given.config[key]), - get_property: (key) => given.properties[key], + get_distinct_id: () => 'blah id', + getGroups: () => {}, + _prepare_callback: (callback) => callback, + persistence: { + props: { + $active_feature_flags: ['beta-feature', 'alpha-feature-2', 'multivariate-flag'], + $enabled_feature_flags: { + 'beta-feature': true, + 'alpha-feature-2': true, + 'multivariate-flag': 'variant-1', + }, + $override_feature_flags: false, + }, + register: (dict) => { + given.instance.persistence.props = { ...given.instance.persistence.props, ...dict } + }, + }, + get_property: (key) => given.instance.persistence.props[key], capture: () => {}, decideEndpointWasHit: given.decideEndpointWasHit, + _send_request: jest.fn().mockImplementation((url, data, headers, callback) => callback(given.decideResponse)), })) given('featureFlags', () => new PostHogFeatureFlags(given.instance)) @@ -16,16 +36,6 @@ describe('featureflags', () => { jest.spyOn(window.console, 'warn').mockImplementation() }) - given('properties', () => ({ - $active_feature_flags: ['beta-feature', 'alpha-feature-2', 'multivariate-flag'], - $enabled_feature_flags: { - 'beta-feature': true, - 'alpha-feature-2': true, - 'multivariate-flag': 'variant-1', - }, - $override_feature_flags: false, - })) - it('should return the right feature flag and call capture', () => { expect(given.featureFlags.getFlags()).toEqual(['beta-feature', 'alpha-feature-2', 'multivariate-flag']) expect(given.featureFlags.getFlagVariants()).toEqual({ @@ -50,7 +60,7 @@ describe('featureflags', () => { }) it('supports overrides', () => { - given('properties', () => ({ + given.instance.persistence.props = { $active_feature_flags: ['beta-feature', 'alpha-feature-2', 'multivariate-flag'], $enabled_feature_flags: { 'beta-feature': true, @@ -61,7 +71,8 @@ describe('featureflags', () => { 'beta-feature': false, 'alpha-feature-2': 'as-a-variant', }, - })) + } + expect(given.featureFlags.getFlags()).toEqual(['alpha-feature-2', 'multivariate-flag']) expect(given.featureFlags.getFlagVariants()).toEqual({ 'alpha-feature-2': 'as-a-variant', @@ -84,6 +95,87 @@ describe('featureflags', () => { called = false }) + + describe('reloadFeatureFlags', () => { + given('decideResponse', () => ({ + featureFlags: { + first: 'variant-1', + second: true, + }, + })) + + given('config', () => ({ + token: 'random fake token', + })) + + it('on providing anonDistinctId', () => { + given.featureFlags.sendAnonymousDistinctId('rando_id') + given.featureFlags.reloadFeatureFlags() + + jest.runAllTimers() + + expect(given.featureFlags.getFlagVariants()).toEqual({ + first: 'variant-1', + second: true, + }) + + // check the request sent $anon_distinct_id + expect( + JSON.parse(Buffer.from(given.instance._send_request.mock.calls[0][1].data, 'base64').toString()) + ).toEqual({ + token: 'random fake token', + distinct_id: 'blah id', + $anon_distinct_id: 'rando_id', + }) + }) + + it('on providing anonDistinctId and calling reload multiple times', () => { + given.featureFlags.sendAnonymousDistinctId('rando_id') + given.featureFlags.reloadFeatureFlags() + given.featureFlags.reloadFeatureFlags() + + jest.runAllTimers() + + expect(given.featureFlags.getFlagVariants()).toEqual({ + first: 'variant-1', + second: true, + }) + + // check the request sent $anon_distinct_id + expect( + JSON.parse(Buffer.from(given.instance._send_request.mock.calls[0][1].data, 'base64').toString()) + ).toEqual({ + token: 'random fake token', + distinct_id: 'blah id', + $anon_distinct_id: 'rando_id', + }) + + given.featureFlags.reloadFeatureFlags() + given.featureFlags.reloadFeatureFlags() + jest.runAllTimers() + + // check the request didn't send $anon_distinct_id the second time around + expect( + JSON.parse(Buffer.from(given.instance._send_request.mock.calls[1][1].data, 'base64').toString()) + ).toEqual({ + token: 'random fake token', + distinct_id: 'blah id', + // $anon_distinct_id: "rando_id" + }) + + given.featureFlags.reloadFeatureFlags() + jest.runAllTimers() + + // check the request didn't send $anon_distinct_id the second time around + expect( + JSON.parse(Buffer.from(given.instance._send_request.mock.calls[1][1].data, 'base64').toString()) + ).toEqual({ + token: 'random fake token', + distinct_id: 'blah id', + // $anon_distinct_id: "rando_id" + }) + }) + }) }) describe('parseFeatureFlagDecideResponse', () => { diff --git a/src/__tests__/posthog-core.js b/src/__tests__/posthog-core.js index 30cc58689..7afd6b272 100644 --- a/src/__tests__/posthog-core.js +++ b/src/__tests__/posthog-core.js @@ -36,6 +36,9 @@ describe('identify()', () => { _captureMetrics: { incr: jest.fn(), }, + featureFlags: { + sendAnonymousDistinctId: jest.fn(), + }, reloadFeatureFlags: jest.fn(), })) @@ -63,6 +66,7 @@ describe('identify()', () => { { $set_once: {} } ) expect(given.overrides.people.set).not.toHaveBeenCalled() + expect(given.overrides.featureFlags.sendAnonymousDistinctId).toHaveBeenCalledWith('oldIdentity') }) it('calls capture when identity changes and old ID is anonymous', () => { @@ -80,6 +84,7 @@ describe('identify()', () => { { $set_once: {} } ) expect(given.overrides.people.set).not.toHaveBeenCalled() + expect(given.overrides.featureFlags.sendAnonymousDistinctId).toHaveBeenCalledWith('oldIdentity') }) it("don't identify if the old id isn't anonymous", () => { @@ -89,6 +94,7 @@ describe('identify()', () => { expect(given.overrides.capture).not.toHaveBeenCalled() expect(given.overrides.people.set).not.toHaveBeenCalled() + expect(given.overrides.featureFlags.sendAnonymousDistinctId).not.toHaveBeenCalled() }) it('calls capture with user properties if passed', () => { @@ -106,6 +112,7 @@ describe('identify()', () => { { $set: { email: 'john@example.com' } }, { $set_once: { howOftenAmISet: 'once!' } } ) + expect(given.overrides.featureFlags.sendAnonymousDistinctId).toHaveBeenCalledWith('oldIdentity') }) describe('identity did not change', () => { @@ -116,6 +123,7 @@ describe('identify()', () => { expect(given.overrides.capture).not.toHaveBeenCalled() expect(given.overrides.people.set).not.toHaveBeenCalled() + expect(given.overrides.featureFlags.sendAnonymousDistinctId).not.toHaveBeenCalled() }) it('calls people.set when user properties passed', () => { @@ -125,6 +133,7 @@ describe('identify()', () => { given.subject() expect(given.overrides.capture).not.toHaveBeenCalled() + expect(given.overrides.featureFlags.sendAnonymousDistinctId).not.toHaveBeenCalled() expect(given.overrides.people.set).toHaveBeenCalledWith({ email: 'john@example.com' }) expect(given.overrides.people.set_once).toHaveBeenCalledWith({ howOftenAmISet: 'once!' }) }) @@ -148,6 +157,7 @@ describe('identify()', () => { it('reloads when identity changes', () => { given.subject() + expect(given.overrides.featureFlags.sendAnonymousDistinctId).toHaveBeenCalledWith('oldIdentity') expect(given.overrides.reloadFeatureFlags).toHaveBeenCalled() }) @@ -156,6 +166,7 @@ describe('identify()', () => { given.subject() + expect(given.overrides.featureFlags.sendAnonymousDistinctId).not.toHaveBeenCalled() expect(given.overrides.reloadFeatureFlags).not.toHaveBeenCalled() }) @@ -165,7 +176,7 @@ describe('identify()', () => { given('userPropertiesToSetOnce', () => ({ howOftenAmISet: 'once!' })) given.subject() - + expect(given.overrides.featureFlags.sendAnonymousDistinctId).not.toHaveBeenCalled() expect(given.overrides.reloadFeatureFlags).not.toHaveBeenCalled() }) }) diff --git a/src/posthog-core.js b/src/posthog-core.js index e9de90432..32826c542 100644 --- a/src/posthog-core.js +++ b/src/posthog-core.js @@ -880,6 +880,9 @@ PostHogLib.prototype.identify = function (new_distinct_id, userPropertiesToSet, { $set: userPropertiesToSet || {} }, { $set_once: userPropertiesToSetOnce || {} } ) + // let the reload feature flag request know to send this previous distinct id + // for flag consistency + this.featureFlags.sendAnonymousDistinctId(previous_distinct_id) } else { if (userPropertiesToSet) { this['people'].set(userPropertiesToSet) @@ -892,8 +895,6 @@ PostHogLib.prototype.identify = function (new_distinct_id, userPropertiesToSet, // Reload active feature flags if the user identity changes. // Note we don't reload this on property changes as these get processed async if (new_distinct_id !== previous_distinct_id) { - // let the reload feature flag request know to send the previous distinct id - this.featureFlags.sendAnonymousDistinctId(previous_distinct_id) this.reloadFeatureFlags() } } diff --git a/src/posthog-featureflags.js b/src/posthog-featureflags.js index df55f1277..414cc7b62 100644 --- a/src/posthog-featureflags.js +++ b/src/posthog-featureflags.js @@ -123,8 +123,6 @@ export class PostHogFeatureFlags { $anon_distinct_id: this.$anon_distinct_id, }) - // console.log('json data: ', json_data) - // reset anon_distinct_id after a single request with it // makes it through this.$anon_distinct_id = undefined From cc51b9d574db974e1e9bc2a6a1382bc16eabf0bb Mon Sep 17 00:00:00 2001 From: Neil Kakkar Date: Fri, 24 Jun 2022 13:06:31 +0100 Subject: [PATCH 3/7] update test --- src/__tests__/featureflags.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/__tests__/featureflags.js b/src/__tests__/featureflags.js index 32f8d14bf..1d46ab5a6 100644 --- a/src/__tests__/featureflags.js +++ b/src/__tests__/featureflags.js @@ -168,7 +168,7 @@ describe('featureflags', () => { // check the request didn't send $anon_distinct_id the second time around expect( - JSON.parse(Buffer.from(given.instance._send_request.mock.calls[1][1].data, 'base64').toString()) + JSON.parse(Buffer.from(given.instance._send_request.mock.calls[2][1].data, 'base64').toString()) ).toEqual({ token: 'random fake token', distinct_id: 'blah id', From 75889311f91b51c06630aad6f0ea8e57acafb1ea Mon Sep 17 00:00:00 2001 From: Neil Kakkar Date: Tue, 28 Jun 2022 11:49:16 +0100 Subject: [PATCH 4/7] address comment --- src/__tests__/featureflags.js | 4 ++-- src/posthog-core.js | 2 +- src/posthog-featureflags.js | 10 +++++----- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/__tests__/featureflags.js b/src/__tests__/featureflags.js index 1d46ab5a6..e3121fa1c 100644 --- a/src/__tests__/featureflags.js +++ b/src/__tests__/featureflags.js @@ -109,7 +109,7 @@ describe('featureflags', () => { })) it('on providing anonDistinctId', () => { - given.featureFlags.sendAnonymousDistinctId('rando_id') + given.featureFlags.setAnonymousDistinctId('rando_id') given.featureFlags.reloadFeatureFlags() jest.runAllTimers() @@ -130,7 +130,7 @@ describe('featureflags', () => { }) it('on providing anonDistinctId and calling reload multiple times', () => { - given.featureFlags.sendAnonymousDistinctId('rando_id') + given.featureFlags.setAnonymousDistinctId('rando_id') given.featureFlags.reloadFeatureFlags() given.featureFlags.reloadFeatureFlags() diff --git a/src/posthog-core.js b/src/posthog-core.js index 32826c542..88083eb3d 100644 --- a/src/posthog-core.js +++ b/src/posthog-core.js @@ -882,7 +882,7 @@ PostHogLib.prototype.identify = function (new_distinct_id, userPropertiesToSet, ) // let the reload feature flag request know to send this previous distinct id // for flag consistency - this.featureFlags.sendAnonymousDistinctId(previous_distinct_id) + this.featureFlags.setAnonymousDistinctId(previous_distinct_id) } else { if (userPropertiesToSet) { this['people'].set(userPropertiesToSet) diff --git a/src/posthog-featureflags.js b/src/posthog-featureflags.js index 414cc7b62..b61d6d539 100644 --- a/src/posthog-featureflags.js +++ b/src/posthog-featureflags.js @@ -90,7 +90,7 @@ export class PostHogFeatureFlags { } } - sendAnonymousDistinctId(anon_distinct_id) { + setAnonymousDistinctId(anon_distinct_id) { this.$anon_distinct_id = anon_distinct_id } @@ -123,16 +123,16 @@ export class PostHogFeatureFlags { $anon_distinct_id: this.$anon_distinct_id, }) - // reset anon_distinct_id after a single request with it - // makes it through - this.$anon_distinct_id = undefined - const encoded_data = _.base64Encode(json_data) this.instance._send_request( this.instance.get_config('api_host') + '/decide/?v=2', { data: encoded_data }, { method: 'POST' }, this.instance._prepare_callback((response) => { + // reset anon_distinct_id after at least a single request with it + // makes it through + this.$anon_distinct_id = undefined + this.receivedFeatureFlags(response) // :TRICKY: Reload - start another request if queued! From ed6f20603e72b8259cdeea5c740e9a2af49c0ed0 Mon Sep 17 00:00:00 2001 From: Neil Kakkar Date: Tue, 28 Jun 2022 11:50:23 +0100 Subject: [PATCH 5/7] fix tests --- src/__tests__/posthog-core.js | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/__tests__/posthog-core.js b/src/__tests__/posthog-core.js index 7afd6b272..66245dd37 100644 --- a/src/__tests__/posthog-core.js +++ b/src/__tests__/posthog-core.js @@ -37,7 +37,7 @@ describe('identify()', () => { incr: jest.fn(), }, featureFlags: { - sendAnonymousDistinctId: jest.fn(), + setAnonymousDistinctId: jest.fn(), }, reloadFeatureFlags: jest.fn(), })) @@ -66,7 +66,7 @@ describe('identify()', () => { { $set_once: {} } ) expect(given.overrides.people.set).not.toHaveBeenCalled() - expect(given.overrides.featureFlags.sendAnonymousDistinctId).toHaveBeenCalledWith('oldIdentity') + expect(given.overrides.featureFlags.setAnonymousDistinctId).toHaveBeenCalledWith('oldIdentity') }) it('calls capture when identity changes and old ID is anonymous', () => { @@ -84,7 +84,7 @@ describe('identify()', () => { { $set_once: {} } ) expect(given.overrides.people.set).not.toHaveBeenCalled() - expect(given.overrides.featureFlags.sendAnonymousDistinctId).toHaveBeenCalledWith('oldIdentity') + expect(given.overrides.featureFlags.setAnonymousDistinctId).toHaveBeenCalledWith('oldIdentity') }) it("don't identify if the old id isn't anonymous", () => { @@ -94,7 +94,7 @@ describe('identify()', () => { expect(given.overrides.capture).not.toHaveBeenCalled() expect(given.overrides.people.set).not.toHaveBeenCalled() - expect(given.overrides.featureFlags.sendAnonymousDistinctId).not.toHaveBeenCalled() + expect(given.overrides.featureFlags.setAnonymousDistinctId).not.toHaveBeenCalled() }) it('calls capture with user properties if passed', () => { @@ -112,7 +112,7 @@ describe('identify()', () => { { $set: { email: 'john@example.com' } }, { $set_once: { howOftenAmISet: 'once!' } } ) - expect(given.overrides.featureFlags.sendAnonymousDistinctId).toHaveBeenCalledWith('oldIdentity') + expect(given.overrides.featureFlags.setAnonymousDistinctId).toHaveBeenCalledWith('oldIdentity') }) describe('identity did not change', () => { @@ -123,7 +123,7 @@ describe('identify()', () => { expect(given.overrides.capture).not.toHaveBeenCalled() expect(given.overrides.people.set).not.toHaveBeenCalled() - expect(given.overrides.featureFlags.sendAnonymousDistinctId).not.toHaveBeenCalled() + expect(given.overrides.featureFlags.setAnonymousDistinctId).not.toHaveBeenCalled() }) it('calls people.set when user properties passed', () => { @@ -133,7 +133,7 @@ describe('identify()', () => { given.subject() expect(given.overrides.capture).not.toHaveBeenCalled() - expect(given.overrides.featureFlags.sendAnonymousDistinctId).not.toHaveBeenCalled() + expect(given.overrides.featureFlags.setAnonymousDistinctId).not.toHaveBeenCalled() expect(given.overrides.people.set).toHaveBeenCalledWith({ email: 'john@example.com' }) expect(given.overrides.people.set_once).toHaveBeenCalledWith({ howOftenAmISet: 'once!' }) }) @@ -157,7 +157,7 @@ describe('identify()', () => { it('reloads when identity changes', () => { given.subject() - expect(given.overrides.featureFlags.sendAnonymousDistinctId).toHaveBeenCalledWith('oldIdentity') + expect(given.overrides.featureFlags.setAnonymousDistinctId).toHaveBeenCalledWith('oldIdentity') expect(given.overrides.reloadFeatureFlags).toHaveBeenCalled() }) @@ -166,7 +166,7 @@ describe('identify()', () => { given.subject() - expect(given.overrides.featureFlags.sendAnonymousDistinctId).not.toHaveBeenCalled() + expect(given.overrides.featureFlags.setAnonymousDistinctId).not.toHaveBeenCalled() expect(given.overrides.reloadFeatureFlags).not.toHaveBeenCalled() }) @@ -176,7 +176,7 @@ describe('identify()', () => { given('userPropertiesToSetOnce', () => ({ howOftenAmISet: 'once!' })) given.subject() - expect(given.overrides.featureFlags.sendAnonymousDistinctId).not.toHaveBeenCalled() + expect(given.overrides.featureFlags.setAnonymousDistinctId).not.toHaveBeenCalled() expect(given.overrides.reloadFeatureFlags).not.toHaveBeenCalled() }) }) From 41f3b847eb6d25fd8e5700dff7311da5186985dc Mon Sep 17 00:00:00 2001 From: Neil Kakkar Date: Tue, 28 Jun 2022 12:29:20 +0100 Subject: [PATCH 6/7] try a longer timeout --- testcafe/helpers.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/testcafe/helpers.js b/testcafe/helpers.js index e87cd8c9c..ac044a199 100644 --- a/testcafe/helpers.js +++ b/testcafe/helpers.js @@ -45,7 +45,7 @@ export async function retryUntilResults(operation, target_results, limit = 100) results.length >= target_results ? resolve(results) : attempt(count + 1, resolve, reject) ) .catch(reject) - }, 600) + }, 1000) } return new Promise((...args) => attempt(0, ...args)) From c91d0302b97d540505f42d2de9052592d47668a1 Mon Sep 17 00:00:00 2001 From: Neil Kakkar Date: Tue, 28 Jun 2022 12:46:38 +0100 Subject: [PATCH 7/7] Revert "try a longer timeout" This reverts commit 41f3b847eb6d25fd8e5700dff7311da5186985dc. --- testcafe/helpers.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/testcafe/helpers.js b/testcafe/helpers.js index ac044a199..e87cd8c9c 100644 --- a/testcafe/helpers.js +++ b/testcafe/helpers.js @@ -45,7 +45,7 @@ export async function retryUntilResults(operation, target_results, limit = 100) results.length >= target_results ? resolve(results) : attempt(count + 1, resolve, reject) ) .catch(reject) - }, 1000) + }, 600) } return new Promise((...args) => attempt(0, ...args))