Send remote evaluation context as canonical JSON - #712
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The new Browser SDK ContextValue type permits undefined inside arrays (which serializes to null in JSON), creating a mismatch with “JSON-compatible” intent and potentially surprising contextJson semantics.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR updates both the Node and Browser SDKs to send remote evaluation context via a single canonical JSON query parameter (contextJson), enabling stable URLs/caching while supporting array/object-valued context attributes.
Changes:
- Add canonical JSON serialization that recursively sorts object keys while preserving array order, and use it to send
contextJsonfor remote flag evaluation requests. - Widen Browser SDK context types to accept JSON-compatible arrays/objects and update SSE/context serialization to use the canonical representation.
- Update Node/Browser SDK tests and add a changeset documenting the minor release.
File summaries
| File | Description |
|---|---|
| packages/node-sdk/test/utils.test.ts | Adds unit coverage for canonical JSON key sorting/array order behavior. |
| packages/node-sdk/test/client.test.ts | Updates remote evaluation URL assertions to use contextJson (including array context). |
| packages/node-sdk/src/utils.ts | Introduces canonicalJSONStringify() for stable canonical JSON serialization. |
| packages/node-sdk/src/client.ts | Switches remote evaluation requests to send context via contextJson. |
| packages/browser-sdk/test/flags.test.ts | Updates flag initialization tests to assert contextJson transport (including arrays). |
| packages/browser-sdk/test/client.test.ts | Updates mocked request assertions to read context from contextJson. |
| packages/browser-sdk/src/sse.ts | Replaces ad-hoc context serialization with canonicalContextJSONStringify(). |
| packages/browser-sdk/src/index.ts | Exports the new ContextValue type. |
| packages/browser-sdk/src/flag/flags.ts | Sends canonical contextJson for evaluated flags requests. |
| packages/browser-sdk/src/context.ts | Adds canonical context JSON serializer + widens context value typing for arrays/objects. |
| .changeset/context-json-sdk-transport.md | Declares minor versions for both SDKs for the transport/type changes. |
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟡 Changes recommended
canonical JSON serialization currently allows uncaught JSON.stringify exceptions (notably bigint/circular refs), creating regressions and inconsistent error surfacing versus the prior flattening transport.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 2
- Review effort level: Lite
| function canonicalJSONStringify(value: unknown): string { | ||
| const serialized = JSON.stringify(value, (_key, nestedValue: unknown) => { | ||
| if ( | ||
| nestedValue === null || | ||
| typeof nestedValue !== "object" || | ||
| Array.isArray(nestedValue) | ||
| ) { | ||
| return nestedValue; | ||
| } | ||
|
|
||
| return Object.fromEntries( | ||
| Object.entries(nestedValue).sort(([left], [right]) => | ||
| left < right ? -1 : left > right ? 1 : 0, | ||
| ), | ||
| ); | ||
| }); | ||
| if (serialized === undefined) | ||
| throw new Error("value must be JSON serializable"); | ||
| return serialized; | ||
| } |
| export function canonicalJSONStringify(value: unknown): string { | ||
| const serialized = JSON.stringify(value, (_key, nestedValue: unknown) => { | ||
| if ( | ||
| nestedValue === null || | ||
| typeof nestedValue !== "object" || | ||
| Array.isArray(nestedValue) | ||
| ) { | ||
| return nestedValue; | ||
| } | ||
|
|
||
| return Object.fromEntries( | ||
| Object.entries(nestedValue).sort(([left], [right]) => | ||
| left < right ? -1 : left > right ? 1 : 0, | ||
| ), | ||
| ); | ||
| }); | ||
| ok(serialized !== undefined, "value must be JSON serializable"); | ||
| return serialized; | ||
| } |
There was a problem hiding this comment.
🟡 Changes recommended
The new context JSON transport can emit noisy/empty canonical payloads (e.g., contextJson={} or nested empty objects) and can silently coerce undefined array elements to null in the Node SDK, which can fragment cache keys and alter semantics.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
packages/node-sdk/src/client.ts:1677
Object.keys(context).lengthcan be non-zero even whencanonicalJSONStringify(context)serializes to{}(e.g.,context: { user: undefined }oruser: { id: undefined }), which would sendcontextJson={}and create unnecessary request/cache-key variants. Consider only settingcontextJsonwhen the canonical JSON is not an empty object.
packages/node-sdk/src/utils.ts:172JSON.stringifyconvertsundefinedarray elements tonull. Since the Node SDKContexttypes allowany, callers can accidentally includeundefinedinside arrays and silently change semantics incontextJson. Consider failing fast by detectingundefinedarray elements during serialization.
- Files reviewed: 11/11 changed files
- Comments generated: 1
- Review effort level: Lite
| export function canonicalContextJSONStringify( | ||
| context: ReflagContext | undefined, | ||
| ): string | undefined { | ||
| if (!context) return undefined; | ||
| const nonEmptyContext = Object.fromEntries( | ||
| Object.entries(context).filter( | ||
| ([, attributes]) => | ||
| attributes && | ||
| Object.values(attributes).some((value) => value !== undefined), | ||
| ), | ||
| ); | ||
| return Object.keys(nonEmptyContext).length | ||
| ? canonicalJSONStringify(nonEmptyContext) | ||
| : undefined; | ||
| } |
Summary
contextJsonRollout
This is stacked on #711 for review. Do not release these SDK changes until
contextJsonsupport from reflagcom/web#3600 has been deployed. The evaluator from #711 should be released first, then consumed and deployed by web before this PR is merged/released.Verification
resolveSnapshotPathtimeout under concurrent load