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

WithVariantFallback and WithVariantFallbackFunc are called when feature isn't found #160

Closed
jameshartig opened this issue Dec 7, 2023 · 3 comments · Fixed by #171
Closed
Assignees
Labels

Comments

@jameshartig
Copy link
Contributor

Describe the bug

WithVariantFallback and WithVariantFallbackFunc both say they are called when no variant is found but they're actually called when a feature isn't found. There's no way to provide a fallback when variants aren't found.

Steps to reproduce the bug

  1. Call GetVariant on a non-existent feature with WithVariantFallback.
  2. Observe fallback being used when the feature doesn't exist

Expected behavior

I expected the options to match their descriptions and only be called when the variant isn't found.

Logs, error output, etc.

No response

Screenshots

No response

Additional context

No response

Unleash version

No response

Subscription type

Pro

Hosting type

Hosted by Unleash

SDK information (language and version)

Version 4.0.1

@thomasheartman
Copy link
Contributor

I've done some digging, some comparing with other SDKs, and you're completely right 🙌🏼 The issue here is with the implementation. The fallback should be used when there is no flag, when the flag doesn't have any variants, and if the flag has variants, but is disabled.

If you feel like you wanna tackle this too, then we'd be very happy to review the PR when it comes through. If not, we'll pencil it in and probably get to fixing this early next week (depending on other incoming tasks).

@jameshartig
Copy link
Contributor Author

The fallback should be used when there is no flag, when the flag doesn't have any variants, and if the flag has variants, but is disabled.

I do believe this would resolve our issue. Thanks!

If you feel like you wanna tackle this too, then we'd be very happy to review the PR when it comes through.

I didn't have any time today and my Monday is already booked. I'll post next week if Tuesday is better.

@thomasheartman
Copy link
Contributor

I didn't have any time today and my Monday is already booked. I'll post next week if Tuesday is better.

No worries! This was at the top of our pile today, so I got a PR up #171 that I believe should fix this 😄

@thomasheartman thomasheartman moved this from Approved PRs to In Progress in Issues and PRs Dec 18, 2023
thomasheartman added a commit that referenced this issue Dec 21, 2023
This PR fixes a bug in how we use fallback variants in this SDK.

The fallback should be used when there is no flag, when the flag doesn't have any variants, and if the flag has variants, but is disabled. However, prior to this, it was only used if the flag didn't exist.

It addresses the issues in and closes #160.

* fix: add tests and impl

* fix: update comments

* fix: minor rename

* fix: use noopListener

* fix: don't change the "FeatureEnabled" state of the fallback variant

* Chore: remove empty struct from list

* Test: ensure that the fallback variant's `FeatureEnabled` is unchanged

* Docs: add notes about `FeatureEnabled` to function docs
@github-project-automation github-project-automation bot moved this from In Progress to Done in Issues and PRs Dec 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
2 participants