fix(config-cat): Forward default value to underlying client

[adams85]

This PR

Makes an attempt at fixing #1213 by forwarding default value to the underlying client. Also, improves type-safety and error reporting.

Related Issues

Fixes #1213

[lukas-reining]

The change looks good to me!
@adams85 please check the formatting and the linter errors which also seem only be related to imports and format.
You should also be able to fix it by running npm run lint -- --fix from the root.

[adams85]

Okay, it looks like the formatting issues are gone but some test cases are broken.

Unfortunately, I don't have time for fixing them now, will look into that later.

[lukas-reining]

Unfortunately, I don't have time for fixing them now, will look into that later.

Great, thanks!

should throw TypeMismatchError if type is different than expected (3 ms)

All the tests that test for a TypeMismatchError are failing because they receive a GeneralError.
And the resolveObject tests fail additionaly.

[adams85]

All the tests that test for a TypeMismatchError are failing because they receive a GeneralError.
And the resolveObject tests fail additionaly.

Thanks for the guidance. I made some progress and brought the number of failing tests down to 1.

The only test case that fails now is "should throw TypeMismatchError if string is only a JSON primitive", and I'm a bit confused about that.

Could you explain why TypeMismatchError is expected in this case?

The method signature of resolveObjectEvaluation suggests me that it can return any valid JSON value, including a string (the constraint T extends JsonValue theoretically allows that).

[lukas-reining]

You are right @adams85, I agree the test is wrong and we should follow the types allowed.
Are you changing it or should I?

[adams85]

Hmm, it might not be that simple after all... Let me explain.

So again, this is the method signature that's defined by the Provider interface:

resolveObjectEvaluation<T extends JsonValue>(flagKey: string, defaultValue: T, context: EvaluationContext, logger: Logger): ResolutionDetails<T>;

When we call this method like it's done in the problematic test case, we may run into problems:

const result = provider.resolveObjectEvaluation('jsonPrimitive', {}, { targetingKey }));

What's the inferred type of result in the example above? Since the generic type argument T is not specified explicitly, TypeScript will infer T based on the type of the defaultValue argument. Which is JsonObject (more precisely, {}).

What guarantees that this method call always returns a ResolutionDetails<JsonObject> at run-time? From what I see, nothing at the moment... For example, what if feature flag evaluation successfully returns a string containing a JSON array, which is then parsed into a JS array and returned by resolveObjectEvaluation, wrapped in a ResolutionDetails<JsonArray>. The run-time JS type and the compile-time TS type will mismatch!

In other words, this method signature seems to make a "false promise" currently.

I can see two options to resolve this correctly:

1. resolveObjectEvaluation must make sure that the compile-time and run-time types are in sync under all circumstances. That is, it should check whether the default value and the evaluated value types match and if not, it should throw TypeMismatchError.
2. The signature of resolveObjectEvaluation should be changed to not make false promises. To something like this: resolveObjectEvaluation(flagKey: string, defaultValue: JsonValue, context: EvaluationContext, logger: Logger): ResolutionDetails<JsonValue>;. This would eliminate the connection between the type of default value and return value, which is the root cause of the issue.

Option 1 doesn't feel right. Why should this method require feature flag evaluation to return a value of exactly the same type as the default value. That wouldn't make much sense. Plus, as far as I can see, other providers don't do such checks either.

Option 2 seems to be the correct approach but it would be a significant breaking change to a core interface...

So, what's your opinion, what should we do here? Do you maybe have any other ideas to address this type-safety issue correctly? Or should we just simply ignore it?

[adams85]

@lukas-reining @beeme1mr Have you had a chance to think about my comment above? It would be nice to move forward with this fix, so it's ok with me if we ignore this issue now. This is a more fundamental one, not something we should discuss here anyway. Let's open a separate issue for this, shall we?

[beeme1mr]

I think I understand the concern but it would also affect the SDK itself. I think it's best to move on with what you have and discuss possible improvements in a separate issue.

[adams85]

I fixed the remaining failing test case as discussed above, so, as far as I'm concerned, the PR is ready for merge.

[lukas-reining]

Sorry for the delay @adams85!
I totally see the issue here, but I think we will have to open a new issue for this and we might consider this for a 2.0 release.
What do you think?

[lukas-reining]

I fixed the remaining failing test case as discussed above, so, as far as I'm concerned, the PR is ready for merge.

Looks good, thanks!

[adams85]

I totally see the issue here, but I think we will have to open a new issue for this and we might consider this for a 2.0 release.
What do you think?

Done already: open-feature/js-sdk#1158 😄