feat: adding start() method to common client sdk package#1244
feat: adding start() method to common client sdk package#1244
Conversation
|
@launchdarkly/js-sdk-common size report |
|
@launchdarkly/browser size report |
joker23
force-pushed
the
skz/sdk-1759/client-start
branch
from
1a440b9 to
d249ad8
Compare
|
@launchdarkly/js-client-sdk size report |
|
@launchdarkly/js-client-sdk-common size report |
|
@cursor 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 d249ad8. Configure here.
joker23
force-pushed
the
skz/sdk-1759/client-start
branch
from
d249ad8 to
8f1e564
Compare
There was a problem hiding this comment.
mostly moving all of the start tests to common
| * When true, the SDK requires `start()` to be called before `identify()`. | ||
| * Set this value to `true` to use the new initialization pattern. | ||
| */ | ||
| requiresStart?: boolean; |
There was a problem hiding this comment.
added this to compat react native... this was the only way I found that would leave RN alone
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
joker23
force-pushed
the
skz/sdk-1759/client-start
branch
from
8f1e564 to
111c7e2
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
I don't know that I like changes to this file simultaneous to the change. From a validation perspective I would prefer this be unchanged. And then potentially these be de-duplicated. That said I am not certain they really should be de-duplicated.
| const noTimeout = identifyOptions?.timeout === undefined && identifyOptions?.noTimeout === true; | ||
| if (this._requiresStart && !this.startPromise) { | ||
| this.logger.error( | ||
| 'Client must be started before it can identify a context, did you forget to call start()?', |
There was a problem hiding this comment.
I would prefer something that didn't contain 'forget'.
| 'Client must be started before it can identify a context, did you forget to call start()?', | |
| 'The client must be started before a context can be identified. Call start() prior to identifying a context.', |
Or maybe something like:
The client has not been started. Please call start() before identifying subsequent contexts.
There was a problem hiding this comment.
Though something simpler than "subsequent" might be better.
I'm basically after 2 things:
- Don't assume/imply they knew the correct operation.
- If possible be clear that identify isn't for the first context. Only when changing contexts is required.
2 participants
This PR will consolidate the
start()method into the shared client sdk package.To do this we also:
requiresStartwhich should betruefor new SDKs (and only RN has it defaultfalse)requiresStartis, effectively, a feature flag for whether the SDK is using the newcreate+startinitialization patternstartmethod into client commonstartmethod out of browser and electron sdksNote
Medium Risk
Centralizes client initialization and adds a new
requiresStartgate that changes whenidentifyis allowed, affecting startup/bootstrapping behavior across browser and electron SDKs. Moderate risk due to changes in core init/identify flow and promise/timeout handling, though covered by new shared unit tests.Overview
Consolidates
start()into the sharedLDClientImplso browser/electron no longer maintain their own initialization logic;start()is now idempotent, caches its promise/result, and supports bootstrap flag presetting (including reason data) from either top-levelbootstraporidentifyOptions.bootstrap.Adds an internal
requiresStartoption (enabled for browser and electron) that blocksidentify()calls beforestart()and, when enabled, defaults post-startidentifycalls tosheddable: trueunless explicitly set.Updates browser/electron SDKs to wire
requiresStart, usesetInitialContext, and trims/adjusts SDK-specific tests; a new shared test suite (LDClientImpl.start.test.ts) coversstart()/bootstrap behavior, therequiresStartguard, andwaitForInitializationintegration.Reviewed by Cursor Bugbot for commit 66673ed. Bugbot is set up for automated code reviews on this repo. Configure here.