feat: Add identify timeout to client-sdk. #420

yusinto · 2024-04-03T02:13:57Z

No description provided.

shortcut-integration · 2024-04-03T02:14:00Z

This pull request has been linked to Shortcut Story #239001: Add identify timeout option to rn sdk.

packages/shared/sdk-client/src/LDClientImpl.ts

…ify its own value. Set rn default to 15s.

packages/shared/sdk-client/src/LDClientImpl.ts

packages/shared/sdk-client/src/api/LDIdentifyOptions.ts

kinyoklion · 2024-04-03T16:46:58Z

packages/shared/common/src/utils/timeout.ts

+    setTimeout(() => {
+      const e = `${taskName} timed out after ${t} seconds.`;
+      logger?.error(e);
+      reject(new Error(e));


I have concerns with rejecting the promise. Being as this condition will not normally encountered it may result in broken apps instead of safer apps.

(I know the node SDK does it, but I am not certain that is a good idea either.)

I don't have an issue with resolving. However the breaking side-effect is identify now returns Promise<void | Error> rather than just Promise<void>.
If we stay with rejecting, the identify promise should be caught by the consumer. This is an accepted safe practice. If we document this clearly I think the risk of breaking apps is low.

… timedPromise.

…IdentifyTimeout.

yusinto · 2024-04-03T19:41:58Z

packages/shared/sdk-client/src/LDClientImpl.ts

  context?: LDContext;
  diagnosticsManager?: internal.DiagnosticsManager;
  eventProcessor?: internal.EventProcessor;
+  identifyTimeout: number = 5;


Harcoded to 5s by default. This will be set in identify if provided.

yusinto · 2024-04-03T19:42:55Z

packages/shared/sdk-client/src/LDClientImpl.ts

+  async identify(pristineContext: LDContext, identifyOptions?: LDIdentifyOptions): Promise<void> {
+    if (identifyOptions?.timeout) {
+      this.identifyTimeout = identifyOptions.timeout;
+    }
+
+    if (this.identifyTimeout > this.highTimeoutThreshold) {
+      this.logger.warn('identify called with high timeout parameter.');
+    }


shout: This is the core change.

louis-launchdarkly

If I understand correctly, because JS support default parameter (so it is not a breaking signature change), and the identify method can already error for other reason, adding and defaulting the timeout doesn't count as a behavior breaking change.

If that is the case, this should be the fix and approved.

🤖 I have created a release *beep* *boop* --- <details><summary>akamai-edgeworker-sdk-common: 1.1.3</summary> ## [1.1.3](akamai-edgeworker-sdk-common-v1.1.2...akamai-edgeworker-sdk-common-v1.1.3) (2024-04-09) ### Dependencies * The following workspace dependencies were updated * dependencies * @launchdarkly/js-server-sdk-common bumped from ^2.2.2 to ^2.2.3 </details> <details><summary>akamai-server-base-sdk: 2.1.3</summary> ## [2.1.3](akamai-server-base-sdk-v2.1.2...akamai-server-base-sdk-v2.1.3) (2024-04-09) ### Dependencies * The following workspace dependencies were updated * dependencies * @launchdarkly/akamai-edgeworker-sdk-common bumped from ^1.1.2 to ^1.1.3 * @launchdarkly/js-server-sdk-common bumped from ^2.2.2 to ^2.2.3 </details> <details><summary>akamai-server-edgekv-sdk: 1.1.3</summary> ## [1.1.3](akamai-server-edgekv-sdk-v1.1.2...akamai-server-edgekv-sdk-v1.1.3) (2024-04-09) ### Dependencies * The following workspace dependencies were updated * dependencies * @launchdarkly/akamai-edgeworker-sdk-common bumped from ^1.1.2 to ^1.1.3 * @launchdarkly/js-server-sdk-common bumped from ^2.2.2 to ^2.2.3 </details> <details><summary>cloudflare-server-sdk: 2.5.0</summary> ## [2.5.0](cloudflare-server-sdk-v2.4.2...cloudflare-server-sdk-v2.5.0) (2024-04-09) ### Features * JSR support for Cloudflare SDK. ([#415](#415)) ([30866f3](30866f3)) ### Dependencies * The following workspace dependencies were updated * devDependencies * @launchdarkly/js-server-sdk-common-edge bumped from 2.2.2 to 2.2.3 </details> <details><summary>js-client-sdk-common: 1.1.0</summary> ## [1.1.0](js-client-sdk-common-v1.0.3...js-client-sdk-common-v1.1.0) (2024-04-09) ### Features * Add identify timeout to client-sdk. ([#420](#420)) ([5d73dfe](5d73dfe)) ### Bug Fixes * Identify incorrectly rejected in client-sdk ([#426](#426)) ([a019dd6](a019dd6)) ### Dependencies * The following workspace dependencies were updated * dependencies * @launchdarkly/js-sdk-common bumped from 2.3.1 to 2.4.0 </details> <details><summary>js-sdk-common: 2.4.0</summary> ## [2.4.0](js-sdk-common-v2.3.1...js-sdk-common-v2.4.0) (2024-04-09) ### Features * Add identify timeout to client-sdk. ([#420](#420)) ([5d73dfe](5d73dfe)) ### Bug Fixes * Identify incorrectly rejected in client-sdk ([#426](#426)) ([a019dd6](a019dd6)) </details> <details><summary>js-server-sdk-common: 2.2.3</summary> ## [2.2.3](js-server-sdk-common-v2.2.2...js-server-sdk-common-v2.2.3) (2024-04-09) ### Dependencies * The following workspace dependencies were updated * dependencies * @launchdarkly/js-sdk-common bumped from 2.3.1 to 2.4.0 </details> <details><summary>js-server-sdk-common-edge: 2.2.3</summary> ## [2.2.3](js-server-sdk-common-edge-v2.2.2...js-server-sdk-common-edge-v2.2.3) (2024-04-09) ### Dependencies * The following workspace dependencies were updated * dependencies * @launchdarkly/js-server-sdk-common bumped from 2.2.2 to 2.2.3 </details> <details><summary>node-server-sdk: 9.2.3</summary> ## [9.2.3](node-server-sdk-v9.2.2...node-server-sdk-v9.2.3) (2024-04-09) ### Dependencies * The following workspace dependencies were updated * dependencies * @launchdarkly/js-server-sdk-common bumped from 2.2.2 to 2.2.3 </details> <details><summary>node-server-sdk-dynamodb: 6.1.6</summary> ## [6.1.6](node-server-sdk-dynamodb-v6.1.5...node-server-sdk-dynamodb-v6.1.6) (2024-04-09) ### Dependencies * The following workspace dependencies were updated * devDependencies * @launchdarkly/node-server-sdk bumped from 9.2.2 to 9.2.3 * peerDependencies * @launchdarkly/node-server-sdk bumped from 9.2.1 to 9.2.3 </details> <details><summary>node-server-sdk-redis: 4.1.6</summary> ## [4.1.6](node-server-sdk-redis-v4.1.5...node-server-sdk-redis-v4.1.6) (2024-04-09) ### Dependencies * The following workspace dependencies were updated * devDependencies * @launchdarkly/node-server-sdk bumped from 9.2.2 to 9.2.3 * peerDependencies * @launchdarkly/node-server-sdk bumped from 9.2.1 to 9.2.3 </details> <details><summary>react-native-client-sdk: 10.1.0</summary> ## [10.1.0](react-native-client-sdk-v10.0.5...react-native-client-sdk-v10.1.0) (2024-04-09) ### Features * Add identify timeout to client-sdk. ([#420](#420)) ([5d73dfe](5d73dfe)) ### Dependencies * The following workspace dependencies were updated * dependencies * @launchdarkly/js-client-sdk-common bumped from 1.0.3 to 1.1.0 </details> <details><summary>vercel-server-sdk: 1.3.4</summary> ## [1.3.4](vercel-server-sdk-v1.3.3...vercel-server-sdk-v1.3.4) (2024-04-09) ### Dependencies * The following workspace dependencies were updated * dependencies * @launchdarkly/js-server-sdk-common-edge bumped from 2.2.2 to 2.2.3 </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> Co-authored-by: Yusinto Ngadiman <yusinto@gmail.com>

yusinto added 4 commits April 2, 2024 14:28

chore: Added identify options.

5ec18c0

chore: Added timeout function. Added comments.

f78ff8f

Update LDClientImpl.ts

c9e5fb1

chore: Added unit tests for timeout.

59f66a5

yusinto requested review from kinyoklion and louis-launchdarkly as code owners April 3, 2024 02:13

yusinto commented Apr 3, 2024

View reviewed changes

packages/shared/sdk-client/src/LDClientImpl.ts Show resolved Hide resolved

fix: Change default timeout to 5 seconds. Improved in-line comments.

31888fe

yusinto marked this pull request as draft April 3, 2024 05:39

yusinto added 2 commits April 3, 2024 00:00

chore: Add defaultIdentifyTimeout class variable so each sdk can spec…

59906a0

…ify its own value. Set rn default to 15s.

chore: Added more unit tests.

e47fdb2

yusinto marked this pull request as ready for review April 3, 2024 08:21

yusinto commented Apr 3, 2024

View reviewed changes

packages/shared/sdk-client/src/LDClientImpl.ts Outdated Show resolved Hide resolved

chore: Log timeout error. Added tests. Improve error listener arg type.

0c80446

yusinto commented Apr 3, 2024

View reviewed changes

packages/shared/sdk-client/src/LDClientImpl.ts Outdated Show resolved Hide resolved

chore: Improve error logging.

53d4845

yusinto enabled auto-merge (squash) April 3, 2024 10:18

yusinto commented Apr 3, 2024

View reviewed changes

packages/shared/sdk-client/src/api/LDIdentifyOptions.ts Outdated Show resolved Hide resolved

kinyoklion reviewed Apr 3, 2024

View reviewed changes

yusinto added 4 commits April 3, 2024 10:34

chore: Improve high timeout warning.

409f340

chore: Renamed timeoutSeconds to timeout. Renamed timeout function to…

d9c0469

… timedPromise.

chore: Fixed broken tests due to warning string.

381feee

chore: Minor comment improvement.

c45a867

yusinto requested a review from kinyoklion April 3, 2024 19:32

yusinto added 2 commits April 3, 2024 12:33

chore: Introduce customizable highTimeoutThreshold to replace default…

d64edae

…IdentifyTimeout.

chore: Fixed broken unit tests.

ebb5d86

yusinto commented Apr 3, 2024

View reviewed changes

louis-launchdarkly approved these changes Apr 3, 2024

View reviewed changes

yusinto merged commit 5d73dfe into main Apr 3, 2024

yusinto deleted the yus/sc-239001/add-identify-timeout-option-to-rn-sdk branch April 3, 2024 22:34

github-actions bot mentioned this pull request Apr 3, 2024

chore: release main #418

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat: Add identify timeout to client-sdk. #420

feat: Add identify timeout to client-sdk. #420

Uh oh!

yusinto commented Apr 3, 2024 •

edited

Loading

Uh oh!

shortcut-integration bot commented Apr 3, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kinyoklion Apr 3, 2024

Uh oh!

kinyoklion Apr 3, 2024

Uh oh!

yusinto Apr 3, 2024 •

edited

Loading

Uh oh!

yusinto Apr 3, 2024

Uh oh!

yusinto Apr 3, 2024

Uh oh!

louis-launchdarkly left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

feat: Add identify timeout to client-sdk. #420

feat: Add identify timeout to client-sdk. #420

Uh oh!

Conversation

yusinto commented Apr 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shortcut-integration bot commented Apr 3, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kinyoklion Apr 3, 2024

Choose a reason for hiding this comment

Uh oh!

kinyoklion Apr 3, 2024

Choose a reason for hiding this comment

Uh oh!

yusinto Apr 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yusinto Apr 3, 2024

Choose a reason for hiding this comment

Uh oh!

yusinto Apr 3, 2024

Choose a reason for hiding this comment

Uh oh!

louis-launchdarkly left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

yusinto commented Apr 3, 2024 •

edited

Loading

yusinto Apr 3, 2024 •

edited

Loading