Feat/dependent feature toggles

[FredrikOseberg]

Implements dependent feature toggles for the golang-sdk

[kwasniew]

not sure how idiomatic continue is in golang but I'd rather negate the contains and early exit with return false

[kwasniew]

func (uc *Client) isParentDependencySatisfied(feature *api.Feature, context context.Context) bool {
    if feature == nil || feature.Dependencies == nil || len(*feature.Dependencies) == 0 {
        return true
    }

    for _, parent := range *feature.Dependencies {
        parentToggle := uc.repository.getToggle(parent.Feature)
        isEnabled := uc.isEnabled(parent.Feature, WithContext(context)).Enabled

        if parentToggle == nil || 
           (parentToggle.Dependencies != nil && len(*parentToggle.Dependencies) > 0) || 
           (parent.Enabled != nil && !isEnabled) {
            return false
        }

        if parent.Enabled == nil {
            variantName := uc.getVariantWithoutMetrics(parent.Feature, WithVariantContext(context)).Name
            hasValidVariant := parent.Variants != nil && len(*parent.Variants) > 0 && contains(*parent.Variants, variantName)
            
            if !hasValidVariant && !isEnabled {
                return false
            }
        }
    }

    return true
}

[FredrikOseberg]

Unless we write an every helper, we can't return out here or we will exit the entire function without properly looping over all the dependencies.

[kwasniew]

Looks good!

[kwasniew]

I called this just Dependency in Node SDK with an option to other dependencies. The property inside Feature is indicating what type of dependency it is. If we don't expose this type in the public API then I'm ok with any name.

[FredrikOseberg]

Dependency works fine

[kwasniew]

isn't it more than parent enabled? the variable name may be misleading

[FredrikOseberg]

Agree

[kwasniew]

I'm curious why you decided to do this check outside the isParentDependencySatisfied. I made the opposite decision but didn't have a strong opinion here

[FredrikOseberg]

To me it made more sense readability wise. If I don't have dependencies or the length of dependencies is 0 then I don't need to do check if isParentDependencySatisfied(f, *ctx). Moving it inside that function obfuscates the check that determines whether or not that function should actually be run. I don't have strong opinions about this either, but I found it easier to reason about this way because now the function is limited to doing one thing and one thing only.