feat: common identify now reports identify status#1289
Conversation
|
@launchdarkly/js-sdk-common size report |
|
@launchdarkly/browser size report |
|
@launchdarkly/js-client-sdk size report |
|
@launchdarkly/js-client-sdk-common size report |
joker23
changed the title
| override async identify( | ||
| context: LDContext, | ||
| identifyOptions?: LDIdentifyOptions, | ||
| ): Promise<LDIdentifyResult> { |
There was a problem hiding this comment.
I think should be okay because I don't think anyone is checking for void returns?
There was a problem hiding this comment.
It doesn't make me particularly comfortable to actually be returning something.
There was a problem hiding this comment.
yea, I don't think we could really do this step comfortably until we major version RN... which is fine... I'll move this back to draft or close, otherwise the changes seem fairly trivial.
|
@cursor review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This reverts commit 7d7d036.
|
@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 5e0b4cb. Configure here.
|
@cursor review |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
@cursor review |
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 b88b805. Configure here.
| this.logger.error(timeoutError.message); | ||
| throw timeoutError; | ||
| } | ||
| return result; |
There was a problem hiding this comment.
RN identify rejects despite interface promising no rejection
Medium Severity
ReactNativeLDClient.identify() throws on error and timeout statuses, but the base LDClient interface now explicitly documents that "The promise returned from this method will not be rejected." The RN constructor passes this directly to internal.safeRegisterPlugins, so plugins receive a client whose identify can reject — violating the contract they rely on. A plugin calling identify without a try-catch (reasonable given the new contract) risks unhandled promise rejections in the React Native SDK specifically.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit b88b805. Configure here.
There was a problem hiding this comment.
This is intentional
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
2 participants
This PR will consolidate the identify implementation in the sdk common level.
sdk-2078
sdk-2081
sdk-2082
The change here is that identify will always return a promise that resolves to a result. This pattern allows for better identify status tracking.
A few caveats:
Note
Medium Risk
Changes the public
identify()contract across the shared client and browser/electron SDKs from throwing/void to returningLDIdentifyResult, requiring downstream callers to handle{status}outcomes; behavior differences (timeouts/errors no longer reject) could break assumptions.Overview
Unifies
identify()to always return anLDIdentifyResult(completed/error/timeout/shed) and guarantees the Promise does not reject, removing the separateidentifyResult()API and updating the sharedLDClienttype accordingly.Browser and Electron clients are updated to call/forward the new
identify()(including IPC handlers and PIMPL facades) while still defaultingsheddable: truein their overrides; related tests are rewritten to assert returned status objects instead of promise rejections.React Native preserves backward compatibility by exporting an RN-specific
LDClienttype withidentify(): Promise<void>semantics and overridingReactNativeLDClient.identify()to throw on error/timeout (and explicitly disallowingstart()).Reviewed by Cursor Bugbot for commit b88b805. Bugbot is set up for automated code reviews on this repo. Configure here.