[Discussion] EvaluationOptions confusion

[james-milligan]

The Evaluation Options structure is currently a little confusing when implementing a provider.

Currently the structure is as follows (taken from the golang-sdk, but mirrored in other languages):

type EvaluationOptions struct {
    hooks     []Hook
    hookHints HookHints
}

This structure is currently being passed from the Client to the FeatureProvider, this is intentional as the hookHints may be used to alter the behaviour of the given provider, such as mutating the resulting flag value before it is passed back into the client. However, the FeatureProvider should not be altering the existing hooks, nor should it be calling their respective methods, so passing the hooks/hookHints as an argument may be a little confusing and breaks the separation of interest between the Client and FeatureProvider.

I propose that either an additional field should be added to the Evaluation Options structure, such as providerHints for use in the FeatureProvider, or for a similar structure to be passed to the provider directly to reduce confusion.

type EvaluationOptions struct {
    hooks     []Hook
    hookHints HookHints
        providerHints ProviderHints
}
type ProviderHints struct {
    mapOfHints map[string]interface{}
}

Personally I think that only passing relevant information to the FeatureProvider would be favourable in this situation, especially if the EvaluationOptions has the possibility of becoming bloated with hooks/hookHints that should not be used downstream from the Client

[james-milligan]

@beeme1mr @toddbaert @skyerus @justinabrahms @thomaspoignant

[toddbaert]

I fully agree with the problem statement. I think we're leaking things to providers that we shouldn't (hooks, hookhints).

As for the solution, I would be open to either passing only providerHints as you suggest, or simply removing this argument entirely in the FeatureProvider interface (we may be solving problems we don't yet have).

Let's allow others to weight in.

[skyerus]

My preference would be to remove the argument entirely if the provider has no need for it to avoid any future confusion.

[toddbaert]

We could just rename hookHints to something more general, and then pass this bucket object to both hooks and providers as a way to supply arbitrary data to both. I'm still open to any of the above.

[justinabrahms]

I vote to remove the argument. If a provider wants access to it, they can do that via provider hooks.

[beeme1mr]

I vote to remove the argument. If a provider wants access to it, they can do that via provider hooks.

I completely agree and was just about to write the same thing. Simply removing the options arguments from the resolvers would make the separation more clear.

@james-milligan, could you please open a PR for this?

[benjiro]

Yup totally agree with the problem statement. That's a +1 vote for me.