optimizely / javascript-sdk

JavaScript SDK for Optimizely Feature Experimentation and Optimizely Full Stack (legacy)
https://www.optimizely.com/products/experiment/feature-experimentation/
Apache License 2.0
76 stars 80 forks source link

JS node SDK: isFeatureEnabled returning false even if the feature is enabled #756

Closed ghost closed 6 months ago

ghost commented 2 years ago

What I wanted to do

Ask if feature A is enabled when using attributes with undefined value

What I expected to happen

To get True if feature A is enabled

What actually happened

I am getting False event though feature is enabled

Steps to reproduce

You will get false... This is because if the attribute has undefined then you just stop evaluating the feature for some reason and you return false... Imho we should not return false and continue evaluation of feature flag OR at least document this behaviour and update the TS types for that function

"@optimizely/optimizely-sdk": "4.9.1", node version: v16.13.0

zashraf1985 commented 2 years ago

@dusan-dragon I could not try this exact scenario at my end yet but our documentation does specify what type of values attributes can take and undefined is not among them. Here is what the documentation here says

You can pass strings, numbers, Booleans, and null as custom user attribute values.

It appears that null is allowed but undefined is not.

ghost commented 2 years ago

@zashraf1985 yeah but you are linking to kind of unrelated code right? I am not talking about user context stuff here...

Here are types which says anything is allowed https://github.com/optimizely/javascript-sdk/blob/master/packages/optimizely-sdk/lib/shared_types.ts#L40

Do you see a reason why undefined should not be allowed though? I mean even if it is not allowed, should the lib just ignore that attribute instead of stopping evaluation of everything?

zashraf1985 commented 2 years ago

You are right. I sent link to the wrong version of the documentation. Here is the correct link but still says the same thing about user attributes.

You are right about the type definition. We should update that to exactly allow only the types that are allowed.

Do you see a reason why undefined should not be allowed though? I mean even if it is not allowed, should the lib just ignore that attribute instead of stopping evaluation of everything?

This is something that needs more time to look up and answer but the quickest solution for you would be to see if you can either use null instead of undefined or remove the attribute altogether when it is undefined.

ghost commented 2 years ago

I am able to create my own workaround. I am reporting this issue so it can get fixed... Also the best documentation is code itself, and if the code (types) says it accept anything 🤷‍♂️

Anyway, it would be nice if it would accept also undefined so we do not need to have workarounds for it

zashraf1985 commented 2 years ago

It's Great to hear that you got around it using your own technique.

I fully agree with both your points. We are adding both these points to our backlog and will try to prioritize them. The first one looks like a low hanging fruit so we might actually do it pretty soon. For the second one, we will need to dig in to the code and see whats wrong and it might take some time.

Thanks a lot for the great feedback.

mikechu-optimizely commented 10 months ago

This is part of a maintenance story (internal ticket FSSDK-8508) that's being broken down. We'll look to get a ticket issued for this. Thanks for contributing.

@Tamara-Barum

raju-opti commented 6 months ago

886 updates the UserAttribute type with specific types instead of any. So the typescirpt type is now consistent that unknown is not allowed. Closing this issue.