chore: adding the implementation for main process client#1103
chore: adding the implementation for main process client#1103
Conversation
|
@launchdarkly/js-sdk-common size report |
|
@launchdarkly/js-client-sdk-common size report |
|
@launchdarkly/browser size report |
|
@launchdarkly/js-client-sdk size report |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| } | ||
| }); | ||
| return ret; | ||
| } |
There was a problem hiding this comment.
Duplicated bootstrap.ts from browser SDK package
Low Severity
The readFlagsFromBootstrap function is an exact character-for-character copy of packages/sdk/browser/src/bootstrap.ts. This duplication means any future bug fix or improvement to bootstrap parsing needs to be applied in both places. This function could be extracted into @launchdarkly/js-client-sdk-common to avoid the duplication.
There was a problem hiding this comment.
tracking as sdk-1874
| () => ({ | ||
| pathGet(encoding: Encoding, _plainContextString: string): string { | ||
| return useClientSideId | ||
| ? `/sdk/evalx/${credential}/contexts/${base64UrlEncode(_plainContextString, encoding)}` |
There was a problem hiding this comment.
this is where we are letting developers define whether they are using client side sdk or the default mobile sdk. I have a task open to clarify that secure mode is only available if you are using client side id (SDK-1875)
There was a problem hiding this comment.
I wonder if we could hoist the paths up to a higher packages and then import the correct ones. It should be tree shake-able.
I like having a file with like "this is all the mobile stuff" then using that for RN and electron. Using the client-side id stuff for browser and electron.
Similar to the flutter pattern.
https://github.com/launchdarkly/flutter-client-sdk/blob/fa48b8e2b833b51c4201b4803701a7cc8ec3efec/packages/common_client/lib/src/config/defaults/io_config.dart#L6
| * Builds the LaunchDarkly client facade (PIMPL). Exposes a single identify method that returns | ||
| * identify results. Plugins are registered with the facade. | ||
| */ | ||
| export function makeClient( |
There was a problem hiding this comment.
this file should be very similar to the browser client sdk
| this.setEventSendingEnabled(!this.isOffline(), false); | ||
|
|
||
| if (validatedElectronOptions.enableIPC) { | ||
| // Not implemented yet |
There was a problem hiding this comment.
Separated this out to the next PR since we will probably have the most concerns there
| } catch (error) { | ||
| this.logger.error('Failed to bootstrap data', error); | ||
| } | ||
| } |
There was a problem hiding this comment.
Redundant bootstrap processing in start() method
Low Severity
The bootstrap processing block in start() (parsing via readFlagsFromBootstrap and calling presetFlags) is entirely redundant. start() immediately calls this.identifyResult() on line 195, which contains an identical bootstrap processing block that re-parses (if needed) and calls presetFlags again with the same data. Since both are synchronous and execute sequentially, the block in start() provides no timing benefit and just duplicates logic that identifyResult() already handles.
Additional Locations (1)
There was a problem hiding this comment.
I assume this is a result of the synchronous availability changes. We may just want some comments to help human and robot alike.
| getConnectionMode(): ConnectionMode { | ||
| return this.connectionMode; | ||
| } | ||
| } |
There was a problem hiding this comment.
ElectronDataManager duplicates MobileDataManager almost entirely
Low Severity
ElectronDataManager is a near-verbatim copy of MobileDataManager from the React Native SDK. The _setupConnection, setConnectionMode, getConnectionMode, setNetworkAvailability, _debugLog methods and the core identify flow (cache loading, offline handling, connection setup) are identical. Only bootstrap handling is Electron-specific. This risks inconsistent bug fixes across the two data managers.
| const logTag = '[ElectronDataManager]'; | ||
|
|
||
| export default class ElectronDataManager extends BaseDataManager { | ||
| // Not implemented yet. |
There was a problem hiding this comment.
Should we attach a ticket to this? (Though we may also choose not to implement this, which maybe would be a little different comment).
There was a problem hiding this comment.
I agree with the bot analysis that this likely could be a shared configuration of the MobileDataManager.
That said, with the client-side FDv2 work, I am going to see if we can change this pattern to reduce the complexity of customizing the data manager.
Maybe some more composable approach that allows configuring conditions in different ways. So I wouldn't worry about it too much right now.
| // NOTE: we can choose to validate events with a whitelist? However, this might be | ||
| // more for the implementers to do. | ||
|
|
||
| // TODO: we will need to refactor the verbiage of client side id to mobile key in the future. |
2 participants
This PR will:
Note
Medium Risk
Touches core SDK initialization/identify and connection-management paths, which can impact flag delivery and event sending; mitigated by substantial new unit tests covering bootstrap, modes, and credential behaviors.
Overview
Adds a new Electron main-process LaunchDarkly client (
ElectronClient+createClient) that requiresstart()before use, supportsbootstrapflag seeding, and exposes connection mode controls (offline/polling/streaming) with appropriate event/processor behavior.Introduces Electron-specific option validation (
enableIPC,initialConnectionMode,useClientSideId, plugins) and a newElectronDataManagerthat handles bootstrap+cache identify flow and switches between polling/streaming processors.Adds extensive unit coverage for main-process usage without IPC, credential-type URL/header behavior (mobile key vs
useClientSideId), plugin hook registration/execution, bootstrap parsing, and option validation, plus temporary migration/IPC architecture documentation.Written by Cursor Bugbot for commit 50b7675. This will update automatically on new commits. Configure here.