open-feature / playground

OpenFeature SDK demos and experimentation
https://openfeature.dev
Apache License 2.0
54 stars 27 forks source link

few questions on the API #1

Closed justinabrahms closed 2 years ago

justinabrahms commented 2 years ago

looking at https://github.com/open-feature/sdk-research/blob/main/packages/openfeature-js/src/lib/types.ts, a few questions came up for me.

export type FlagEvaluationVariationResponse = FlagEvaluationResponse & {
  stringValue?: string;
  boolValue?: string;
  numberValue?: string;
};

I would have expected boolValue to be a bool, etc?

FlagEvaluationRequest should require a default, I believe, in the case of connectivity issues.

There's no way to understand why you got one of those results. e.g. did you match a rule? was it a fallthrough case? Useful for debugging.

Is there a use-case where folks don't know the data type their requesting? I would have guessed that:

  getVariation(
    id: string,
    context?: Context
  ): Promise<FlagEvaluationVariationResponse>

would actually return something like Promise<FlagResponse<Boolean>> instead.

I'm not clear why there's so much overlap on the data inside getVariation and evaluateFlag.

beeme1mr commented 2 years ago

@justinabrahms, good catch! Yes, I messed up the types on the FlagEvaluationVariationResponse. I'll fix that ASAP.

I think handling fallback scenarios should be part of a bigger discussion. In JavaScript, it would be nice to pass in an optional callback. One of the arguments could be an error type. The developer would have more context around the issue so they could handle it how they see fit.

For example:

isEnabled("test-feature", { user: "test-user" }, (errType) => {
 console.log(errType)
 return false;
})

We could also accept a default value like this.

isEnabled("test-feature", { user: "test-user" }, false);
beeme1mr commented 2 years ago

Perhaps it would be better to have methods that explicitly handle data types instead of having a single method. My thought around the single method is that additional data could be included in the response like a flag evaluation summary. What would be your preference?

beeme1mr commented 2 years ago

getVariation is developer facing and evaluateFlag is for the provider.

justinabrahms commented 2 years ago

The SDK we're looking at is a mixture. Either a value-oriented result or a context oriented result.

assert true == isEnabled("test-feature", { user: "test-user" }, false);  // default happy case w/ a match
assert false == isEnabled("test-feature", { user: "test-user" }, false); // case where there was a connectivity issue

versus..

result = contextyApi("test-feature", { user: "test-user" }, false);
assert true == result.getValue()
assert enum.CONNECTIVITY_ISSUE == result.getReason()

That getReason is a terrible API, but hopefully it communicates that you can interrogate how we came to the decision that the value was true.

beeme1mr commented 2 years ago

When would getReason be used? Would it be for troubleshooting only or would you use it in your application logic?

Perhaps something like LaunchDarkly's evaluation reasons would work well.

justinabrahms commented 2 years ago

We'd primarily use it for logs/debugging purposes.

toddbaert commented 2 years ago

I think the current spec addresses most of these concerns. Please re-open if you disagree.