fix(flags): Enqueue follow up requests without dropping (#797)

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
 

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,60 @@ 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: '[email protected]' },
+                token,
+            },
+        ])
+    })
+})
+
+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: '[email protected]' })
+            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: '[email protected]',
+                },
+                group_properties: {
+                    playlist: {
+                        length: 8,
+                    },
+                },
+                token,
+            },
+        ])
+    })
 })

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) =>

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' }),

src/__tests__/posthog-core.js

@@ -893,6 +893,7 @@ describe('_loaded()', () => {
         featureFlags: {
             setReloadingPaused: jest.fn(),
             resetRequestQueue: jest.fn(),
+            _startReloadTimer: jest.fn(),
         },
         _start_queue_if_opted_in: jest.fn(),
     }))

src/__tests__/posthog-core.loaded.js

@@ -18,6 +18,7 @@ describe('loaded() with flags', () => {
         featureFlags: {
             setReloadingPaused: jest.fn(),
             resetRequestQueue: jest.fn(),
+            _startReloadTimer: jest.fn(),
             receivedFeatureFlags: jest.fn(),
         },
         _start_queue_if_opted_in: jest.fn(),
@@ -48,24 +49,30 @@ describe('loaded() with flags', () => {
         beforeEach(() => {
             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')
         })
 
         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 +81,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)
     })
 })

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.')

src/posthog-core.ts

@@ -548,6 +548,10 @@ export class PostHog {
         // 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 above.
+            this.featureFlags.resetRequestQueue()
         }
     }