Skip to content
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: Add Flagsmith Provider #836

Merged
merged 16 commits into from
Apr 8, 2024

Conversation

kyle-ssg
Copy link
Contributor

@kyle-ssg kyle-ssg commented Apr 2, 2024

This PR

Adds a Flagsmith Provider

Notes

  • This has been tested and has corresponding examples in Standard, React, SSR and React Native frameworks. Sample applications provided here.
  • Flagsmith separates remote configuration from the enabled state, therefore resolveBooleanEvaluation checks the enabled state and others read a feature's remote configuration.

How to test

  • Jest tests provided

Signed-off-by: kyle-ssg <kyle@solidstategroup.com>
@kyle-ssg kyle-ssg requested a review from a team as a code owner April 2, 2024 15:21
@kyle-ssg
Copy link
Contributor Author

kyle-ssg commented Apr 2, 2024

Didn't expect any lint issues, AFAIK I've resolved this here

libs/providers/flagsmith/README.md Outdated Show resolved Hide resolved
libs/providers/flagsmith/README.md Outdated Show resolved Hide resolved
libs/providers/flagsmith/src/lib/flagsmith-provider.ts Outdated Show resolved Hide resolved
libs/providers/flagsmith/src/lib/flagsmith-provider.ts Outdated Show resolved Hide resolved
libs/providers/flagsmith/src/lib/flagsmith-provider.ts Outdated Show resolved Hide resolved
kyle-ssg added 2 commits April 2, 2024 19:33
Signed-off-by: kyle-ssg <kyle@solidstategroup.com>
Signed-off-by: kyle-ssg <kyle@solidstategroup.com>
@toddbaert toddbaert self-requested a review April 2, 2024 20:06
Copy link
Member

@lukas-reining lukas-reining left a comment

Choose a reason for hiding this comment

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

This looks really good! Added some comments about the provider name and type nits.

libs/providers/flagsmith/README.md Outdated Show resolved Hide resolved
libs/providers/flagsmith/package.json Outdated Show resolved Hide resolved
libs/providers/flagsmith/src/lib/flagsmith-provider.ts Outdated Show resolved Hide resolved
libs/providers/flagsmith/src/lib/flagsmith.mocks.ts Outdated Show resolved Hide resolved
libs/providers/flagsmith/src/lib/flagsmith.mocks.ts Outdated Show resolved Hide resolved
libs/providers/flagsmith/src/lib/flagsmith-provider.ts Outdated Show resolved Hide resolved
@toddbaert toddbaert self-requested a review April 3, 2024 16:48
Copy link
Member

@toddbaert toddbaert left a comment

Choose a reason for hiding this comment

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

Looks really good. Left some suggestions.

kyle-ssg and others added 6 commits April 4, 2024 15:10
Co-authored-by: Lukas Reining <lukas.reining@codecentric.de>
Signed-off-by: Kyle Johnson <kyle@solidstategroup.com>
Co-authored-by: Todd Baert <todd.baert@dynatrace.com>
Signed-off-by: Kyle Johnson <kyle@solidstategroup.com>
Co-authored-by: Lukas Reining <lukas.reining@codecentric.de>
Signed-off-by: Kyle Johnson <kyle@solidstategroup.com>
Signed-off-by: kyle-ssg <kyle@solidstategroup.com>
# Conflicts:
#	.release-please-manifest.json
#	package-lock.json
#	release-please-config.json
#	tsconfig.base.json
Signed-off-by: kyle-ssg <kyle@solidstategroup.com>
@beeme1mr
Copy link
Member

beeme1mr commented Apr 5, 2024

Thanks @kyle-ssg, I'll rereview today. Would you like me to publish to npm if everything looks good?

kyle-ssg added 3 commits April 5, 2024 16:33
Signed-off-by: kyle-ssg <kyle@solidstategroup.com>
Signed-off-by: kyle-ssg <kyle@solidstategroup.com>
Signed-off-by: kyle-ssg <kyle@solidstategroup.com>
@kyle-ssg
Copy link
Contributor Author

kyle-ssg commented Apr 5, 2024

Thanks @kyle-ssg, I'll rereview today. Would you like me to publish to npm if everything looks good?

I'm going to be doing a release of the flagsmith SDK to support a couple improvements here. I think I just need clarity on this. I'll keep in draft until I get the SDK release out.

@kyle-ssg kyle-ssg marked this pull request as draft April 5, 2024 15:55
@kyle-ssg
Copy link
Contributor Author

kyle-ssg commented Apr 6, 2024

I've implemented all of the suggested changes and we have a couple improvements upstream ready to help out but I'm finding a bit of an issue with the provider. I don't think it's a blocking issue as I've worked around it in the examples I've made, but this still caught me out.

In attempting the following in an angular application

    OpenFeature.addHandler(ProviderEvents.ConfigurationChanged, handleFlags);
    OpenFeature.addHandler(ProviderEvents.Error, handleFlagsError);
    OpenFeature.setProvider(flagsmithClientProvider)

It works for the most part except It looks like ProviderEvents.ConfigurationChanged doesn't trigger if client.initialise is immediately ready (which is the case if cache is retrieved). Triggering a ProviderEvents.Ready event prior to the ProviderEvents.ConfigurationChanged has no effect. I'm guessing it's essentially due to initialize not resolving prior to the events being emitted.

Im aware that I could do the following, but is there anything else?

  • await setProviderAndWait but then I don't get feature metadata
  • setup a 1 time ProviderEvents.Ready but similar issue and it adds complexity to the consumer application
  • set setTimeout(()=>{emitEvents},0) in the provider. This does work but will result in a flicker
  • Detect that the provider is immediately ready by checking the status after setProvider but it seems there's no way to access event metadata.
ConfigurationChanged.mov

@kyle-ssg kyle-ssg marked this pull request as ready for review April 8, 2024 08:24
Signed-off-by: kyle-ssg <kyle@solidstategroup.com>
@kyle-ssg
Copy link
Contributor Author

kyle-ssg commented Apr 8, 2024

Ok, I think this is ready to go!

Signed-off-by: kyle-ssg <kyle@solidstategroup.com>
Copy link
Member

@beeme1mr beeme1mr left a comment

Choose a reason for hiding this comment

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

Nice job, looks great!

@toddbaert toddbaert requested a review from lukas-reining April 8, 2024 20:24
@toddbaert
Copy link
Member

I've implemented all of the suggested changes and we have a couple improvements upstream ready to help out but I'm finding a bit of an issue with the provider. I don't think it's a blocking issue as I've worked around it in the examples I've made, but this still caught me out.

In attempting the following in an angular application

    OpenFeature.addHandler(ProviderEvents.ConfigurationChanged, handleFlags);
    OpenFeature.addHandler(ProviderEvents.Error, handleFlagsError);
    OpenFeature.setProvider(flagsmithClientProvider)

It works for the most part except It looks like ProviderEvents.ConfigurationChanged doesn't trigger if client.initialise is immediately ready (which is the case if cache is retrieved). Triggering a ProviderEvents.Ready event prior to the ProviderEvents.ConfigurationChanged has no effect. I'm guessing it's essentially due to initialize not resolving prior to the events being emitted.

Im aware that I could do the following, but is there anything else?

  • await setProviderAndWait but then I don't get feature metadata
  • setup a 1 time ProviderEvents.Ready but similar issue and it adds complexity to the consumer application
  • set setTimeout(()=>{emitEvents},0) in the provider. This does work but will result in a flicker
  • Detect that the provider is immediately ready by checking the status after setProvider but it seems there's no way to access event metadata.

ConfigurationChanged.mov

Your assumptions here are all correct, and the solutions are valid. I'll think about this a bit... there may be something that can be done in the SDK but it seems like a bit of an edge case (is it expected or important that you get a change event immediately after becoming ready?)

I guess that in a real-world scenario, if this change event was indeed the result of a change in the remote system, it's just as likely that the change would have occurred before the provider is connected, in which case it would of course not be received. Is this just part of a test scenario?

@toddbaert toddbaert self-requested a review April 8, 2024 20:36
Copy link
Member

@toddbaert toddbaert 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 this looks great.

@toddbaert
Copy link
Member

toddbaert commented Apr 8, 2024

@kyle-ssg I am merging this. Nice work and thanks, as always. A release PR will be opened which I will not merge until you (or Flagsmith other folks) approve it.

@toddbaert toddbaert merged commit dc6e77f into open-feature:main Apr 8, 2024
6 checks passed
@kyle-ssg
Copy link
Contributor Author

kyle-ssg commented Apr 8, 2024

Your assumptions here are all correct, and the solutions are valid. I'll think about this a bit... there may be something that can be done in the SDK but it seems like a bit of an edge case (is it expected or important that you get a change event immediately after becoming ready?)

I guess that in a real-world scenario, if this change event was indeed the result of a change in the remote system, it's just as likely that the change would have occurred before the provider is connected, in which case it would of course not be received. Is this just part of a test scenario?

Yes and no, it was part of an example app where I rely on changes to push to an array of logs to demonstrate OpenFeature. With how things work currently I think all apps will have to just treat the first ready as a change rather than relying on a single event. I think what I was trying to do relied to heavily on the meta that gets returned in the events, really if people are doing that they may as well just query the provider directly if they need info from it.

@kyle-ssg
Copy link
Contributor Author

kyle-ssg commented Apr 8, 2024

@kyle-ssg I am merging this. Nice work and thanks, as always. A release PR will be opened which I will not merge until you (or Flagsmith other folks) approve it.

Awesome, I'm excited to use it!

msamper pushed a commit to growthbook/js-sdk-contrib that referenced this pull request May 16, 2024
Signed-off-by: kyle-ssg <kyle@solidstategroup.com>
Signed-off-by: Kyle Johnson <kyle@solidstategroup.com>
Co-authored-by: Lukas Reining <lukas.reining@codecentric.de>
Co-authored-by: Todd Baert <todd.baert@dynatrace.com>
Signed-off-by: Michael Samper <msamper@growthbook.io>
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