feat: Allow waiting for the network response on identify. #548
Conversation
packages/shared/mocks/src/crypto.ts
Outdated
| @@ -1,12 +1,9 @@ | |||
| import type { Hasher } from '@common'; | |||
|
|
|||
| // eslint-disable-next-line import/no-mutable-exports | |||
There was a problem hiding this comment.
Forgot to remove this piece of global data in the test updates.
packages/shared/mocks/src/hasher.ts
Outdated
There was a problem hiding this comment.
This file was redundant.
| @@ -16,6 +16,10 @@ export const setupMockStreamingProcessor = ( | |||
| errorTimeoutSeconds: number = 0, | |||
| initTimeoutMs: number = 0, | |||
| ) => { | |||
| let initTimeoutHandle: any; | |||
There was a problem hiding this comment.
The simulated stream needs to be able to cancel its timers or it will run after tests are expected to end.
| describe('sdk-client object', () => { | ||
| let ldc: LDClientImpl; |
There was a problem hiding this comment.
Decreasing the scope of these to the fixture.
| } else { | ||
| this.logger.debug('Offline identify no storage. Defaults will be used.'); | ||
| this.logger.debug( | ||
| 'Offline identify - no cached flags, using defaults or already loaded flags.', |
There was a problem hiding this comment.
Small suggestion for clarity. I had to think a bit to realize what this was conveying.
| 'Offline identify - no cached flags, using defaults or already loaded flags.', | |
| 'Identify - Currently offline with no cached flags for provided context. Maintaining current flags. This may result in default flag evaluations if no flags have been loaded before.' |
Is the trailing comma intentional?
There was a problem hiding this comment.
Our linting requires training coma for parameters that are not on one line. The linter also forces this line to be wrapped as such.
Co-authored-by: Todd Anderson <[email protected]>
🤖 I have created a release *beep* *boop* --- <details><summary>js-client-sdk-common: 1.5.0</summary> ## [1.5.0](js-client-sdk-common-v1.4.0...js-client-sdk-common-v1.5.0) (2024-08-19) ### Features * Allow waiting for the network response on identify. ([#548](#548)) ([1375660](1375660)) </details> <details><summary>react-native-client-sdk: 10.5.1</summary> ## [10.5.1](react-native-client-sdk-v10.5.0...react-native-client-sdk-v10.5.1) (2024-08-19) ### Dependencies * The following workspace dependencies were updated * dependencies * @launchdarkly/js-client-sdk-common bumped from 1.4.0 to 1.5.0 </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
| @@ -8,4 +8,16 @@ export interface LDIdentifyOptions { | |||
| * Defaults to 5 seconds. | |||
| */ | |||
| timeout: number; | |||
There was a problem hiding this comment.
@kinyoklion timeout should be optional then
There was a problem hiding this comment.
Unfortunately timeout was already implemented as non-optional. That said it defaults to 5, so setting it to 5 is equal to not providing it.
There was a problem hiding this comment.
We will consider it.
There are advantages to having it non-optional and some SDKs require it with every call identify. Which makes it more obvious that the operation can timeout. We document it, but people are still often surprised that it times out.
Fixes: #503
Fixes: #505
When non-cached values are desired after identify the
waitForNetworkResultsidentify option can be used. This will caused the identify to wait for configuration from the configured data source or timeout.The recommended approach, for resilience and performance, will be to use cached flag values and respond to fresh values reactively and that will remain the default.