Most basic open feature provider for node. #1

kinyoklion · 2022-08-25T22:26:47Z

This PR creates a basic provider with is compatible with open feature.

A large amount of the change is just adding the scaffolding (README, CONTRIBUTING, tsconfig, eslint, package.json, etc.)

If it is problematic for review I could make a scaffold PR first.

This is an example script to demonstrate use.

import { OpenFeature } from '@openfeature/nodejs-sdk';
import { init } from 'launchdarkly-node-server-sdk';
import { LaunchDarklyProvider } from 'open-feature-node';


async function main() {
  const ldClient = init('your_key');
  await ldClient.waitForInitialization();
  OpenFeature.setProvider(new LaunchDarklyProvider(ldClient));
  
  const client = OpenFeature.getClient();
  const detail = await client.getBooleanDetails('app-enabled', false, {targetingKey: 'potato'});
  console.log("The value %s", JSON.stringify(detail));

  const unspecifiedUserValue = await client.getBooleanDetails('app-enabled', false);
  console.log("The value for an unspecified user was %s", JSON.stringify(unspecifiedUserValue));

  const wrongType = await client.getStringDetails('app-enabled', "defaultValue", {targetingKey: 'potato'});
  console.log("The value for the wrong type was", JSON.stringify(wrongType));
}

main();

After the release of the TS re-write of the server SDK I can probably de-node this. But with the current version throwing node in there makes sense.

…onversion.

kinyoklion · 2022-08-26T23:26:47Z

README.md

@@ -0,0 +1,76 @@
+# LaunchDarkly OpenFeature provider for the Server-Side SDK for Node.js
+
+This provider allows for using LaunchDarkly with the OpenFeature Node SDK.


It would probably be nice to have some boilerplate about what open feature is and why someone should care.

kinyoklion · 2022-08-26T23:27:07Z

CONTRIBUTING.md

@@ -0,0 +1,43 @@
+# Contributing to the LaunchDarkly OpenFeature provider for the Server-Side SDK for Node.js
+
+LaunchDarkly has published an [SDK contributor's guide](https://docs.launchdarkly.com/sdk/concepts/contributors-guide) that provides a detailed explanation of how our SDKs work. See below for additional information on how to contribute to this provider.


Should be close enough as far as a contribution guide goes.

kinyoklion · 2022-08-26T23:27:48Z

src/translateContext.ts

+ *
+ * @internal
+ */
+export default function translateContext(evalContext: EvaluationContext): LDUser {


In some ways this will get much simpler with the U2C version.

kinyoklion · 2022-08-26T23:30:50Z

README.md

+```
+npm install @openfeature/nodejs-sdk
+npm install launchdarkly-node-server-sdk
+npm install @launchdarkly/open-feature-node


This is, of course, dirty lies at the moment.

src/translateResult.ts

…date-error-handling

eli-darkly · 2022-09-13T22:48:36Z

CONTRIBUTING.md

+npm test
+```
+
+### Auditing package dependencies


I'm not 100% sure this stuff is still relevant or the best way to do things. The problem I was trying to solve at the time was that we only had npm audit for finding vulnerabilities, and it would constantly flag dev dependencies of jest/babel/etc. If we can do better now with Dependabot or something else, then we could probably ditch this script.

I can remove it. I think dependabot is generally reasonable. (Though it still probably flags more than needed, this project doesn't have a great deal of dependencies.)

kinyoklion · 2022-09-13T22:54:59Z

package.json

+  ],
+  "license": "Apache-2.0",
+  "peerDependencies": {
+    "@openfeature/nodejs-sdk": "^0.3.2",


I think doing these as peer dependencies should give us the best compatibility experience. I am open to change though if there is some reason not to do this.

I think that that is exactly the intended use of peer dependencies. Our library is a thing that's meant to be used with the same instance of the OF SDK that the application is using; there isn't any case in which we would want npm to give us a different one, if we specified a different version.

eli-darkly

Fairly minor comments.

README.md

src/LaunchDarklyProvider.ts

src/translateContext.ts

eli-darkly · 2022-09-13T23:00:44Z

src/translateResult.ts

+    case 'FLAG_NOT_FOUND':
+      return 'FLAG_NOT_FOUND';
+    // General errors.
+    case 'USER_NOT_SPECIFIED':


What's the intention with this line? It looks like it's not doing anything default isn't doing.

This is just habit of including all the known values. For me this makes it clear that USER_NOT_SPECIFIED was considered, and does the same as default. Versus it just being forgotten about.

I could use a comment to accomplish the same thing if you prefer.

A comment would make it a little clearer that this isn't just a mistake. Some other languages don't allow fallthroughs in switch at all because it's so easy to make such mistakes.

Yeah, true, though I am more accustomed to C# for instance, where empty cases specifically are allowed to fallthrough, but not ones with code, because that is more likely to be a mistake.

Regardless I can add a comment.

eli-darkly · 2022-09-13T23:28:07Z

package.json

+  ],
+  "license": "Apache-2.0",
+  "peerDependencies": {
+    "@openfeature/nodejs-sdk": "^0.3.2",


I think that that is exactly the intended use of peer dependencies. Our library is a thing that's meant to be used with the same instance of the OF SDK that the application is using; there isn't any case in which we would want npm to give us a different one, if we specified a different version.

eli-darkly · 2022-09-13T23:31:29Z

src/LaunchDarklyProvider.ts

+    defaultValue: boolean,
+    context: EvaluationContext,
+  ): Promise<ResolutionDetails<boolean>> {
+    const res = await this.client.variationDetail(flagKey, translateContext(context), defaultValue);


Am I right that in OpenFeature, every flag evaluation by a provider is "with detail"— i.e. even though the facade also has "value only" methods, those are always implemented by calling a method like this and then throwing away everything but the value?

If so, there is one slight possibly unintended consequence that probably no one will care about, but we might want to document it anyway. The LD SDKs will include reason data in evaluation events if either an experiment was involved... or you called variationDetail. Now, probably no one will care about that because individual evaluation events aren't normally sent; it would only really make a difference if full event tracking is turned on for a flag, and data export is being used. And maybe this has already been discussed, but I just wanted to bring it up for the record if not.

Yes, thank you. I considered this, and I thought I brought it up in the open feature discussion, but likely before we recorded and while your internet was being crazy.

I wonder what the appropriate documentation for this would be. Maybe I could add something to the readme along the lines of evaluations operating as if you were using variationDetail, which will include reasons.

It used to be more of an important distinction than it is now, because the SDKs used to send an individual event for every evaluation no matter what. Now it's more of an edge case, and I'm not really sure of the best way to explain it without confusing the vast majority of customers to whom it doesn't apply. I mean, the explanation I wrote above kind of assumes that you're familiar with lower-level SDK implementation details.

Ok. I will leave this out of the readme for now. Though I probably need to make an internal open feature FAQ or something to contain it.

kinyoklion added 11 commits August 25, 2022 15:07

Most basic open feature provider for node.

cd5078a

Change the LD node SDK to be a peer dependency.

823d746

Remove validators.

7014af0

Add documentation.

d100e6f

Add README.md

426c679

Missing word.

69889b0

Extend README.md, add CONTRIBUTING.md, and add better-audit.sh

e151b6e

Explain context conversion in the README.md. Implement more context c…

fec2981

…onversion.

Add unit tests for translateContext.

b3b0e02

Improve context translation.

abfca0f

Add tests for the LaunchDarklyProvider.

53c2780

kinyoklion requested review from InTheCloudDan and yusinto August 26, 2022 23:22

kinyoklion marked this pull request as ready for review August 26, 2022 23:22

Comment about working with open feature.

806fec3

kinyoklion commented Aug 26, 2022

View reviewed changes

Add a blank line to the end of the package.json.

d2f5854

kinyoklion commented Aug 26, 2022

View reviewed changes

Change WRONG_TYPE to TYPE_MISMATCH, which is from the specification.

4bc8e32

kinyoklion commented Aug 29, 2022

View reviewed changes

src/translateResult.ts Outdated Show resolved Hide resolved

kinyoklion added 8 commits September 13, 2022 15:25

Translate error codes.

38d7e51

Lint

0bf72cc

Merge from initial implementation.

03090d3

Update open feature version.

1f1aa8d

Update open feature version.

1c7f154

Update build settings.

3e729b0

Merge branch 'rlamb/implement-initial-client' into rlamb/sc-167589/up…

f332a22

…date-error-handling

Clarify server.

905b0d3

Merge branch 'rlamb/implement-initial-client' into rlamb/sc-167589/up…

4526b31

…date-error-handling

kinyoklion requested a review from eli-darkly September 13, 2022 22:44

eli-darkly reviewed Sep 13, 2022

View reviewed changes

Remove better audit.

2001416

kinyoklion commented Sep 13, 2022

View reviewed changes

eli-darkly suggested changes Sep 13, 2022

View reviewed changes

kinyoklion added 3 commits September 14, 2022 09:27

Correct sample code in README.

02afc39

Update README with better text for explaining the key attribute.

2b5855a

Remove explicit typings for wrongType results.

f7d5e4e

eli-darkly approved these changes Sep 14, 2022

View reviewed changes

Specify that USER_NOT_SPECIFIED is also default.

e7adc97

kinyoklion merged commit bfc30ba into main Sep 14, 2022

kinyoklion deleted the rlamb/implement-initial-client branch September 14, 2022 20:04

		@@ -0,0 +1,76 @@
		# LaunchDarkly OpenFeature provider for the Server-Side SDK for Node.js

		This provider allows for using LaunchDarkly with the OpenFeature Node SDK.

		@@ -0,0 +1,43 @@
		# Contributing to the LaunchDarkly OpenFeature provider for the Server-Side SDK for Node.js

		LaunchDarkly has published an [SDK contributor's guide](https://docs.launchdarkly.com/sdk/concepts/contributors-guide) that provides a detailed explanation of how our SDKs work. See below for additional information on how to contribute to this provider.

Most basic open feature provider for node. #1

Most basic open feature provider for node. #1

Uh oh!

Conversation

kinyoklion commented Aug 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eli-darkly left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eli-darkly Sep 14, 2022 • 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!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kinyoklion commented Aug 25, 2022 •

edited

Loading

eli-darkly Sep 14, 2022 •

edited

Loading