-
Notifications
You must be signed in to change notification settings - Fork 160
Allow passing groups to feature flags #2010
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -29,7 +29,16 @@ describe('useFeatureFlagPayload hook', () => { | |
|
||
given('posthog', () => ({ | ||
isFeatureEnabled: (flag) => !!FEATURE_FLAG_STATUS[flag], | ||
getFeatureFlag: (flag) => FEATURE_FLAG_STATUS[flag], | ||
getFeatureFlag: (flag, options) => { | ||
// Mock that groups options are passed through | ||
if (options && options.groups) { | ||
// Return different values when groups are passed to test the functionality | ||
if (flag === 'group_specific_flag' && options.groups.team === '123') { | ||
return 'group_variant' | ||
} | ||
Comment on lines
+35
to
+38
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: Magic string 'group_variant' should be defined as a constant at the top of the file |
||
} | ||
return FEATURE_FLAG_STATUS[flag] | ||
}, | ||
getFeatureFlagPayload: (flag) => FEATURE_FLAG_PAYLOADS[flag], | ||
onFeatureFlags: (callback) => { | ||
const activeFlags = [] | ||
|
@@ -90,4 +99,18 @@ describe('useFeatureFlagPayload hook', () => { | |
}) | ||
expect(result.current).toEqual(expected) | ||
}) | ||
|
||
it('should pass groups to feature flag variant key', () => { | ||
let { result } = renderHook(() => useFeatureFlagVariantKey('group_specific_flag', { groups: { team: '123' } }), { | ||
wrapper: given.renderProvider, | ||
}) | ||
expect(result.current).toEqual('group_variant') | ||
}) | ||
|
||
it('should pass groups to feature flag enabled check', () => { | ||
let { result } = renderHook(() => useFeatureFlagEnabled('group_specific_flag', { groups: { team: '123' } }), { | ||
wrapper: given.renderProvider, | ||
}) | ||
expect(result.current).toEqual(true) | ||
}) | ||
}) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,16 +1,21 @@ | ||
import { useEffect, useState } from 'react' | ||
import { usePostHog } from './usePostHog' | ||
|
||
export function useFeatureFlagEnabled(flag: string): boolean | undefined { | ||
export function useFeatureFlagEnabled( | ||
flag: string, | ||
options?: { groups?: Record<string, string> } | ||
): boolean | undefined { | ||
const client = usePostHog() | ||
|
||
const [featureEnabled, setFeatureEnabled] = useState<boolean | undefined>(() => client.isFeatureEnabled(flag)) | ||
const [featureEnabled, setFeatureEnabled] = useState<boolean | undefined>(() => | ||
!!client.getFeatureFlag(flag, { send_event: false, ...options }) | ||
) | ||
|
||
useEffect(() => { | ||
return client.onFeatureFlags(() => { | ||
setFeatureEnabled(client.isFeatureEnabled(flag)) | ||
setFeatureEnabled(!!client.getFeatureFlag(flag, { send_event: false, ...options })) | ||
}) | ||
}, [client, flag]) | ||
}, [client, flag, options]) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. logic: including 'options' in deps array may cause infinite re-renders if object is recreated on each render |
||
|
||
return featureEnabled | ||
} |
Original file line number | Diff line number | Diff line change | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -2,7 +2,10 @@ import { useEffect, useState } from 'react' | |||||||||||||
import { JsonType } from 'posthog-js' | ||||||||||||||
import { usePostHog } from './usePostHog' | ||||||||||||||
|
||||||||||||||
export function useFeatureFlagPayload(flag: string): JsonType { | ||||||||||||||
export function useFeatureFlagPayload( | ||||||||||||||
flag: string, | ||||||||||||||
options?: { groups?: Record<string, string> } | ||||||||||||||
): JsonType { | ||||||||||||||
const client = usePostHog() | ||||||||||||||
|
||||||||||||||
const [featureFlagPayload, setFeatureFlagPayload] = useState<JsonType>(() => client.getFeatureFlagPayload(flag)) | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. logic: Initial call to getFeatureFlagPayload() is missing the groups option, causing inconsistency with subsequent updates
Suggested change
|
||||||||||||||
|
@@ -11,7 +14,7 @@ export function useFeatureFlagPayload(flag: string): JsonType { | |||||||||||||
return client.onFeatureFlags(() => { | ||||||||||||||
setFeatureFlagPayload(client.getFeatureFlagPayload(flag)) | ||||||||||||||
}) | ||||||||||||||
Comment on lines
14
to
16
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. logic: The groups option is not being passed to getFeatureFlagPayload() here, which means group-based evaluation won't work despite the parameter being added to the function signature
Suggested change
|
||||||||||||||
}, [client, flag]) | ||||||||||||||
}, [client, flag, options]) | ||||||||||||||
|
||||||||||||||
return featureFlagPayload | ||||||||||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,18 +1,21 @@ | ||
import { useEffect, useState } from 'react' | ||
import { usePostHog } from './usePostHog' | ||
|
||
export function useFeatureFlagVariantKey(flag: string): string | boolean | undefined { | ||
export function useFeatureFlagVariantKey( | ||
flag: string, | ||
options?: { groups?: Record<string, string> } | ||
): string | boolean | undefined { | ||
const client = usePostHog() | ||
|
||
const [featureFlagVariantKey, setFeatureFlagVariantKey] = useState<string | boolean | undefined>(() => | ||
client.getFeatureFlag(flag) | ||
client.getFeatureFlag(flag, { send_event: false, ...options }) | ||
) | ||
Comment on lines
10
to
12
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: Consider memoizing options object with useMemo to prevent unnecessary re-renders when passing inline objects |
||
|
||
useEffect(() => { | ||
return client.onFeatureFlags(() => { | ||
setFeatureFlagVariantKey(client.getFeatureFlag(flag)) | ||
setFeatureFlagVariantKey(client.getFeatureFlag(flag, { send_event: false, ...options })) | ||
}) | ||
}, [client, flag]) | ||
}, [client, flag, options]) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. logic: Adding 'options' to deps array could cause infinite loops if options object is created inline - needs memoization |
||
|
||
return featureFlagVariantKey | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,6 +12,7 @@ import { SessionIdManager } from '../sessionid' | |
import { RequestQueue } from '../request-queue' | ||
import { SessionRecording } from '../extensions/replay/sessionrecording' | ||
import { SessionPropsManager } from '../session-props' | ||
import { isArray } from '../utils/type-utils' | ||
|
||
let mockGetProperties: jest.Mock | ||
|
||
|
@@ -1115,6 +1116,7 @@ describe('posthog core', () => { | |
posthog._requestQueue = { | ||
enqueue: jest.fn(), | ||
} as unknown as RequestQueue | ||
jest.spyOn(posthog as any, '_send_retriable_request').mockImplementation(jest.fn()) | ||
}) | ||
|
||
it('sends group information in event properties', () => { | ||
|
@@ -1123,9 +1125,9 @@ describe('posthog core', () => { | |
|
||
posthog.capture('some_event', { prop: 5 }) | ||
|
||
expect(posthog._requestQueue!.enqueue).toHaveBeenCalledTimes(1) | ||
|
||
const eventPayload = jest.mocked(posthog._requestQueue!.enqueue).mock.calls[0][0] | ||
const eventPayload = | ||
jest.mocked(posthog._requestQueue!.enqueue).mock.calls[0]?.[0] || | ||
jest.mocked(posthog._send_retriable_request as any).mock.calls[0][0] | ||
Comment on lines
+1128
to
+1130
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: Consider refactoring this payload extraction logic into a test helper function since it's used multiple times |
||
// need to help TS know event payload data is not an array | ||
// eslint-disable-next-line posthog-js/no-direct-array-check | ||
if (Array.isArray(eventPayload.data!)) { | ||
|
@@ -1137,6 +1139,18 @@ describe('posthog core', () => { | |
instance: 'app.posthog.com', | ||
}) | ||
}) | ||
|
||
it('supports groups passed via capture options', () => { | ||
posthog.capture('some_event', {}, { groups: { team: '1' } }) | ||
|
||
const eventPayload = | ||
jest.mocked(posthog._requestQueue!.enqueue).mock.calls[0]?.[0] || | ||
jest.mocked(posthog._send_retriable_request as any).mock.calls[0][0] | ||
if (isArray(eventPayload.data!)) { | ||
throw new Error('') | ||
} | ||
expect(eventPayload.data!.properties.$groups).toEqual({ team: '1' }) | ||
}) | ||
}) | ||
|
||
describe('error handling', () => { | ||
|
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -472,9 +472,12 @@ export class PostHogFeatureFlags { | |||||||||
* if(posthog.getFeatureFlag('my-flag') === 'some-variant') { // do something } | ||||||||||
* | ||||||||||
* @param {Object|String} key Key of the feature flag. | ||||||||||
* @param {Object|String} options (optional) If {send_event: false}, we won't send an $feature_flag_called event to PostHog. | ||||||||||
* @param {Object|String} options (optional) If {send_event: false}, we won't send an $feature_flag_called event to PostHog. If {groups: {group_type: group_key}}, we will use the group key to evaluate the flag. | ||||||||||
*/ | ||||||||||
Comment on lines
+475
to
476
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: JSDoc param type doesn't match updated TypeScript type (Object|String vs { send_event?: boolean; groups?: Record<string, string> })
Suggested change
|
||||||||||
getFeatureFlag(key: string, options: { send_event?: boolean } = {}): boolean | string | undefined { | ||||||||||
getFeatureFlag( | ||||||||||
key: string, | ||||||||||
options: { send_event?: boolean; groups?: Record<string, string> } = {} | ||||||||||
): boolean | string | undefined { | ||||||||||
if (!this._hasLoadedFlags && !(this.getFlags() && this.getFlags().length > 0)) { | ||||||||||
logger.warn('getFeatureFlag for key "' + key + '" failed. Feature flags didn\'t load in time.') | ||||||||||
return undefined | ||||||||||
|
@@ -533,7 +536,11 @@ export class PostHogFeatureFlags { | |||||||||
properties.$feature_flag_original_payload = flagDetails?.metadata?.original_payload | ||||||||||
} | ||||||||||
|
||||||||||
this._instance.capture('$feature_flag_called', properties) | ||||||||||
if (options.groups) { | ||||||||||
this._instance.capture('$feature_flag_called', properties, { groups: options.groups }) | ||||||||||
} else { | ||||||||||
this._instance.capture('$feature_flag_called', properties) | ||||||||||
} | ||||||||||
} | ||||||||||
} | ||||||||||
return flagValue | ||||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Consider moving variant call up to parent component to avoid duplicate feature flag evaluations (already called in lines 28-29)