feat: Add experimental FDv2 support for React Native.#1243
feat: Add experimental FDv2 support for React Native.#1243kinyoklion wants to merge 6 commits intomainfrom
Conversation
kinyoklion
force-pushed
the
rlamb/react-native-fdv2
branch
from
f44eae2 to
6f8f006
Compare
|
@launchdarkly/js-sdk-common size report |
|
@launchdarkly/browser size report |
|
@launchdarkly/js-client-sdk size report |
|
@launchdarkly/js-client-sdk-common size report |
| * https://launchdarkly.com/docs/sdk/features/data-saving-mode | ||
| */ | ||
| // eslint-disable-next-line @typescript-eslint/no-empty-interface | ||
| export interface RNDataSystemOptions extends LDClientDataSystemOptions {} |
There was a problem hiding this comment.
This may not be strictly required. But if we decide to add RN specific options, then we will have a type to do it in. Where if we use LDClientDataSystemOptions, then we have to do name shenanigans. Or risk breaking things.
The public API shows setConnectionMode(mode: ConnectionMode) and getConnectionMode(): ConnectionMode. The FDv2-specific overloads accepting FDv2ConnectionMode and undefined are stripped from declarations via @internal + stripInternal.
|
bugbot review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
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 21b3f73. Configure here.
kinyoklion
changed the title
|
|
||
| // Wire state detection directly to FDv2 data manager. | ||
| const stateDetector = new RNStateDetector(); | ||
| this._stateDetector = stateDetector; |
There was a problem hiding this comment.
I don't see another reference to this._stateDetector could we get rid of this?
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Add protected readonly isFDv2 to LDClientImpl, derived from the validated configuration.dataSystem rather than raw user options. This prevents a mismatch when validation rejects the dataSystem value but the flag still indicates FDv2 is active.
| private _eventSendingEnabled: boolean = false; | ||
| private _baseHeaders: LDHeaders; | ||
| protected dataManager: DataManager; | ||
| protected readonly isFDv2: boolean; |
There was a problem hiding this comment.
This is so we don't have to infer from other signal. We just know.
| this.emitter, | ||
| this._diagnosticsManager, | ||
| ); | ||
| this.isFDv2 = !!this._config.dataSystem; |
There was a problem hiding this comment.
Validated data system config, not the input options.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes 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 cabb3f1. Configure here.
| state === NetworkState.Available ? 'available' : 'unavailable', | ||
| ); | ||
| }); | ||
| } |
There was a problem hiding this comment.
FDv2 stateDetector is unreachable, preventing cleanup on close
Medium Severity
The stateDetector in the FDv2 path is a local variable with no stored reference. RNStateDetector's constructor registers a permanent AppState.addEventListener callback, keeping it alive indefinitely. After close() is called on the client, the stateDetector continues firing. When the app transitions to background, setLifecycleState('background') is called on the closed data manager, which invokes flushCallback?.() (set on line 143 and not cleared by the data manager's close()), triggering this.flush() on the closed client. Unlike the FDv1 path where _connectionManager is stored as an instance field allowing potential cleanup, the FDv2 stateDetector is completely unreachable after construction.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit cabb3f1. Configure here.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| if (validatedRnOptions.automaticBackgroundHandling) { | ||
| stateDetector.setApplicationStateListener((state) => { | ||
| fdv2DataManager.setLifecycleState( | ||
| state === ApplicationState.Foreground ? 'foreground' : 'background', | ||
| ); | ||
| }); | ||
| } | ||
|
|
||
| if (validatedRnOptions.automaticNetworkHandling) { | ||
| stateDetector.setNetworkStateListener((state) => { | ||
| fdv2DataManager.setNetworkState( | ||
| state === NetworkState.Available ? 'available' : 'unavailable', | ||
| ); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🟡 FDv2 path ignores dataSystem.automaticModeSwitching config for lifecycle/network listener wiring
The FDv2 constructor path uses legacy RN options (validatedRnOptions.automaticBackgroundHandling / validatedRnOptions.automaticNetworkHandling) to decide whether to register lifecycle and network state listeners, instead of consulting the FDv2-specific dataSystem.automaticModeSwitching config. The AutomaticModeSwitchingConfig type (packages/shared/sdk-client/src/api/datasource/LDClientDataSystemOptions.ts:62) provides explicit lifecycle and network boolean fields, and the docs state that false means "disable all automatic switching." However, none of these are checked. A user who sets dataSystem: { automaticModeSwitching: false } (or { type: 'automatic', lifecycle: false }) still gets automatic lifecycle-based mode switching because the legacy automaticBackgroundHandling defaults to true. The setLifecycleState calls flow through the debounce manager and transition table (MOBILE_TRANSITION_TABLE at packages/shared/sdk-client/src/datasource/ModeResolver.ts:46-50), causing the data source to switch to background mode despite the user explicitly requesting no automatic switching.
Prompt for agents
In ReactNativeLDClient.ts, the FDv2 constructor path (lines 148-162) should read the validated dataSystem.automaticModeSwitching config to decide whether to wire lifecycle and network listeners, rather than using the legacy FDv1 options (validatedRnOptions.automaticBackgroundHandling / automaticNetworkHandling).
The automaticModeSwitching config can be:
- true (boolean): enable all automatic switching
- false (boolean): disable all automatic switching
- { type: 'automatic', lifecycle?: boolean, network?: boolean }: granular control
- { type: 'manual', initialConnectionMode: ... }: disable all automatic switching
The config is available on the validated Configuration object (this._config.dataSystem.automaticModeSwitching), but _config is private to LDClientImpl. You could either:
1. Access it via the options object which is still in scope (options.dataSystem?.automaticModeSwitching), though this would be unvalidated.
2. Expose a method or property on LDClientImpl to access the relevant dataSystem config.
3. Read automaticModeSwitching from the configuration passed to the dataManagerFactory closure (which has access to configuration.dataSystem).
When automaticModeSwitching is false or ManualModeSwitching, neither listener should be wired. When it is AutomaticModeSwitchingConfig, use its lifecycle and network fields (defaulting to true for mobile per MOBILE_DATA_SYSTEM_DEFAULTS). When it is true (the default), wire both listeners.
Was this helpful? React with 👍 or 👎 to provide feedback.
2 participants
Requirements
Related issues
Provide links to any issues in this repository or elsewhere relating to this pull request.
Describe the solution you've provided
Provide a clear and concise description of what you expect to happen.
Describe alternatives you've considered
Provide a clear and concise description of any alternative solutions or features you've considered.
Additional context
Add any other context about the pull request here.
Note
Medium Risk
Introduces an alternate FDv2 data path and new connection-mode semantics in
ReactNativeLDClient, which can affect flag delivery and lifecycle/network handling behavior even though it is gated behind the newdataSystemoption.Overview
Adds an experimental
dataSystemconfiguration option (RNDataSystemOptions) to enable FDv2 flag delivery in the React Native SDK, and exports the new type from the public entrypoint.Updates
ReactNativeLDClientto instantiate either the existing FDv1MobileDataManageror an FDv2 data manager (viacreateFDv2DataManagerBase), wiring app/network state directly into FDv2 and extendingsetConnectionMode/getConnectionModeto support FDv2-only modes ('one-shot','background') with validation and warnings.Updates the shared
LDClientImplto expose anisFDv2flag (derived fromconfig.dataSystem) so platform clients can branch behavior based on the selected data system.Reviewed by Cursor Bugbot for commit cabb3f1. Bugbot is set up for automated code reviews on this repo. Configure here.