Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Config-cat-web not forwarding default value to client #1213

Open
juandelacruz23 opened this issue Feb 14, 2025 · 6 comments · May be fixed by #1214
Open

Config-cat-web not forwarding default value to client #1213

juandelacruz23 opened this issue Feb 14, 2025 · 6 comments · May be fixed by #1214

Comments

@juandelacruz23
Copy link

juandelacruz23 commented Feb 14, 2025

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
Copy link
Member

FYI @lukas-reining @adams85

@adams85
Copy link
Contributor

adams85 commented Feb 17, 2025

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

@adams85
Copy link
Contributor

adams85 commented Feb 17, 2025

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
Copy link
Member

lukas-reining commented Feb 17, 2025

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 pushed a commit to adams85/openfeature-js-sdk-contrib that referenced this issue Feb 18, 2025
adams85 pushed a commit to adams85/openfeature-js-sdk-contrib that referenced this issue Feb 18, 2025
@adams85
Copy link
Contributor

adams85 commented Feb 18, 2025

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.

adams85 pushed a commit to adams85/openfeature-js-sdk-contrib that referenced this issue Feb 18, 2025
@lukas-reining
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants