diff --git a/src/__tests__/decide.js b/src/__tests__/decide.js index 0790c9dfe..4f8493b23 100644 --- a/src/__tests__/decide.js +++ b/src/__tests__/decide.js @@ -151,19 +151,19 @@ describe('Decide', () => { expect(given.posthog.sessionRecording.afterDecideResponse).toHaveBeenCalledWith(given.decideResponse) expect(given.posthog.toolbar.afterDecideResponse).toHaveBeenCalledWith(given.decideResponse) - expect(given.posthog.featureFlags.receivedFeatureFlags).toHaveBeenCalledWith(given.decideResponse) + expect(given.posthog.featureFlags.receivedFeatureFlags).toHaveBeenCalledWith(given.decideResponse, false) expect(given.posthog._afterDecideResponse).toHaveBeenCalledWith(given.decideResponse) expect(autocapture.afterDecideResponse).toHaveBeenCalledWith(given.decideResponse, given.posthog) }) - it('Make sure receivedFeatureFlags is not called if the decide response fails', () => { + it('Make sure receivedFeatureFlags is called with errors if the decide response fails', () => { given('decideResponse', () => undefined) window.POSTHOG_DEBUG = true console.error = jest.fn() given.subject() - expect(given.posthog.featureFlags.receivedFeatureFlags).not.toHaveBeenCalled() + expect(given.posthog.featureFlags.receivedFeatureFlags).toHaveBeenCalledWith({}, true) expect(console.error).toHaveBeenCalledWith('[PostHog.js]', 'Failed to fetch feature flags from PostHog.') }) diff --git a/src/__tests__/featureflags.js b/src/__tests__/featureflags.js index b95445321..df2ce778a 100644 --- a/src/__tests__/featureflags.js +++ b/src/__tests__/featureflags.js @@ -26,14 +26,17 @@ describe('featureflags', () => { get_property: (key) => given.instance.persistence.props[key], capture: () => {}, decideEndpointWasHit: given.decideEndpointWasHit, - _send_request: jest - .fn() - .mockImplementation(({ callback }) => callback({ statusCode: 200, json: given.decideResponse })), + _send_request: jest.fn().mockImplementation(({ callback }) => callback(given.decideResponsePayload)), reloadFeatureFlags: () => given.featureFlags.reloadFeatureFlags(), })) given('featureFlags', () => new PostHogFeatureFlags(given.instance)) + given('decideResponsePayload', () => ({ + statusCode: 200, + json: given.decideResponse, + })) + beforeEach(() => { jest.spyOn(given.instance, 'capture').mockReturnValue() jest.spyOn(window.console, 'warn').mockImplementation() @@ -226,11 +229,13 @@ describe('featureflags', () => { var called = false let _flags = [] let _variants = {} + let _error = undefined - given.featureFlags.onFeatureFlags((flags, variants) => { + given.featureFlags.onFeatureFlags((flags, variants, errors) => { called = true _flags = flags _variants = variants + _error = errors?.errorsLoading }) expect(called).toEqual(false) @@ -239,6 +244,7 @@ describe('featureflags', () => { jest.runAllTimers() expect(called).toEqual(true) + expect(_error).toEqual(false) expect(_flags).toEqual(['first', 'second']) expect(_variants).toEqual({ first: 'variant-1', @@ -365,8 +371,11 @@ describe('featureflags', () => { expect(given.instance.persistence.props.$early_access_features).toEqual([EARLY_ACCESS_FEATURE_FIRST]) - given('decideResponse', () => ({ - earlyAccessFeatures: [EARLY_ACCESS_FEATURE_SECOND], + given('decideResponsePayload', () => ({ + statusCode: 200, + json: { + earlyAccessFeatures: [EARLY_ACCESS_FEATURE_SECOND], + }, })) // request again, should call _send_request because we're forcing a reload @@ -749,6 +758,116 @@ describe('featureflags', () => { }) }) }) + + describe('when decide times out or errors out', () => { + given('decideResponsePayload', () => ({ + statusCode: 500, + text: 'Internal Server Error', + })) + + it('should not change the existing flags', () => { + given.instance.persistence.register({ + $enabled_feature_flags: { + 'beta-feature': true, + 'random-feature': 'xatu', + }, + }) + + given.featureFlags.reloadFeatureFlags() + jest.runAllTimers() + + expect(given.featureFlags.getFlagVariants()).toEqual({ + 'beta-feature': true, + 'random-feature': 'xatu', + }) + }) + + it('should call onFeatureFlags even when decide errors out', () => { + var called = false + let _flags = [] + let _variants = {} + let _errors = undefined + + given.instance.persistence.register({ + $enabled_feature_flags: {}, + }) + + given.featureFlags.onFeatureFlags((flags, variants, errors) => { + called = true + _flags = flags + _variants = variants + _errors = errors?.errorsLoading + }) + expect(called).toEqual(false) + + given.featureFlags.reloadFeatureFlags() + + jest.runAllTimers() + expect(called).toEqual(true) + expect(_errors).toEqual(true) + expect(_flags).toEqual([]) + expect(_variants).toEqual({}) + }) + + it('should call onFeatureFlags with existing flags', () => { + var called = false + let _flags = [] + let _variants = {} + let _errors = undefined + + given.featureFlags.onFeatureFlags((flags, variants, errors) => { + called = true + _flags = flags + _variants = variants + _errors = errors?.errorsLoading + }) + expect(called).toEqual(false) + + given.featureFlags.reloadFeatureFlags() + + jest.runAllTimers() + expect(called).toEqual(true) + expect(_errors).toEqual(true) + expect(_flags).toEqual(['beta-feature', 'alpha-feature-2', 'multivariate-flag']) + expect(_variants).toEqual({ + 'beta-feature': true, + 'alpha-feature-2': true, + 'multivariate-flag': 'variant-1', + }) + }) + + it('should call onFeatureFlags with existing flags on timeouts', () => { + given('decideResponsePayload', () => ({ + statusCode: 0, + text: '', + })) + + var called = false + let _flags = [] + let _variants = {} + let _errors = undefined + + given.featureFlags.onFeatureFlags((flags, variants, errors) => { + called = true + _flags = flags + _variants = variants + _errors = errors?.errorsLoading + }) + expect(called).toEqual(false) + + given.featureFlags.reloadFeatureFlags() + + jest.runAllTimers() + expect(called).toEqual(true) + expect(_errors).toEqual(true) + expect(_flags).toEqual(['beta-feature', 'alpha-feature-2', 'multivariate-flag']) + expect(_variants).toEqual({ + 'beta-feature': true, + 'alpha-feature-2': true, + 'multivariate-flag': 'variant-1', + }) + }) + }) }) describe('parseFeatureFlagDecideResponse', () => { @@ -796,7 +915,7 @@ describe('parseFeatureFlagDecideResponse', () => { }) it('doesnt remove existing feature flags when no flags are returned', () => { - given('decideResponse', () => ({ status: 0 })) + given('decideResponse', () => ({})) given.subject() expect(given.persistence.register).not.toHaveBeenCalled() diff --git a/src/decide.ts b/src/decide.ts index 43b29560a..75469b320 100644 --- a/src/decide.ts +++ b/src/decide.ts @@ -48,7 +48,16 @@ export class Decide { // :TRICKY: Reload - start another request if queued! this.instance.featureFlags._startReloadTimer() - if (!response) { + 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 } @@ -65,13 +74,6 @@ export class Decide { autocapture.afterDecideResponse(response, this.instance) this.instance._afterDecideResponse(response) - if ( - !this.instance.config.advanced_disable_feature_flags_on_first_load && - !this.instance.config.advanced_disable_feature_flags - ) { - this.instance.featureFlags.receivedFeatureFlags(response) - } - // eslint-disable-next-line @typescript-eslint/ban-ts-comment // @ts-ignore const surveysGenerator = window?.extendPostHogWithSurveys diff --git a/src/posthog-featureflags.ts b/src/posthog-featureflags.ts index 2efb3bff7..c123bb263 100644 --- a/src/posthog-featureflags.ts +++ b/src/posthog-featureflags.ts @@ -199,15 +199,20 @@ export class PostHogFeatureFlags { callback: (response) => { this.setReloadingPaused(false) - if (response.statusCode !== 200) { - return // or error out?? - } - // reset anon_distinct_id after at least a single request with it - // makes it through - this.$anon_distinct_id = undefined - if (response.json) { - this.receivedFeatureFlags(response.json) + let errorsLoading = true + + if (response.statusCode === 200) { + // successful request + // reset anon_distinct_id after at least a single request with it + // makes it through + 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. + this.receivedFeatureFlags(response.json ?? {}, errorsLoading) // :TRICKY: Reload - start another request if queued! this._startReloadTimer() @@ -280,7 +285,7 @@ export class PostHogFeatureFlags { this.featureFlagEventHandlers = this.featureFlagEventHandlers.filter((h) => h !== handler) } - receivedFeatureFlags(response: Partial): void { + receivedFeatureFlags(response: Partial, errorsLoading?: boolean): void { if (!this.instance.persistence) { return } @@ -288,7 +293,7 @@ export class PostHogFeatureFlags { const currentFlags = this.getFlagVariants() const currentFlagPayloads = this.getFlagPayloads() parseFeatureFlagDecideResponse(response, this.instance.persistence, currentFlags, currentFlagPayloads) - this._fireFeatureFlagsCallbacks() + this._fireFeatureFlagsCallbacks(errorsLoading) } /* @@ -405,9 +410,9 @@ export class PostHogFeatureFlags { } } - _fireFeatureFlagsCallbacks(): void { + _fireFeatureFlagsCallbacks(errorsLoading?: boolean): void { const { flags, flagVariants } = this._prepareFeatureFlagsForCallbacks() - this.featureFlagEventHandlers.forEach((handler) => handler(flags, flagVariants)) + this.featureFlagEventHandlers.forEach((handler) => handler(flags, flagVariants, { errorsLoading })) } /** diff --git a/src/types.ts b/src/types.ts index 11335d599..189a18044 100644 --- a/src/types.ts +++ b/src/types.ts @@ -286,7 +286,13 @@ export interface DecideResponse { siteApps: { id: number; url: string }[] } -export type FeatureFlagsCallback = (flags: string[], variants: Record) => void +export type FeatureFlagsCallback = ( + flags: string[], + variants: Record, + context?: { + errorsLoading?: boolean + } +) => void // TODO: delete custom_properties after changeless typescript refactor export interface AutoCaptureCustomProperty {