Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(flags): Fire callback when decide errors out #1056

Merged
merged 3 commits into from
Mar 7, 2024
Merged

fix(flags): Fire callback when decide errors out #1056

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
080026b
fix(flags): Fire callback when decide errors out
neilkakkar Mar 7, 2024
774c9df
adjust decide callback
neilkakkar Mar 7, 2024
8f4e3dd
address comments
neilkakkar Mar 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/__tests__/decide.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.')
})

Expand Down
133 changes: 126 additions & 7 deletions src/__tests__/featureflags.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -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)

Expand All @@ -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',
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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', () => {
Expand Down Expand Up @@ -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()
Expand Down
18 changes: 10 additions & 8 deletions src/decide.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand All @@ -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
Expand Down
29 changes: 17 additions & 12 deletions src/posthog-featureflags.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels like a hack... Why call receivedFeatureFlags when from what I can see the only thing it needs is this._fireFeatureFlagsCallbacks(errorsLoading) - why not just call that directly?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also want all the other stuff that this function does (like setting decideEndpointWasHit) which then warns users when they call getFeatureFlag() that flags didn't load in time.

// 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()
Expand Down Expand Up @@ -280,15 +285,15 @@ export class PostHogFeatureFlags {
this.featureFlagEventHandlers = this.featureFlagEventHandlers.filter((h) => h !== handler)
}

receivedFeatureFlags(response: Partial<DecideResponse>): void {
receivedFeatureFlags(response: Partial<DecideResponse>, errorsLoading?: boolean): void {
if (!this.instance.persistence) {
return
}
this.instance.decideEndpointWasHit = true
const currentFlags = this.getFlagVariants()
const currentFlagPayloads = this.getFlagPayloads()
parseFeatureFlagDecideResponse(response, this.instance.persistence, currentFlags, currentFlagPayloads)
this._fireFeatureFlagsCallbacks()
this._fireFeatureFlagsCallbacks(errorsLoading)
}

/*
Expand Down Expand Up @@ -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 }))
}

/**
Expand Down
8 changes: 7 additions & 1 deletion src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -286,7 +286,13 @@ export interface DecideResponse {
siteApps: { id: number; url: string }[]
}

export type FeatureFlagsCallback = (flags: string[], variants: Record<string, string | boolean>) => void
export type FeatureFlagsCallback = (
flags: string[],
variants: Record<string, string | boolean>,
context?: {
errorsLoading?: boolean
}
) => void

// TODO: delete custom_properties after changeless typescript refactor
export interface AutoCaptureCustomProperty {
Expand Down
Loading