chore: RN provider and hooks attempt 2 #321

yusinto · 2023-11-30T04:10:43Z

This is a second attempt of #306 using the newly added event source.

use StreamingProcessor and event source to fetch flags
added LDProvider
added hooks
added typed variation[Detail] functions

Please read the example README and try running the example app.

…ent in event source. TODO: fix malformed es uri. fix broken typedvariation eval.

…aths. moved base64 function to common.

…fix ldclient tests.

…ed confusing failed EventName.

…ext after a successful put. emit events for change and delete. fixed a bug where delete handler is listening to patch.

shortcut-integration · 2023-11-30T04:10:47Z

This pull request has been linked to Shortcut Story #221870: Implement RN provider api.

yusinto · 2023-11-30T04:11:56Z

packages/sdk/react-native/src/hooks/variation/useTypedVariation.ts

+  const unsupportedType = useMemo(
+    () => ({
+      value: defaultValue,
+      variationIndex: null,
+      reason: { kind: 'ERROR', errorKind: 'UNSUPPORTED_VARIATION_TYPE' },
+    }),
+    [defaultValue],
+  );


This is my proposed fix for this comment in #306 .

Yeah, I think this will do it. 👍

yusinto · 2023-11-30T04:13:20Z

packages/sdk/react-native/package.json

@@ -40,7 +40,7 @@
    "ios": "yarn ./example && yarn build && yarn ./example ios"
  },
  "peerDependencies": {
-    "react-native": "*"
+    "react": "*"


We need react for useMemo.

yusinto · 2023-11-30T04:15:38Z

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

I only alphabetically re-ordered the members of this interface, no change in functionality here.

yusinto · 2023-11-30T04:16:21Z

packages/shared/sdk-client/src/evaluation/fetchUtils.ts

-/**
- * In react-native use base64-js to polyfill btoa. This is safe
- * because the react-native repo uses it too. Set the global.btoa to the encode
- * function of base64-js.
- * https://github.com/beatgammit/base64-js
- * https://github.com/axios/axios/issues/2235#issuecomment-512204616
- *
- * Ripped from https://thewoods.blog/base64url/
- */
-export const base64UrlEncode = (s: string, encoding: Encoding): string =>
-  encoding.btoa(s).replace(/\+/g, '-').replace(/\//g, '_').replace(/=+$/, '');
-


Moved to common so this can be shared.

yusinto · 2023-11-30T04:29:26Z

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


  /**
   * Creates the client object synchronously. No async, no network calls.
   */
  constructor(
    public readonly sdkKey: string,
-    public context: LDContext,


I'm decoupling the construction of LDClient instance and identify. The idea is to allow an LDClient object to be constructed, and at a later time call identify to fetch flags. We seem to be getting many requests for this and for the rn sdk, this seems to work ok.

yusinto · 2023-11-30T04:30:29Z

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

-    } catch (error: any) {
-      this.emitter.emit('failed', error);
-      throw error;
-    }
  }


This seems redundant because identify does the same work. It is a breaking change so I am calling it out here.

It seems the failed functionality was not replaced.

I think the fundamental flow is also different.

So a ready/failed event was a singular activity. The SDK wouldn't emit ready more than once. You people put code there that they want to execute once the SDK is ready to go. Or when they know it will never be ready, failed.

yusinto · 2023-11-30T04:59:11Z

packages/sdk/react-native/src/ReactNativeLDClient.test.ts

These are just basic tests for now. I will add more soon in a separate PR.

kinyoklion

Your tests seem like maybe they have an open handles problem and are failing.

It would be nice to have some amount of documentation on each of the hooks. To provide some intellisense so developers no what to expect.

I am continuing to look through other parts, but looks reasonable so far.

yusinto · 2023-11-30T18:41:07Z

tests seem like maybe they have an open handles

Yep. The issue was EventProcessor was actually trying to send events. I have set sendEvents: false for unit tests to fix this. Thank you for checking.

kinyoklion · 2023-11-30T18:41:41Z

packages/sdk/react-native/src/hooks/variation/useTypedVariation.ts

+  const ldClient = useLDClient();
+  const { status } = useLDDataSourceStatus();
+
+  if (status === 'ready') {


Is error a terminal state? This is concerning.

Typically we always want the client to be used, we generate unknown flag events. If it goes from good to bad we also have cached events.

Or maybe the status needs explained. My reflex is this is problemati.

You are right. I did this originally to avoid valuation errors caused by a null/undefined ldClient. This was early in the iteration when using polling. The Provider now requires an ldClient instance so that means we can fallback to the ldClient by default. I have made this change now.

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

Co-authored-by: Ryan Lamb <[email protected]>

yusinto · 2023-11-30T22:31:34Z

documentation on each of the hooks

Good idea. These hooks call the ldClient variation methods, which are fully documented there. I think we should link them somehow so we only maintain 1 copy? I'm not sure how to link them though.

kinyoklion · 2023-11-30T22:45:01Z

documentation on each of the hooks

Good idea. These hooks call the ldClient variation methods, which are fully documented there. I think we should link them somehow so we only maintain 1 copy? I'm not sure how to link them though.

Well, to make actual links you can do {@link LDClient.variation} for example. It isn't ideal because it will literally link you to the item.

I am not sure, cross package, if there is a way to insert the docs inline. With inheritance you can inherit them, but doesn't help for hooks.

yusinto · 2023-11-30T23:08:20Z

documentation on each of the hooks

Ok I duplicated the comments and also added comments for the base hooks. There were mistakes in the LDClient docs as well and I corrected those too.

yusinto and others added 16 commits November 27, 2023 23:01

chore: initial commit.

cbfd5b1

chore: added npm commands to run natively.

da01855

chore: fixed wrong listener argument shape. fixed missing error argum…

fc41787

…ent in event source. TODO: fix malformed es uri. fix broken typedvariation eval.

Delete index.test.tsx

7eb9de5

Update index.ts

7813408

fix: erroneous stream uri path

c0e4577

fix: emit ready when put is finished.

06f08a9

fix: override createStreamUriPath so client sdks can specify custom p…

afd4680

…aths. moved base64 function to common.

fix: createStreamUriPath returns string now.

9ff4930

fix: fix broken tests related to fromLDContext arg being undefined.

ced9caa

chore: allow mock streamprocessor to return custom json. starting to …

51aef40

…fix ldclient tests.

chore: return a promise from identify and fix unit tests.

f8f6c23

chore: added newline.

dea6e45

chore: removed unused listeners. revert eventSource to private. remov…

efc36ee

…ed confusing failed EventName.

chore: removed variation:error and variation:success. only set ldcont…

900f654

…ext after a successful put. emit events for change and delete. fixed a bug where delete handler is listening to patch.

chore: improve readme.

9d458f2

yusinto requested review from kinyoklion and louis-launchdarkly as code owners November 30, 2023 04:10

yusinto commented Nov 30, 2023

View reviewed changes

yusinto added 2 commits November 29, 2023 20:49

chore: refactored promise creation to a separate function.

66499d0

chore: added basic tests.

cbd13cc

yusinto commented Nov 30, 2023

View reviewed changes

Update welcome.tsx

cfba59b

kinyoklion requested changes Nov 30, 2023

View reviewed changes

fix: disable events for unit tests.

0c70c03

kinyoklion reviewed Nov 30, 2023

View reviewed changes

fix: variation calls to fallback to ldClient.

5c76d3f

kinyoklion requested changes Nov 30, 2023

View reviewed changes

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

yusinto and others added 2 commits November 30, 2023 13:10

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

9838b07

Co-authored-by: Ryan Lamb <[email protected]>

fix: close streamer first before starting. test listener count.

335d3c9

chore: added tsdoc for hooks. corrected LDClient variation tsdoc errors.

a419b86

kinyoklion approved these changes Nov 30, 2023

View reviewed changes

yusinto merged commit a343c83 into main Nov 30, 2023
15 checks passed

yusinto deleted the yus/sc-221870/rn-provider-hooks branch November 30, 2023 23:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: RN provider and hooks attempt 2 #321

chore: RN provider and hooks attempt 2 #321

yusinto commented Nov 30, 2023

shortcut-integration bot commented Nov 30, 2023

yusinto Nov 30, 2023

kinyoklion Nov 30, 2023

yusinto Nov 30, 2023

yusinto Nov 30, 2023

yusinto Nov 30, 2023

yusinto Nov 30, 2023

yusinto Nov 30, 2023

kinyoklion Nov 30, 2023

yusinto Nov 30, 2023

kinyoklion left a comment

yusinto commented Nov 30, 2023

kinyoklion Nov 30, 2023

yusinto Nov 30, 2023

yusinto commented Nov 30, 2023

kinyoklion commented Nov 30, 2023

yusinto commented Nov 30, 2023

chore: RN provider and hooks attempt 2 #321

chore: RN provider and hooks attempt 2 #321

Conversation

yusinto commented Nov 30, 2023

shortcut-integration bot commented Nov 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kinyoklion left a comment

Choose a reason for hiding this comment

yusinto commented Nov 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yusinto commented Nov 30, 2023

kinyoklion commented Nov 30, 2023

yusinto commented Nov 30, 2023