feat: Thread FDv1 fallback TTL through polling and streaming data sources#1806
feat: Thread FDv1 fallback TTL through polling and streaming data sources#1806joker23 wants to merge 9 commits into
Conversation
|
@launchdarkly/js-sdk-common size report |
|
@launchdarkly/js-client-sdk-common size report |
|
@launchdarkly/js-client-sdk size report |
|
@launchdarkly/browser size report |
|
@cursor review |
…rces This commit will add in fallback TTL reading support as well as allow RN native event source read headers.
e4fa9cb to
1400310
Compare
|
@cursor review |
| * 1. separated event handling to use onprogress for data changes and | ||
| * onreadystatechange for status changes, since some platforms (e.g. Vega OS) | ||
| * never fire a readystatechange event while the response is still streaming in. | ||
| * 2. onprogress now also performs the CONNECTING -> OPEN transition and dispatches |
There was a problem hiding this comment.
Would like to have some thoughts here. This looks like a bug in our previous implementation, but would like to understand if there are any legacy reasons for this.
| package_name: '@launchdarkly/js-client-sdk-common' | ||
| pr_number: ${{ github.event.number }} | ||
| size_limit: 39000 | ||
| size_limit: 40000 |
There was a problem hiding this comment.
FYI - needed to bump this up to accomondate the TTL logic :(
46de8d4 to
6604149
Compare
|
@cursor review |
|
@cursor review |
29aefb7 to
73eeb30
Compare
|
@cursor review |
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit c01baaa. Configure here.
| terminalError, | ||
| } from './FDv2SourceResult'; | ||
|
|
||
| function getFallback(headers: { get(name: string): string | null }): boolean { |
There was a problem hiding this comment.
replacing with
| fdv1FallbackTtlMs = pendingFallbackTtlMs; | ||
| resultQueue.put(changeSet(action.payload, true, undefined, undefined, pendingFallbackTtlMs)); | ||
| pendingFallback = false; | ||
| pendingFallbackTtlMs = undefined; |
There was a problem hiding this comment.
Should this section use resolveFallback?
| case 'payload': | ||
| logConnectionResult(true); | ||
| resultQueue.put(changeSet(action.payload, fdv1Fallback)); | ||
| if (pendingFallback) { |
There was a problem hiding this comment.
Is this if check required? Is there a way to write it such that it just uses resolveFallback always?
| fdv1Fallback = true; | ||
| resultQueue.put(terminalError(errorInfoFromUnknown(action.reason), true, ttlMs)); | ||
| pendingFallback = false; | ||
| pendingFallbackTtlMs = undefined; |
There was a problem hiding this comment.
Same comment here. I thought the purpose of the resolveFallback was to remove duplication of this routine of consuming the pending fallback and clearing values.
| const fallback = resolveFallback(); | ||
| if (!result.fdv1Fallback && fallback.fdv1Fallback) { | ||
| result.fdv1Fallback = true; | ||
| result.fdv1FallbackTtlMs = fallback.fdv1FallbackTtlMs; |
There was a problem hiding this comment.
Is editing result appropriate? Seems like it shouldn't be mutated from a good practices standpoint.
There was a problem hiding this comment.
Something feels off about how many spots have if checks on pending fallback and logic for moving things around and masking values on top of others. It just seems very scattered and likely a subtle tweak will mess up the logic. Is there a way to make it such that all results that go into the queue conform to some type / interface and fallback can be applied to that thing unconditonally, sometimes a no-op but sometimes a real fallback apply occurs. The pending mechanism could be something that can only apply once and then turns itself into a no-op or something.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 1622eff. Configure here.
| // Convert final interrupted -> terminal_error | ||
| const status = lastResult as StatusResult; | ||
| return terminalError(status.errorInfo!, status.fdv1Fallback); | ||
| return terminalError(status.errorInfo!, { fdv1Fallback: status.fdv1Fallback }); |
There was a problem hiding this comment.
Polling initializer drops fallback TTL
Medium Severity
After retryable polling failures are exhausted, the initializer converts the last interrupted status into terminalError using only fdv1Fallback, so any fdv1FallbackTtlMs that poll() already attached from response headers is dropped on that path.
Reviewed by Cursor Bugbot for commit 1622eff. Configure here.
This will allow us to resolve directives that should override the fallback behavior such as goodbye or error
| "packages/sdk/shopify-oxygen/example", | ||
| "packages/sdk/browser/example", | ||
| "packages/sdk/browser/example-fdv2", | ||
| "packages/sdk/browser/example-fdv1-fallback", |
There was a problem hiding this comment.
🟡 Project setup references a package folder that does not exist
A new project folder is listed as part of the workspace (package.json:68) but the folder was never added to the repository, so the setup step points at something that isn't there.
Impact: Developers running the standard install/build for the whole repository may hit warnings or a failed setup because the referenced package folder is missing.
Dangling Yarn workspace entry
The entry "packages/sdk/browser/example-fdv1-fallback" was added to the workspaces array in package.json:68, but packages/sdk/browser/example-fdv1-fallback does not exist on disk and is not tracked by git (verified via git ls-files and by comparing against the base SHA). Every other workspace entry (e.g. packages/sdk/browser/example-fdv2) points to an existing directory with a package.json. The root check script runs yarn, and this dangling reference is at best dead configuration and at worst a source of install/build noise or errors depending on the Yarn resolution behavior. Either the example package needs to be committed as part of this PR, or the workspace entry should be removed.
Prompt for agents
The workspaces array in package.json now includes "packages/sdk/browser/example-fdv1-fallback" but that directory does not exist in the repository (not tracked by git, not present on disk). This is a dangling workspace reference introduced by this PR. Either commit the example-fdv1-fallback package (with its own package.json) so the workspace entry resolves, or remove the entry from the workspaces array in package.json.
Was this helpful? React with 👍 or 👎 to provide feedback.


This commit will add in fallback TTL reading support as well as allow RN native event source read headers.
NOTE: while working this PR, I found a lot of gaps in the RN eventsource. Internally tracked in ticket 2721 in sdk project.
Note
Medium Risk
Changes core FDv2 data-source result shaping and when the orchestrator sees fallback vs goodbye, so incorrect TTL or directive clearing could affect FDv1/FDv2 switching timing.
Overview
Adds
fdv1FallbackTtlMsend-to-end for FDv2 polling and streaming by centralizingx-ld-fd-fallback/x-ld-fd-fallback-ttl(and in-bandprotocolFallbackTTLon goodbye) in a sharedFallbackDirectivetype, and threading that object throughFDv2SourceResulthelpers instead of a bare boolean.Polling now stamps TTL on success and error responses and treats a goodbye combined with a fallback directive as
terminal_errorso the orchestrator switches off FDv2 instead of treating it as a benign disconnect.Streaming defers open-time fallback headers until the next queued result, merges deferred TTL into ping-poll results without mutating handler objects, prefers in-band goodbye TTL over deferred open TTL, and resets pending/committed fallback state on clean reconnect and
close(). CI package size limit for@launchdarkly/js-client-sdk-commonrises 39k → 41.5k; root workspaces addpackages/sdk/browser/example-fdv1-fallback.Reviewed by Cursor Bugbot for commit e95c2f2. Bugbot is set up for automated code reviews on this repo. Configure here.