Config-cat-web not forwarding default value to client

[juandelacruz23]

Trying to debug #1212, I realized that config-cat-web-provider is not passing the default value to the underlying client. It's always passing undefined.

Is this a bug or is it expected behaviour? Even on the screenshot of #1212, you can see that the defaultValue evaluated is undefined, although in the example repo I passed false

[beeme1mr]

FYI @lukas-reining @adams85

[adams85]

I don't think this is intended but looks like a bug since e.g. the .NET provider forwards the default value.

[adams85]

After taking a deeper look, I'm no longer sure that this is an actual issue with the provider.

The thing is that the underlying ConfigCat SDK returns the default value passed to it in only one case: when an error occurs during feature flag evaluation. In that case, however, config-cat-web-provider always throws an error: https://github.com/open-feature/js-sdk-contrib/blob/config-cat-provider-v0.7.2/libs/providers/config-cat-web/src/lib/config-cat-web-provider.ts#L133 (A successful evaluation never returns undefined, i.e. when we get undefined, we can be sure that an error occurred. Of course, the provider's current behavior is not too accurate because the cause of the error can as well be other than the flag not found. But this is irrelevant to the current question.)

That is, in the case of config-cat-web provider, you either get a successful ResolutionDetails or an error is thrown. In neither case will the default value be returned. So it doesn't really matter if it's passed to the underlying ConfigCat SDK or not. It affects only the logs.

One thing I'm not sure about is whether it's correct to throw an error in case the underlying provider reports an error. For example, the LaunchDarkly provider returns ResolutionDetails with an error code instead: https://github.com/open-feature/js-sdk-contrib/blob/launchdarkly-client-provider-v0.3.1/libs/providers/launchdarkly-client/src/lib/launchdarkly-client-provider.ts#L129

@lukas-reining Could you tell us which one is the desired behavior?

[lukas-reining]

Hey thanks for having a look @adams85!
Yes, when implementing I decided to not send the default value to the SDK to be sure that we can set the resolution reason to StandardResolutionReasons.DEFAULT. [1]
I am not sure if evaluationDetails.isDefaultValue already existed back then, but this would have been an alternative.

To the question regarding throw vs return: We decided that we do not prefer the providers to return error resolutions themselves. This is actually a proposed breaking change for a possible 2.0 release in the future. This makes error handling for the OpenFeatureClient easier. [2]
So throwing would be the option to prefer.

I think we could use evaluationDetails.isDefaultValue now and check it after resolution to improve the logs.

I can open a PR tomorrow if you like @adams85.
There we can also improve the handling of non-not-found errors. This was actually correct in the initial PR [3], but it change in #918.

[1] https://github.com/open-feature/js-sdk-contrib/pull/334/files#diff-b591f9f22e8f3b5c4eb5ce9c3d54cf28d36ba85b744ea55c2d5d2f1b1e561b46R48
[2] https://github.com/open-feature/js-sdk/issues/799
[3] https://github.com/open-feature/js-sdk-contrib/pull/334/files#diff-b591f9f22e8f3b5c4eb5ce9c3d54cf28d36ba85b744ea55c2d5d2f1b1e561b46R82

[adams85]

Hi @lukas-reining,

To the question regarding throw vs return

Thank you for clearing this up.

I think we could use evaluationDetails.isDefaultValue now and check it after resolution to improve the logs.

Agreed, so I went ahead and made a proposal on how to do this. Feel free to add your suggestions.

[lukas-reining]

Had a look and it looks good @adams85. Only the formatting and import paths have to be fixed for the linting to work.