-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
🪟 🔧 Add comment about feature/experiment usage #20123
Conversation
@@ -1,5 +1,8 @@ | |||
/* eslint sort-keys: "error" */ | |||
/** | |||
* Experiments are short-term flags for A/B testing or staged rollouts of features. | |||
* They are only available in Airbyte Cloud. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find this part of the comment a bit confusing in this file. Since the useExperiment
hook has a default value which will be used in OSS always, I feel this might confuse new developers to read this as: "don't use experiments in OSS code", which isn't the case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, I just removed that part since how the default/fallback experiment value works is not actually relevant to the interface here.
@@ -1,3 +1,8 @@ | |||
/** | |||
* FeatureItems are for permanent flags to differentiate features between environments (e.g. Cloud vs. OSS), | |||
* workspaces, specific user groups, etc. The corresponding flag should be added to LaunchDarkly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The corresponding flag should be added to LaunchDarkly.
Not entirely sure we should put that in here? For features we currently don't have that PR that makes each feture an individual flag (#19773) merged yet.
Did you mean to put this on top of the experiments.ts file, where we will need a LD flag for everything?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've just removed it now. I guess it's not necessary to be added to LD (only if you're overriding a default FeatureItem
), so the explanation about how LD works with FeatureService
is probably better off in the Notion docs.
What
Adds some clarifying comments about
FeatureItem
vs.Experiment