Unleash / unleash-client-go

Unleash client SDK for Go
https://docs.getunleash.io
Apache License 2.0
138 stars 55 forks source link

feat: add FeatureEnabled to Variant #164

Closed jameshartig closed 10 months ago

jameshartig commented 10 months ago

About the changes

FeatureEnabled is similar to Enabled except in the case where the feature is enabled but there are no variants defined. This follows the client specification case [1].

[1] https://github.com/Unleash/client-specification/blob/c0169d7ace35db66cdf41a7b1b4e390a4a843c3b/specifications/08-variants.json#L448

Closes #159

Important files

api/variant.go

Discussion points

It's a bit unclear what should happen with api.DISABLED_VARIANT. Personally I think GetDefaultVariant should be deprecated and instead users should be checking Enabled on the variant but I'm not sure what other SDKs are doing. I added a new GetDefaultVariantFeatureEnabled method to return the feature-enabled default variant but it's not obvious how anyone outside of the unleash client should be using the GetDefaultVariant method(s). There might need to be more clarification on the methods to discourage or guide usage.

jameshartig commented 10 months ago

Perhaps the GetDefaultVariant method could be enhanced to take in a parameter and dynamically create the disabled variant with the new FeatureEnabled property?

I thought about that but it would be a breaking change. After thinking about it more, what do you think about removing the GetDefaultVariantFeatureEnabled I added to discourage use, especially if we plan to remove the existing method later?

I can move the new variant to not be defined in the api package so it's not public easier.

My thinking is just that it's always easier to expose something if its needed later than to do the opposite.

FredrikOseberg commented 10 months ago

Perhaps the GetDefaultVariant method could be enhanced to take in a parameter and dynamically create the disabled variant with the new FeatureEnabled property?

I thought about that but it would be a breaking change. After thinking about it more, what do you think about removing the GetDefaultVariantFeatureEnabled I added to discourage use, especially if we plan to remove the existing method later?

I can move the new variant to not be defined in the api package so it's not public easier.

My thinking is just that it's always easier to expose something if its needed later than to do the opposite.

Absolutely. We'd have to bump to 5 in that case. Yeah, I think that's a good idea for now, let's keep the new stuff internal and we can always expose it later if it makes sense.

jameshartig commented 10 months ago

@FredrikOseberg thanks for the quick replies! I updated the PR with those changes and added some clarification to the existing method.

FredrikOseberg commented 10 months ago

Fantastic. Thank you! Merging this now.