feat(flags): ConfigurationWire parse + precomputed decode [PR1] (FFL-2686, FFL-2687)#1330
feat(flags): ConfigurationWire parse + precomputed decode [PR1] (FFL-2686, FFL-2687)#1330btthomas wants to merge 8 commits into
Conversation
|
de2a6ec to
2d20825
Compare
There was a problem hiding this comment.
Pull request overview
This PR introduces an internal (non-exported) flags/configuration module that enables offline feature-flag initialization by parsing a portable ConfigurationWire string and decoding precomputed CDN assignments into the SDK’s existing FlagCacheEntry shape.
Changes:
- Added
configurationFromString/configurationToStringfor lenient parsing/serialization ofConfigurationWirev1. - Added
decodePrecomputedFlags(+UnsupportedConfigurationError) to map precomputed CDN assignments toRecord<string, FlagCacheEntry>, including variation-type validation and prototype-safe key handling. - Added types and unit tests for wire round-trip, unsupported/malformed inputs, variation-type decoding, and obfuscation rejection.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/core/src/flags/configuration/wire.ts | Lenient parse/serialize helpers for ConfigurationWire v1 and embedded precomputed response JSON. |
| packages/core/src/flags/configuration/types.ts | Defines wire envelope types, supported versions/variation types, and parsed configuration shapes. |
| packages/core/src/flags/configuration/precomputed.ts | Decoder from precomputed CDN response to FlagCacheEntry map, with validation and predictable failures. |
| packages/core/src/flags/configuration/index.ts | Internal module boundary + re-exports for configuration parsing/decoding primitives. |
| packages/core/src/flags/configuration/tests/wire.test.ts | Unit tests for wire parsing behavior and serialization round-trip. |
| packages/core/src/flags/configuration/tests/precomputed.test.ts | Unit tests for decoding across variation types, obfuscation rejection, and prototype-safe keys. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const toFlagCacheEntry = ( | ||
| key: string, | ||
| flag: PrecomputedFlag | ||
| ): FlagCacheEntry | null => { | ||
| const { variationType, variationValue } = flag; | ||
|
|
| // `serialId` is intentionally not propagated: `FlagCacheEntry` has no slot for it | ||
| // and the native CDN-fetched snapshot omits it too, so dropping it keeps | ||
| // offline/online parity. | ||
| return { | ||
| key, | ||
| value: variationValue, | ||
| allocationKey: flag.allocationKey, | ||
| variationKey: flag.variationKey, | ||
| variationType, | ||
| variationValue: stringifyValue(variationValue), | ||
| reason: flag.reason, | ||
| doLog: flag.doLog, | ||
| extraLogging: flag.extraLogging ?? {} | ||
| }; |
| return { | ||
| key, | ||
| value: variationValue, | ||
| allocationKey: flag.allocationKey, |
There was a problem hiding this comment.
Perhaps you could extract all this from the flag decomposition you are already doing above so the return object looks a bit cleaner:
const { variationType, variationKey, allocationKey, variationValue, reason, doLog, extraLogging } = flag;
...
return {
key,
value: variationValue,
allocationKey,
variationKey,
variationType,
variationValue: stringifyValue(variationValue),
reason,
doLog,
extraLogging: extraLogging ?? {}
};
| export const configurationToString = ( | ||
| configuration: ParsedFlagsConfiguration | ||
| ): string => { | ||
| const wire: ConfigurationWire = { version: 1 }; |
There was a problem hiding this comment.
Should this version be a const somewhere more prominent?
|
|
||
| return configuration; | ||
| } catch { | ||
| return {}; |
There was a problem hiding this comment.
Should this log an InternalLog message communicating that the parsing failed?
| /** | ||
| * The set of `ConfigurationWire` versions this SDK can parse. A known set (rather | ||
| * than a single hardcoded value) leaves room for the future `server`/rules format | ||
| * to bump the version without forcing a parser change. | ||
| */ | ||
| export const SUPPORTED_WIRE_VERSIONS: ReadonlySet<number> = new Set([1]); |
There was a problem hiding this comment.
minor: overengineering. the parser is going to change for version changes, so this doesn't achieve its stated purpose
| * {@link configurationToString}. | ||
| */ | ||
| export interface ConfigurationWire { | ||
| version: number; |
There was a problem hiding this comment.
major: future versions might have different incompatible shape, so number is wrong here
| version: number; | |
| version: 1; |
| case 'object': | ||
| // Object flags are a JSON object at the root; arrays are not valid values. | ||
| return ( | ||
| typeof value === 'object' && | ||
| value !== null && | ||
| !Array.isArray(value) | ||
| ); |
There was a problem hiding this comment.
major: despite the name, object can be arbitrary type, not necessarily an object. Arrays, numbers, strings, nulls are fine here
| case 'object': | |
| // Object flags are a JSON object at the root; arrays are not valid values. | |
| return ( | |
| typeof value === 'object' && | |
| value !== null && | |
| !Array.isArray(value) | |
| ); | |
| case 'object': | |
| return true; |
| */ | ||
| export interface ParsedFlagsConfiguration { | ||
| precomputed?: ParsedPrecomputedConfiguration; | ||
| // server?: ParsedServerConfiguration; // future rules/UFC branch — not implemented |
| server?: { | ||
| response: string; | ||
| fetchedAt?: number; | ||
| }; |
There was a problem hiding this comment.
| server?: { | |
| response: string; | |
| fetchedAt?: number; | |
| }; |
| * the only public entry/exit points are {@link configurationFromString} / | ||
| * {@link configurationToString}. | ||
| */ | ||
| export interface ConfigurationWire { |
There was a problem hiding this comment.
major: is there anything that prevents reuse of types and parsing functions from @datadog/flagging-core?
There was a problem hiding this comment.
I don't think so. Isn't @sameerank working on extracting something from something to make it so I can import it when we tackle dynamic offline? Maybe the evaluation logic? Should I import now or wait for the whole thing to be ready?
There was a problem hiding this comment.
…86, FFL-2687)
Internal, pure-JS building blocks for offline init (kept un-exported until the
exports step):
- configurationFromString / configurationToString for the ConfigurationWire v1
envelope, with a ParsedFlagsConfiguration type (distinct from the enable()
FlagsConfiguration). Lenient parse: returns {} on malformed input or an
unsupported version; accepts a known set of versions.
- decodePrecomputedFlags: maps a precomputed CDN response to the existing
FlagCacheEntry map. Injects key from the flag map key; value = typed
variationValue (integer/float -> JS number, variationType string preserved);
derives the string variationValue (JSON for objects, lowercase booleans) for
Android exposure round-trip; validates variationType and omits unknowns; throws
UnsupportedConfigurationError on obfuscated payloads.
Unit tests cover parse/round-trip, unsupported version/invalid JSON, and decode
across all variation types incl. obfuscation and mismatch handling.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…y object values Review follow-ups on PR1: - H1: accumulate decoded flags in a Map and materialize with Object.fromEntries, so a flag keyed "__proto__" is stored as data instead of hitting the Object.prototype setter (which silently dropped it and reassigned the proto). - H2: reject array values for object-typed flags (object flags are a JSON object at the root). Arrays are omitted plus logged, like other type mismatches. Adds regression tests for both. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ta (review M/L) Remaining PR1 review follow-ups: - M1: integer flags require a whole number; number/float require a finite value (reject NaN/Infinity) so native parsers round-trip. - M2: document that serialId is intentionally not propagated (no FlagCacheEntry slot; native snapshot omits it too). - L3: comment that only flags (and obfuscated) are load-bearing; the rest of the response attributes are optional/ignored on purpose. - L1/L5: broaden round-trip fixture (number + nested object flags) and add tests for fractional integer, non-finite number, and a structurally broken response. L2 (distinguishing parse-failure from valid-but-empty) is intentionally left to PR2 (FFL-2688). L4 (configurationToString vs the reference's bug) is correct as-is. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Model the wire version as the literal type `version: 1` instead of `number`, and drop the SUPPORTED_WIRE_VERSIONS Set in favor of a direct `parsed.version !== 1` check. The literal type is now the single, type-checked source for the supported version (configurationToString's `{ version: 1 }` is validated against it), so no separate constant is needed. A future incompatible wire shape should be modeled as a versioned discriminated union rather than a widened number.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Drop the unimplemented `server` (rules/UFC) branch from ConfigurationWire and ParsedFlagsConfiguration, along with its doc/parse comments and the server-only test. It was speculative future scope (YAGNI) with no reader. The remaining 'no precomputed branch yields empty config' behavior is still covered by the adjacent empty-wire test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…s (PR1 review) - Guard toFlagCacheEntry against non-object flag entries (e.g. a null value) so one malformed flag is skipped with a warning instead of throwing and aborting the whole decode. - Validate the forwarded metadata fields (allocationKey, variationKey, reason, doLog, extraLogging) and omit the flag when their types are wrong, so a corrupt payload cannot propagate bad data into evaluation/tracking. - Destructure the flag once and build the cache entry from the locals for a cleaner return object. - Log an InternalLog WARN when configurationFromString fails to parse, instead of swallowing the error silently. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The decoder previously required an object variation value to be a non-array, non-null object. ffe-service enforces a top-level object at the API layer, but that is not a storage constraint, so a precomputed payload could carry any JSON value. Accept it defensively. Traced the downstream paths: stringifyValue handles arrays/null/primitives, and getDetails validates the value type at evaluation time (typeof flag.value), so a non-object value under an object flag falls back to the default via TYPE_MISMATCH rather than mis-serving. Native tracking only runs post type-check. No path throws. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2d20825 to
1009719
Compare
340691b to
3a284a7
Compare
…PR1 review) Depend on @datadog/flagging-core (and @openfeature/core, which its published types reference) and reuse its canonical wire types and configurationFromString instead of maintaining our own copies. Our local type names are kept as aliases so the rest of the SDK is insulated from upstream naming. configurationToString stays local for now, with a loud TODO to adopt flagging-core's once the next major (>= 2.0.0) ships the fix from openfeature-js-client PR #331 (its 1.2.x serializer is broken). Consequences of delegating to flagging-core's parser: the bespoke parse-failure InternalLog warning is dropped (its parser swallows errors), and obfuscated is read via a small cast since it is absent from flagging-core's response type. integer/float variation types are kept as defensive runtime handling even though the CDN and flagging-core model only boolean/string/number/object. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Offline feature flags — related PRs
FlagsClient.setConfiguration): feat(flags): FlagsClient.setConfiguration + context matching [PR2] (FFL-2688) #1331Summary
Adds the pure-JS building blocks for offline feature-flag initialization in the React Native SDK (FFL-2686, FFL-2687): parsing a portable
ConfigurationWirestring and decoding its precomputed assignments into the SDK's internal flag-cache shape. These let a customer load a configuration they fetched themselves and evaluate flags without a network request.New internal module
packages/core/src/flags/configuration/(not yet exported):wire.ts—configurationFromString(wire): ParsedFlagsConfigurationandconfigurationToString(config). Parses theConfigurationWirev1 envelope and is lenient: it returns an empty configuration on malformed input or an unsupported version rather than throwing.ParsedFlagsConfigurationis a distinct type (not theenable()FlagsConfiguration) and reserves aserver/rules branch for the future.precomputed.ts—decodePrecomputedFlags(response)→Record<string, FlagCacheEntry>(the shape the SDK already caches and evaluates against). The typedvariationValuebecomes the flagvalueplus a stringified form used by native Android exposure tracking;integer/floatdecode to a JSnumberwhile preserving the originalvariationType; unknown variation types are omitted; obfuscated payloads fail predictably.types.ts/index.ts— types and the internal module barrier.Tests
Unit tests cover wire parse + round-trip, unsupported version / invalid JSON, decode across every variation type, prototype-safe key handling, and obfuscation rejection. Lint and
tscare clean.