fix(flags): Re-enable reload only when request finishes (#791)

* fix(flags): Re-enable reload only when request finishes

* fix tests

functional_tests/feature-flags.test.ts

@@ -22,6 +22,10 @@ test('person properties set in identify() with new distinct_id are sent to decid
 
     resetRequests(token)
 
+    // wait for decide callback
+    // eslint-disable-next-line compat/compat
+    await new Promise((res) => setTimeout(res, 500))
+
     // Person properties set here should also be sent to the decide endpoint.
     posthog.identify('test-id', {
         email: '[email protected]',
@@ -63,6 +67,10 @@ test('person properties set in identify() with the same distinct_id are sent to
 
     resetRequests(token)
 
+    // wait for decide callback
+    // eslint-disable-next-line compat/compat
+    await new Promise((res) => setTimeout(res, 500))
+
     // First we identify with a new distinct_id but with no properties set
     posthog.identify('test-id')
 
@@ -98,3 +106,36 @@ 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.
+
+    const token = v4()
+    const posthog = await createPosthogInstance(token, { advanced_disable_decide: false })
+
+    const anonymousId = posthog.get_distinct_id()
+
+    await waitFor(() => {
+        expect(getRequests(token)['/decide/']).toEqual([
+            // This is the initial call to the decide endpoint on PostHog init.
+            {
+                distinct_id: anonymousId,
+                groups: {},
+                token,
+            },
+        ])
+    })
+
+    resetRequests(token)
+
+    // don't wait for decide callback
+
+    posthog.identify('test-id', {
+        email: '[email protected]',
+    })
+
+    await waitFor(() => {
+        expect(getRequests(token)['/decide/']).toEqual([])
+    })
+})

src/__tests__/compression.js

@@ -92,6 +92,8 @@ describe('Payload Compression', () => {
                 },
                 featureFlags: {
                     receivedFeatureFlags: jest.fn(),
+                    resetRequestQueue: jest.fn(),
+                    setReloadingPaused: jest.fn(),
                 },
                 _hasBootstrappedFeatureFlags: jest.fn(),
                 get_property: (property_key) =>

src/__tests__/decide.js

@@ -27,6 +27,8 @@ describe('Decide', () => {
         },
         featureFlags: {
             receivedFeatureFlags: jest.fn(),
+            resetRequestQueue: jest.fn(),
+            setReloadingPaused: jest.fn(),
         },
         _hasBootstrappedFeatureFlags: jest.fn(),
         getGroups: () => ({ organization: '5' }),

src/__tests__/posthog-core.js

@@ -923,10 +923,15 @@ describe('_loaded()', () => {
             Decide.mockImplementation(() => ({ call }))
         })
 
+        afterEach(() => {
+            Decide.mockReset()
+        })
+
         it('is called by default', () => {
             given.subject()
 
             expect(new Decide().call).toHaveBeenCalled()
+            expect(given.overrides.featureFlags.setReloadingPaused).toHaveBeenCalledWith(true)
         })
 
         it('does not call decide if disabled', () => {
@@ -938,6 +943,7 @@ describe('_loaded()', () => {
             given.subject()
 
             expect(new Decide().call).not.toHaveBeenCalled()
+            expect(given.overrides.featureFlags.setReloadingPaused).not.toHaveBeenCalled()
         })
     })
 
@@ -968,16 +974,7 @@ describe('_loaded()', () => {
             )
         })
     })
-
-    it('toggles feature flags on and off', () => {
-        given.subject()
-
-        expect(given.overrides.featureFlags.setReloadingPaused).toHaveBeenCalledWith(true)
-        expect(given.overrides.featureFlags.setReloadingPaused).toHaveBeenCalledWith(false)
-        expect(given.overrides.featureFlags.resetRequestQueue).toHaveBeenCalled()
-    })
 })
-
 describe('session_id', () => {
     given('overrides', () => ({
         sessionManager: {

src/__tests__/posthog-core.loaded.js

@@ -0,0 +1,80 @@
+import { PostHog } from '../posthog-core'
+import { PostHogPersistence } from '../posthog-persistence'
+
+jest.useFakeTimers()
+
+given('lib', () => Object.assign(new PostHog(), given.overrides))
+
+describe('loaded() with flags', () => {
+    beforeAll(() => {
+        jest.unmock('../decide')
+    })
+
+    given('subject', () => () => given.lib._loaded())
+
+    given('overrides', () => ({
+        get_config: (key) => given.config?.[key],
+        capture: jest.fn(),
+        featureFlags: {
+            setReloadingPaused: jest.fn(),
+            resetRequestQueue: jest.fn(),
+            receivedFeatureFlags: jest.fn(),
+        },
+        _start_queue_if_opted_in: jest.fn(),
+        persistence: new PostHogPersistence(given.config),
+        _send_request: jest.fn((host, data, header, callback) => callback({ status: 200 })),
+    }))
+    given('config', () => ({ loaded: jest.fn(), persistence: 'memory' }))
+
+    describe('toggling flag reloading', () => {
+        given('config', () => ({
+            loaded: (ph) => {
+                ph.group('org', 'bazinga', { name: 'Shelly' })
+                setTimeout(() => {
+                    ph.group('org', 'bazinga2', { name: 'Shelly' })
+                }, 100)
+            },
+            persistence: 'memory',
+        }))
+
+        given('overrides', () => ({
+            get_config: (key) => given.config?.[key],
+            capture: jest.fn(),
+            _send_request: jest.fn((host, data, header, callback) => setTimeout(() => callback({ status: 200 }), 1000)),
+            _start_queue_if_opted_in: jest.fn(),
+            persistence: new PostHogPersistence(given.config),
+        }))
+
+        beforeEach(() => {
+            jest.spyOn(given.lib.featureFlags, 'setGroupPropertiesForFlags')
+            jest.spyOn(given.lib.featureFlags, 'setReloadingPaused')
+            jest.spyOn(given.lib.featureFlags, 'resetRequestQueue')
+            jest.spyOn(given.lib.featureFlags, '_reloadFeatureFlagsRequest')
+        })
+
+        it('doesnt call flags while initial load is happening', () => {
+            given.subject()
+
+            jest.runAllTimers()
+
+            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.setReloadingPaused).toHaveBeenCalledWith(false)
+
+            // even if the decide request returned late, we should not call _reloadFeatureFlagsRequest
+            // because it ought to be paused until decide returns
+            expect(given.overrides._send_request).toHaveBeenCalledTimes(1)
+            expect(given.lib.featureFlags._reloadFeatureFlagsRequest).toHaveBeenCalledTimes(0)
+        })
+    })
+
+    it('toggles feature flags on and off', () => {
+        given.subject()
+
+        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.receivedFeatureFlags).toHaveBeenCalledTimes(1)
+    })
+})

src/decide.ts

@@ -39,6 +39,9 @@ export class Decide {
     }
 
     parseDecideResponse(response: DecideResponse): void {
+        this.instance.featureFlags.resetRequestQueue()
+        this.instance.featureFlags.setReloadingPaused(false)
+
         if (response?.status === 0) {
             console.error('Failed to fetch feature flags from PostHog.')
             return

src/posthog-core.ts

@@ -524,7 +524,10 @@ export class PostHog {
         // Pause `reloadFeatureFlags` calls in config.loaded callback.
         // These feature flags are loaded in the decide call made right
         // afterwards
-        this.featureFlags.setReloadingPaused(true)
+        const disableDecide = this.get_config('advanced_disable_decide')
+        if (!disableDecide) {
+            this.featureFlags.setReloadingPaused(true)
+        }
 
         try {
             this.get_config('loaded')(this)
@@ -543,12 +546,9 @@ export class PostHog {
         // 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 (!this.get_config('advanced_disable_decide')) {
+        if (!disableDecide) {
             new Decide(this).call()
         }
-
-        this.featureFlags.resetRequestQueue()
-        this.featureFlags.setReloadingPaused(false)
     }
 
     _start_queue_if_opted_in(): void {