fix(flags): handle plain array and object forms in overrideFeatureFlags#3245
Merged
fix(flags): handle plain array and object forms in overrideFeatureFlags#3245
overrideFeatureFlags#3245Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
Contributor
|
Size Change: +4.04 kB (+0.06%) Total Size: 6.57 MB
ℹ️ View Unchanged
|
Contributor
Prompt To Fix All With AIThis is a comment left during a code review.
Path: packages/browser/src/posthog-featureflags.ts
Line: 973-983
Comment:
**Duplicated array-to-flags-object conversion**
The `for` loop that converts a string array into a `Record<string, string | boolean>` is now written twice — once here for the top-level array form and again inside the `{ flags: [...] }` branch at lines 999–1004. This violates OnceAndOnlyOnce and means any future change (e.g. trimming flag names, logging) must be applied in both places.
Consider extracting a small helper:
```ts
function arrayToFlagsRecord(flags: string[]): Record<string, string | boolean> {
const flagsObj: Record<string, string | boolean> = {}
for (let i = 0; i < flags.length; i++) {
flagsObj[flags[i]] = true
}
return flagsObj
}
```
Then both call sites become a single `arrayToFlagsRecord(...)` call.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: packages/browser/src/__tests__/featureflags.test.ts
Line: 961-995
Comment:
**Prefer parameterised tests**
The first two cases follow an identical structure (call `overrideFeatureFlags` with an input, assert `getFlagVariants()` equals an expected object). Per project convention, these are a good candidate for `it.each`:
```ts
it.each([
[
'plain array',
['beta-feature', 'alpha-feature-2'],
{ 'beta-feature': true, 'alpha-feature-2': true },
],
[
'plain object',
{ 'beta-feature': 'variant-1', 'alpha-feature-2': false },
{ 'beta-feature': 'variant-1', 'alpha-feature-2': false },
],
])('supports %s shorthand form for flag overrides', (_, input, expected) => {
featureFlags.overrideFeatureFlags(input as any)
expect(featureFlags.getFlagVariants()).toEqual(expected)
})
```
The third test ("plain object does not affect payloads") checks a distinct concern and is fine as a standalone `it`.
How can I resolve this? If you propose a fix, please make it concise.Last reviewed commit: 1ccd3ec |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
- Extract arrayToFlagsRecord helper to eliminate duplicated array-to-flags-object conversion - Refactor tests to use it.each for parameterised testing
gustavohstrassburger
approved these changes
Mar 18, 2026
Contributor
gustavohstrassburger
left a comment
gustavohstrassburger
left a comment
There was a problem hiding this comment.
LGTM, some inline comments that you might want to address, but it's up to you.
| const PERSISTENCE_FEATURE_FLAG_REQUEST_ID = '$feature_flag_request_id' | ||
|
|
||
| /** Converts an array of flag names to a Record where each flag is set to true. */ | ||
| const arrayToFlagsRecord = (flags: string[]): Record<string, string | boolean> => { |
Contributor
There was a problem hiding this comment.
The return type always has true in the values of the object, so string | boolean is actually broader than it could be.
| return forceDebugLogger.info('Flag overrides set', { flags: overrideOptions }) | ||
| } | ||
|
|
||
| this._fireFeatureFlagsCallbacks() |
Contributor
There was a problem hiding this comment.
In any scenario, is it expected to get here?
If not, we should probably add a log here.
And maybe we shouldn't even be calling _fireFeatureFlagsCallbacks (I wouldn't change that, but this seems like an error).
- Narrow arrayToFlagsRecord return type from Record<string, string | boolean> to Record<string, true> - Replace unreachable _fireFeatureFlagsCallbacks call with warning log for invalid overrideOptions
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
overrideFeatureFlagsin the browser SDK accepted plain arrays and plain objects in its type signature (OverrideFeatureFlagsOptions), but the implementation only handled two cases:false→ clear overrides{ flags: ..., payloads: ... }→ the explicit formPassing a plain array like
['beta-feature']or a plain object like{'beta-feature': 'variant'}would silently do nothing — the code checked for'flags' in overrideOptionswhich wasfalsefor these forms, so it fell through without registering any overrides.This was reported by a customer who followed the (incorrect) docs and got no errors but no overrides either.
Root cause
When
overrideFeatureFlagswas introduced in #1697, it only implemented the explicit{ flags: ... }form internally. The deprecatedoverride()method papered over this by wrapping its arguments in{ flags: ... }before forwarding, so the old API kept working. But the type union and JSDoc both advertised the plain forms as valid for the new method — they just never worked.When
overrideFeatureFlagswas later added to the node SDK in #2840, the contributor implemented all the forms the types promised (arrays, plain objects, and explicit), so the two SDKs diverged.Fix
Add the missing array and plain-object handling to the browser implementation, matching what the node SDK already does:
['flag-a', 'flag-b']): converts to{ 'flag-a': true, 'flag-b': true }and registers{'flag-a': 'variant'}): registers directly as flag overrides (fallback after theflags/payloadscheck)Also fixes the JSDoc in both
posthog-featureflags.tsandposthog.tsto accurately document all supported forms.Changes
packages/browser/src/posthog-featureflags.ts— Add array and plain-object branches tooverrideFeatureFlags, fix JSDocpackages/types/src/posthog.ts— Fix JSDoc examplespackages/browser/src/__tests__/featureflags.test.ts— Add 3 tests for the new shorthand forms