feat: add FeatureEnabled to Variant

[jameshartig]

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.

[FredrikOseberg]

@jameshartig

Thank you! This looks good to me.

I agree that the disabled variant should be deprecated. Perhaps the GetDefaultVariant method could be enhanced to take in a parameter and dynamically create the disabled variant with the new FeatureEnabled property? Then we wouldn't have to maintain two methods and two statically defined variants. Would require some additional changes in getVariantWithoutMetrics though.

I'm not actually sure why these methods are public. I can't see any good reason why they should be. I'll make a note of investigating if we can make them private in an upcoming major version.

EDIT: Seems like we merged some adjustments to the tests. Can I ask you to resolve them when you have a free second? Thank you.

[jameshartig]

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]

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]

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

[FredrikOseberg]

Fantastic. Thank you! Merging this now.