-
Notifications
You must be signed in to change notification settings - Fork 37
feat(react): add FeatureFlag component #1164
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
base: main
Are you sure you want to change the base?
Conversation
Introduces the FeatureFlag component for React that allow using feature flags in a declarative manner Signed-off-by: Weyert de Boer <[email protected]>
b22a3e3
to
e6614c0
Compare
/** | ||
* The key of the feature flag to evaluate. | ||
*/ | ||
featureKey: string; |
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 think this should be called key
... or possibly flagKey
, but I think that's redundant given the name of the component.
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.
key
is reserved in React: https://react.dev/learn/rendering-lists#keeping-list-items-in-order-with-key
Keys tell React which array item each component corresponds to, so that it can match them up later. This becomes important if your array items can move (e.g. due to sorting), get inserted, or get deleted. A well-chosen key helps React infer what exactly has happened, and make the correct updates to the DOM tree.
so I would vote for flagKey
featureKey: string; | ||
|
||
/** | ||
* Optional value to match against the feature flag value. |
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.
Please specify here exactly what comparison is used\ (==
vs ===
, etc).
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.
if we do the default mentioned below, we can just describe it as By default, strict equality is used
* If a boolean, it will check if the flag is enabled (true) or disabled (false). | ||
* If a string, it will check if the flag variant equals this string. | ||
*/ | ||
match?: string | boolean; |
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 think it would be really cool to add support for a predicate function that accepts the EvaluationDetails
so people can specify exactly how to match.
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 think it would be really cool to add support for a predicate function that accepts the EvaluationDetails so people can specify exactly how to match.
+1
Also with this, you could make this function work for all the types.
And it could have a default that would be
function equals<T>(expected: T, actual: EvaluationDetails<T>){
return expected === actual.value
}
And for objects we could actually make this required if we wanted as this default does not make much sense here.
What do you think @weyert @toddbaert?
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.
Actually ya, I think that's a great idea.
* Default value to use when the feature flag is not found. | ||
*/ | ||
// eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
defaultValue: any; |
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.
You can use the FlagValue
type available from the SDK.
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.
With the predicate from above this could be generic.
The whole component would become nicely typed then as the same type param can be used for the predicate then, so it will always fit the default value.
* Can be a React node or a function that receives flag query details and returns a React node. | ||
*/ | ||
// eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
children: React.ReactNode | ((details: FlagQuery<any>) => React.ReactNode); |
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.
/** | ||
* Optional content to render when the feature flag condition is not met. | ||
*/ | ||
fallback?: React.ReactNode; |
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 think if we are going to only have one alternative child (fallback) we definitely need to do this, so that users can have control over to render this in very specific situations (evaluation errors, reasons, etc).
The alternative (which I don't like as muich) would be to also specify error
components, etc... but I think that isn't as good.
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 alternative (which I don't like as muich) would be to also specify error components, etc... but I think that isn't as good.
I think for it feels a little more idiomatic to have the matcher that is also able to match for error, as you said @toddbaert.
But both could work for me.
/** | ||
* If true, inverts the condition logic (renders children when condition is NOT met). | ||
*/ | ||
negate?: boolean; |
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'm not sure if this is needed if we do this: https://github.com/open-feature/js-sdk/pull/1164/files#r2036805701 I would recommend getting rid of this, then users can white their own invert.
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.
+1
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.
This is really cool. I really like the idea and the concepts. I've made a few minor proposals that I think simply the API but also make it slightly more flexible.
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.
This is a good idea!
Left some thought before approving :)
/** | ||
* The key of the feature flag to evaluate. | ||
*/ | ||
featureKey: string; |
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.
key
is reserved in React: https://react.dev/learn/rendering-lists#keeping-list-items-in-order-with-key
Keys tell React which array item each component corresponds to, so that it can match them up later. This becomes important if your array items can move (e.g. due to sorting), get inserted, or get deleted. A well-chosen key helps React infer what exactly has happened, and make the correct updates to the DOM tree.
so I would vote for flagKey
* If a boolean, it will check if the flag is enabled (true) or disabled (false). | ||
* If a string, it will check if the flag variant equals this string. | ||
*/ | ||
match?: string | boolean; |
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 think it would be really cool to add support for a predicate function that accepts the EvaluationDetails so people can specify exactly how to match.
+1
Also with this, you could make this function work for all the types.
And it could have a default that would be
function equals<T>(expected: T, actual: EvaluationDetails<T>){
return expected === actual.value
}
And for objects we could actually make this required if we wanted as this default does not make much sense here.
What do you think @weyert @toddbaert?
* Default value to use when the feature flag is not found. | ||
*/ | ||
// eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
defaultValue: any; |
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.
With the predicate from above this could be generic.
The whole component would become nicely typed then as the same type param can be used for the predicate then, so it will always fit the default value.
/** | ||
* Optional content to render when the feature flag condition is not met. | ||
*/ | ||
fallback?: React.ReactNode; |
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 alternative (which I don't like as muich) would be to also specify error components, etc... but I think that isn't as good.
I think for it feels a little more idiomatic to have the matcher that is also able to match for error, as you said @toddbaert.
But both could work for me.
/** | ||
* If true, inverts the condition logic (renders children when condition is NOT met). | ||
*/ | ||
negate?: boolean; |
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.
+1
featureKey: string; | ||
|
||
/** | ||
* Optional value to match against the feature flag value. |
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.
if we do the default mentioned below, we can just describe it as By default, strict equality is used
This PR
Introduces the FeatureFlag component for React that allow using feature flags in a declarative manner
Related Issues
No ticket
Notes
Maybe consider adding a similar component for other supported frameworks?
Follow-up Tasks
How to test
I have written unit tests to cover the main features of the component