chore: implement initial flag fetch #294

yusinto · 2023-10-05T23:30:33Z

First attempt to implement an initial flag fetch followed by emitting events. I also added comments like this:

Dom api usage: xxx

There are three right now: fetch, btoa and EventTarget. I left comments in the code for react native how to deal with these.

… removing only one listener. Emit events on ready and failed. Better comments on dom api usage.

shortcut-integration · 2023-10-05T23:30:37Z

This pull request has been linked to Shortcut Story #219625: Implement initial flag fetch.

yusinto · 2023-10-05T23:47:58Z

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

+  async start() {
+    try {
+      this.flags = await fetchFlags(this.sdkKey, this.context, this.config, this.platform);
+      this.emitter.emit('ready');
+    } catch (error: any) {
+      this.logger.error(error);
+      this.emitter.emit('error', error);
+      this.emitter.emit('failed', error);
+    }


This is new. This decouples the construction of the ldClient object and the initial poll. The decoupling gives consuming sdks more flexibility how they want initialization to proceed.

yusinto · 2023-10-05T23:53:37Z

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

   * @private
   */
-  private saveListener(name: EventName, listener: EventListener) {
+  private saveListener(name: EventName, originalListener: Function, customListener: Function) {


We need to save both the original listener and the custom listener in order to remove them both correctly from our own internal map and the javascript EventTarget object respectively.

yusinto · 2023-10-05T23:54:24Z

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

@@ -98,7 +98,7 @@ export default interface LDOptions {
   * {@link LDClientDom.variationDetail} method. Since this increases the size of network requests,
   * such information is not sent unless you set this option to true.
   */
-  evaluationReasons?: boolean;
+  withReasons?: boolean;


This is a breaking api change. I'm making this consistent with the server sdks which I think is a good idea.

yusinto · 2023-10-05T23:55:41Z

packages/shared/sdk-client/src/configuration/Configuration.ts

@@ -12,7 +13,7 @@ import type LDOptions from '../api/LDOptions';
 import validators from './validators';

 export default class Configuration {
-  public static DEFAULT_POLLING = 'https://clientsdk.launchdarkly.com';
+  public static DEFAULT_POLLING = 'https://sdk.launchdarkly.com';


According to this confluence page, this is added as part of u2c and so should be the one used. I think either will work.

yusinto · 2023-10-05T23:57:09Z

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

Apologies, this is not done yet, but most of the functionality in this utils class is tested by its consumer, which is fetchFlags. I will get to this soon, in another PR.

yusinto · 2023-10-05T23:57:48Z

packages/shared/sdk-client/src/platform/PlatformDom.ts

I don't see the need for a dom specific platform for now, so I have deleted this.

Ah, yeah, platform dom needs to exist, but we should never use it in shared code. I misread this the first time and made a different comment.

sdk-client should work for any possible client platform. So it cannot have knowledge of platform dom.

But platform dom, or something, would need to exist for the browser SDK. We need to hide fetch for instance, behind a platform. For me this is an immutable requirement.

For instance when we make the node client SDK, we should hoist the node platform out of server and share it. But it probably should be a different platform than browser. If we support node 16, which is currently an LTS, then we shouldn't use actual browser fetch as it was experimental.

The sdk-server only uses Platform, and then implementors can use more specific platforms.

A concrete real example is that electron and node client SDKs should support proxies using the proxy agent. We would not want this code to be in sdk-client it would be handled by the platform fetch and not affect browser at all.

Ok I'll refactor fetch to use Platform.Requests.

kinyoklion

Ok. We need to determine a different base for you do use, because that base you have chosen has already been merged together with my features. So if you merge there, then you will have the mother of all merge conflicts when you want to eventually merge to main.

feat/merge-client-and-migrations contains all of our changes combined, and if what I was going to merge when we are ready to release migrations.

If these changes don't hurt anything, then you could base it on feat/merge-client-and-migration, and then it will get merged to main when we release migrations.

If it needs to be held back, then we should make a new feat/ branch.

I'll look at other things, but I wanted to get the base addressed.

kinyoklion · 2023-10-06T15:46:19Z

packages/shared/sdk-client/package.json

@@ -49,6 +50,7 @@
    "jest": "^29.6.1",
    "jest-diff": "^29.6.1",
    "jest-environment-jsdom": "^29.6.1",
+    "jest-fetch-mock": "^3.0.3",


This has not been published in 4 years, which makes me pretty nervous.

kinyoklion · 2023-10-06T16:03:48Z

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

+): Promise<Flags> => {
+  const fetchUrl = createFetchUrl(sdkKey, context, config);
+  const fetchOptions: RequestInit = createFetchOptions(sdkKey, context, config, platform.info);
+  const response = await fetch(fetchUrl, fetchOptions);


This (fetch) needs to come from the platform.

…plement-initial-flag-fetch

yusinto · 2023-10-09T18:17:30Z

Ok I've merged changes from feat/merge-client-and-migrations and changed the PR root. Everything seems to be fine.

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

…mock. (#297) Co-authored-by: LaunchDarklyReleaseBot <[email protected]>

kinyoklion · 2023-10-10T17:59:25Z

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

We should remove Dom from the name of this one. Can it just be LDClientImpl?

yusinto added 8 commits October 4, 2023 00:04

chore: started adding fetchFeatures function.

d5ec05c

chore: draft implementation of initial fetch and unit tests.

be3277b

fix: lint errors.

12f82a9

chore: replace evaluationReasons => withReasons.

2cf164a

chore: renamed fetchFeatures to fetchFlags.

f52bcd7

fix: bug where LDEmitter does not remove listeners correctly. Support…

e5b2304

… removing only one listener. Emit events on ready and failed. Better comments on dom api usage.

Update LDClientDomImpl.ts

cde71b6

Update LDClientDomImpl.ts

bcc1e1e

yusinto requested review from kinyoklion and louis-launchdarkly as code owners October 5, 2023 23:30

yusinto added the move-js-common label Oct 5, 2023

yusinto commented Oct 5, 2023

View reviewed changes

LaunchDarklyReleaseBot added 2 commits October 5, 2023 16:49

Update LDClientDomImpl.ts

8a9a47e

Update LDEmitter.ts

6442fb6

yusinto commented Oct 5, 2023

View reviewed changes

kinyoklion requested changes Oct 6, 2023

View reviewed changes

kinyoklion reviewed Oct 6, 2023

View reviewed changes

kinyoklion requested changes Oct 6, 2023

View reviewed changes

yusinto changed the base branch from yus/sc-208024/scaffold-js-common-in-js-core to feat/merge-client-and-migrations October 9, 2023 18:11

Merge branch 'feat/merge-client-and-migrations' into yus/sc-219625/im…

09935e7

…plement-initial-flag-fetch

yusinto and others added 2 commits October 9, 2023 15:26

chore: re-add PlatformDom and refactor btoa to platform (#296)

2c33619

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

chore: refactored fetch to use Platform.requests. Removed jest-fetch-…

67c3beb

…mock. (#297) Co-authored-by: LaunchDarklyReleaseBot <[email protected]>

yusinto requested a review from kinyoklion October 10, 2023 17:54

kinyoklion reviewed Oct 10, 2023

View reviewed changes

chore: rename LDClientDomImpl

8560488

yusinto requested a review from kinyoklion October 10, 2023 18:06

chore: remove all dom references.

e3647b8

kinyoklion approved these changes Oct 10, 2023

View reviewed changes

yusinto merged commit 8f34dfa into feat/merge-client-and-migrations Oct 10, 2023
14 checks passed

yusinto deleted the yus/sc-219625/implement-initial-flag-fetch branch October 10, 2023 18:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: implement initial flag fetch #294

chore: implement initial flag fetch #294

yusinto commented Oct 5, 2023 •

edited

Loading

shortcut-integration bot commented Oct 5, 2023

yusinto Oct 5, 2023

yusinto Oct 5, 2023

yusinto Oct 5, 2023

yusinto Oct 5, 2023

yusinto Oct 5, 2023

yusinto Oct 5, 2023

kinyoklion Oct 6, 2023

kinyoklion Oct 6, 2023

yusinto Oct 9, 2023

kinyoklion left a comment

kinyoklion Oct 6, 2023

kinyoklion Oct 6, 2023 •

edited

Loading

yusinto commented Oct 9, 2023

kinyoklion Oct 10, 2023

yusinto Oct 10, 2023

chore: implement initial flag fetch #294

chore: implement initial flag fetch #294

Conversation

yusinto commented Oct 5, 2023 • edited Loading

shortcut-integration bot commented Oct 5, 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

Choose a reason for hiding this comment

kinyoklion Oct 6, 2023 • edited Loading

Choose a reason for hiding this comment

yusinto commented Oct 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yusinto commented Oct 5, 2023 •

edited

Loading

kinyoklion Oct 6, 2023 •

edited

Loading