Repository navigation
feat(feature-flags)!: Evaluate ordered targeting rules #1717
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 |
|---|---|---|
| @@ -1,7 +1,7 @@ | ||
| import { FlagPollEntry } from './flag-poll-response.interface'; | ||
| import { FlagPollEntryV2 } from './flag-poll-response.interface'; | ||
|
|
||
| export interface FlagChange { | ||
| key: string; | ||
| previous: FlagPollEntry | null; | ||
| current: FlagPollEntry | null; | ||
| previous: FlagPollEntryV2 | null; | ||
| current: FlagPollEntryV2 | null; | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,4 +21,36 @@ export interface FlagPollEntry { | |
| }; | ||
| } | ||
|
|
||
| export type FlagPollResponse = Record<string, FlagPollEntry>; | ||
| export type FlagPollResponseV1 = Record<string, FlagPollEntry>; | ||
|
|
||
| export interface FlagConditionV2 { | ||
| // Future operators may carry different operands. Unsupported conditions | ||
| // never match, even if another condition in the rule matches. | ||
| operator: string; | ||
| target_type?: string; | ||
| values?: unknown; | ||
| } | ||
|
|
||
| export interface FlagRuleV2 { | ||
| id: string; | ||
| kind: string; | ||
| // Future multivariate values must fall back to the caller's boolean default. | ||
| value: unknown; | ||
| conditions?: FlagConditionV2[]; | ||
| } | ||
|
|
||
| export interface FlagPollEntryV2 { | ||
| slug: string; | ||
| enabled: boolean; | ||
| default_value: unknown; | ||
| off_value: unknown; | ||
| /** Evaluated in array order; the first matching rule wins. */ | ||
| rules: FlagRuleV2[]; | ||
| } | ||
|
|
||
| export interface FlagPollResponseV2 { | ||
| version: 2; | ||
| flags: Record<string, FlagPollEntryV2>; | ||
| } | ||
|
|
||
| export type FlagPollResponse = FlagPollResponseV1 | FlagPollResponseV2; | ||
|
Contributor
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.
Knowledge Base Used: Prompt To Fix With AIThis is a comment left during a code review.
Path: src/feature-flags/interfaces/flag-poll-response.interface.ts
Line: 56
Comment:
**Response alias breaks indexing**
`FlagPollResponse` now includes a v2 envelope without a string index signature. Existing TypeScript code that accepts this exported type and indexes it by flag slug will no longer compile. The README describes the `getFlag()` and change-event migration, but not this additional type change, leaving those callers without migration guidance.
**Knowledge Base Used:**
- [Feature flag evaluation](https://app.greptile.com/workos/-/custom-context/knowledge-base/workos/workos-node/-/docs/feature-flags.md)
- [SDK initialization and public entrypoints](https://app.greptile.com/workos/-/custom-context/knowledge-base/workos/workos-node/-/docs/sdk-foundation.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Contributor
Author
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. The type change is intentional as part of the major SDK update, but agreed that we’re missing migration guidance here. I’ll add a note to the README’s configuration migration section to use
Contributor
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. That migration note addresses the concern. Please make the distinction explicit: legacy flat maps should use |
||
Uh oh!
There was an error while loading. Please reload this page.