-
Notifications
You must be signed in to change notification settings - Fork 37
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
feat!: support streaming connection and initialization fixes #418
feat!: support streaming connection and initialization fixes #418
Conversation
cc843ac
to
f5541a1
Compare
f5541a1
to
557e6c5
Compare
b1484c6
to
afaebca
Compare
libs/providers/launchdarkly-client/src/lib/launchdarkly-client-provider.ts
Outdated
Show resolved
Hide resolved
libs/providers/launchdarkly-client/src/lib/launchdarkly-client-provider.ts
Outdated
Show resolved
Hide resolved
libs/providers/launchdarkly-client/src/lib/launchdarkly-client-provider.ts
Outdated
Show resolved
Hide resolved
libs/providers/launchdarkly-client/src/lib/launchdarkly-client-provider.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These look like solid improvements to me. I have a few small concerns, particularly this.
@sago2k8 Based on these policies, we will soon be adding a CODEOWNERs file for each sub component in these monorepos. It's clear you understand the SDK as well as this provider. Would you like to be added to the CODEOWNERS for this component?
I'm awaiting additional reviews, particularly from @kinyoklion and @Mateoc
Sure, Happy to be part of the CODEOWERS |
685ff58
to
30312b6
Compare
libs/providers/launchdarkly-client/src/lib/launchdarkly-client-provider.ts
Outdated
Show resolved
Hide resolved
libs/providers/launchdarkly-client/src/lib/launchdarkly-provider-options.ts
Outdated
Show resolved
Hide resolved
215625b
to
b86e050
Compare
Thanks for the review @toddbaert, I addressed your comments. Waiting for the extra review I can safely say @Mateoc will be ok with the constructor changes, but better if we get confirmation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great.
I'm not sure if you have any strong opinion on this, but though this does fix some behavior, it also adds significant new features. I wonder if feat!
would be a better commit type?
I have removed "launch darkly client" from the title since our release flow automatically updates the readmes/release in such a way it's clear which component the commit applies to.
Agree, set title to feat! |
libs/providers/launchdarkly-client/src/lib/launchdarkly-client-provider.ts
Outdated
Show resolved
Hide resolved
libs/providers/launchdarkly-client/src/lib/launchdarkly-client-provider.ts
Outdated
Show resolved
Hide resolved
@sago2k8 @Mateoc related PR for ownership of this component: https://github.com/open-feature/js-sdk-contrib/pull/446/files#diff-0829329780a5428e2182662b54354b425c340a08b0b4468048abce4bd9d0312bR23-R26 |
@sago2k8 It seems like @kinyoklion still has a concern, but the CI failure is unrelated to your changes. I will fix those in another PR. |
096e895
to
4333a85
Compare
@sago2k8 I'm merging main into this to fix the CI issue. |
36e283e
to
022c38a
Compare
libs/providers/launchdarkly-client/src/lib/launchdarkly-client-provider.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor comment, but I think things look good.
I'll give @sago2k8 some time to address any nits or make final changes. Will merge tomorrow EOD unless I hear otherwise. |
- Update launch darkly sdk to latest and update the semantic definition in the package.json - Allow any version of open feature sdk, I would encourage to use latest but until it's stable it make sense to use any '*' Signed-off-by: Santiago Jimenez Giraldo <[email protected]>
Update documentation on the provider options Signed-off-by: Santiago Jimenez Giraldo <[email protected]>
022c38a
to
04ef788
Compare
BREAKING CHANGE: now the arguments of the constructor match the Launch Darkly initialization arguments, plus an extra argument for setting different loggers. - fix initialize function to simplify the usage of the provider. - Implements streaming flag evaluation. Signed-off-by: Santiago Jimenez Giraldo <[email protected]>
Replace imports from js-sdk to web-sdk, to reduce required dependencies Signed-off-by: Santiago Jimenez Giraldo <[email protected]>
apply single quote style guide Signed-off-by: Santiago Jimenez Giraldo <[email protected]>
Add description to readme, explain new initialization. Signed-off-by: Santiago Jimenez Giraldo <[email protected]>
Update tests to match the result variations spec Signed-off-by: Santiago Jimenez Giraldo <[email protected]>
04ef788
to
f429eb3
Compare
Hey @toddbaert I don't think I have received and invitation to an org yet. |
Very sorry @sago2k8 ! I added you and then removed you again when I alphabetized 🤦 . I've opened another PR to add you. When this is merged, you will get an invite. |
This PR
Notes
This is used for our team at Redpanda, we use open feature across the stack and we want to use it in our frontend apps too.
Follow-up Tasks
Related issues:
How to test