Skip to content

feat(launchdarkly-provider): Add new provider #343

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 12 commits into from
May 13, 2023
Merged

Conversation

Mateoc
Copy link
Contributor

@Mateoc Mateoc commented Apr 27, 2023

This PR

Implements a provider for LaunchDarkly launchdarkly-js-client-sdk.
Is based on the provider for the node sdk

  • LaunchDarklyClientProvider

@Mateoc Mateoc requested a review from a team April 27, 2023 21:47
@toddbaert
Copy link
Member

Hey @Mateoc ! Thanks for your contribution!

We're currently working on updates to the OpenFeature specification to support its usage in web-clients, as well as other client contexts. See here for the progress.

I don't think that the current js-sdk has the required APIs to support a proper LaunchDarkly provider for use in browser contexts. We do however have an experimental web-sdk. I won't speak for LaunchDarkly, but I'd feel more comfortable releasing an experimental provider based on that, than something based on the server-side SDK. You can check out the playground, which features a demo quality LaunchDarkly provider.

@Mateoc
Copy link
Contributor Author

Mateoc commented Apr 28, 2023

@toddbaert Thanks for the quick response. I didn't notice this was exclusively server side specially since there are other web libraries using it (probably not meant for client usage I did not check them too much), I will check the experimental library, at a first glance the interface seems similar enough for it to be a simple change

If I end up fixing it, should I open a new pr here ? or do you have a different repo for contributions on web providers ?

@toddbaert
Copy link
Member

@toddbaert Thanks for the quick response. I didn't notice this was exclusively server side specially since there are other web libraries using it (probably not meant for client usage I did not check them too much), I will check the experimental library, at a first glance the interface seems similar enough for it to be a simple change

If I end up fixing it, should I open a new pr here ? or do you have a different repo for contributions on web providers ?

This is a fine location! The flagd-web provider is another client-side example based on the experimental web-sdk.

Just out of curiosity, are you intending on using this in production somewhere? Or is this more of an experiment?

@c4milo
Copy link
Member

c4milo commented Apr 28, 2023

@toddbaert, production! @Mateoc works with me at @redpanda-data :D

@Mateoc
Copy link
Contributor Author

Mateoc commented May 2, 2023

@toddbaert I made the refactor to use '@openfeature/web-sdk' there is a question I could not find the answer:
though the context in this SDK is not a parameter to the get functions (example: getBooleanDetails) , it still passes it on the evaluation methods, I assumed it always passed the same context until OpenFeature.setContext is called (assumption seems to be right as far as I test), so I just ignored that parameter and just handled the context change on onContextChange which is called when OpenFeature.setContext is called.
Regarding you previous question, we are still doing a couple of POC but yes the aim is to use this on prod.

@toddbaert
Copy link
Member

@Mateoc I'll give this a thorough review today or tomorrow.

@toddbaert
Copy link
Member

toddbaert commented May 4, 2023

I assumed it always passed the same context until OpenFeature.setContext is called (assumption seems to be right as far as I test), so I just ignored that parameter and just handled the context change on onContextChange which is called when OpenFeature.setContext is called.

@Mateoc This is correct. The reason it's passed is some other solutions' SDKs don't have the equivalent of an identify and can accept context in their evaluation methods without such a method being called. Usually this is because those SDKs do all their evaluation locally on a cached ruleset. In the case of LD, what you've done is correct.

@toddbaert
Copy link
Member

@Mateoc @kinyoklion the web-sdk implements our (yet to be released) events specification. You can see a primitive example here.

It's not necessary to implement it in this provider now, but I wanted to bring it to your attention, since it's likely particularly useful for web client scenarios. I should mention that because this API is not yet specified, it's certainly subject to change.

@Mateoc
Copy link
Contributor Author

Mateoc commented May 4, 2023

@toddbaert thanks for all you feedback!
Let me know if the license part is ok or if I should use a different format.
Also let me know if I miss something or if you find something new.

@toddbaert toddbaert requested review from kinyoklion and beeme1mr May 5, 2023 18:51
@toddbaert
Copy link
Member

I've added a few more minor comments. I haven't had a chance to test this myself, but I will do that soon. That's the last thing required for approval on my end.

I'd like to have approval from at least one person from LaunchDarkly, @kinyoklion or otherwise. Particularly because this is built on an experimental SDK, I'd like the explicit approval of somebody representing their organization. I hope you can appreciate that @Mateoc .

@Mateoc
Copy link
Contributor Author

Mateoc commented May 8, 2023

@toddbaert I just fix latest comments. Thanks!
There is no rush lets wait to see what they say.
Also we will need to create a module that integrates @openfeature/web-sdk with react. If we end up creating something worth sharing where could I share it ?

@kinyoklion
Copy link
Member

@Mateoc looks like you need to sign-off on some of your commits.

@toddbaert
Copy link
Member

@Mateoc you have some lint errors. Mostly around @ts-ignore. I would just add whatever cases you need to solve the same problem.

@Mateoc Mateoc force-pushed the main branch 2 times, most recently from 351d0b6 to ed27715 Compare May 10, 2023 14:59
Copy link
Member

@kinyoklion kinyoklion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can improve the initialization, but I think this is probably fine considering this will be experimental at this point in time.

@toddbaert
Copy link
Member

toddbaert commented May 10, 2023

@Mateoc I was able to test this locally and it worked as I'd expect, with the exception of the build (which also failed in the CI). You'll need to add your dependencies to the root package.json. Our NX tooling will automatically add the correct dependencies to your module's package.json (you should be able to run npm run package and confirm that the module's package.json in the dist has your deps in it).

The only thing you should have to do manually is specify peer deps, which you've done, though I recommend changing your version match for the web-sdk, since we treat minor as major under v1.0

},
"dependencies": {
"lodash.isequal": "^4.5.0"
},
"devDependencies": {
"@types/lodash.isequal": "^4.5.6"
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
},
"dependencies": {
"lodash.isequal": "^4.5.0"
},
"devDependencies": {
"@types/lodash.isequal": "^4.5.6"
}
}

Copy link
Member

@toddbaert toddbaert May 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These should compute automatically in the dist/package.json if you add them to the root package.json

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done
I did not think this was going to work, is not even in the peer, how ?

Copy link
Member

@toddbaert toddbaert May 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nx should automatically compute these and put them in the distributed module's package.json. That is how it worked until a recent bug in nx. Upgrading to nx v16 fixes it. I'll do that in a separate PR and make sure everything works again before releasing. I've rebased your branch on main and resolved some conflicts. I'm OK to merge now unless you have any objections.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Mateoc FYI, this is how things SHOULD work: #362

It generates the following deps for you:
image

@toddbaert toddbaert self-requested a review May 10, 2023 19:16
Signed-off-by: Todd Baert <[email protected]>
@toddbaert toddbaert merged commit 10511c7 into open-feature:main May 13, 2023
@toddbaert
Copy link
Member

toddbaert commented May 14, 2023

@Mateoc when this is merged, your provider will be available on npm. I'll do that tomorrow or Monday.

@Mateoc
Copy link
Contributor Author

Mateoc commented May 15, 2023

@toddbaert awesome, thanks

@toddbaert
Copy link
Member

@Mateoc : https://www.npmjs.com/package/@openfeature/launchdarkly-client-provider

@c4milo
Copy link
Member

c4milo commented May 31, 2023

Thanks @toddbaert for all the assistance!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants