feat: adds rocketflag provider #1360
Conversation
Signed-off-by: JK Gunnink <[email protected]>
Signed-off-by: JK Gunnink <[email protected]>
jgunnink
force-pushed
the
implement-rocketflag-provider
branch
from
6e49627 to
8518ef6
Compare
|
hey, thank you for adding your provider to all the implementations, please add yourself here to https://github.com/open-feature/js-sdk-contrib/blob/main/.github/component_owners.yml |
Signed-off-by: JK Gunnink <[email protected]>
|
Hey @jgunnink, sorry for the delay, I will have a look tomorrow :) |
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
Thank you for your contribution! This pull request adds the RocketFlag provider, which is a great addition. I've reviewed the code and found a few areas for improvement, primarily concerning configuration paths, caching logic, and adherence to the provider interface. My comments include suggestions to fix these issues, which will help improve the provider's correctness and efficiency. Overall, this is a solid first contribution.
Signed-off-by: JK Gunnink <[email protected]>
Signed-off-by: JK Gunnink <[email protected]>
| .then((flagStatus) => { | ||
| const details: ResolutionDetails<boolean> = { | ||
| value: flagStatus.enabled, | ||
| reason: userContext.cohort ? StandardResolutionReasons.TARGETING_MATCH : StandardResolutionReasons.DEFAULT, |
There was a problem hiding this comment.
TARGETING_MATCH should indicate why a value was evaluated the way it was. In this case, it doesn't look like you'll have enough information in the response to tell.
There was a problem hiding this comment.
I'm not sure I understand, but I will try to answer.
If the user provides a cohort value in the context, then the resolution reason will be a result of targeting match. As to why the API responded that way, that information is not made available in the API response.
There was a problem hiding this comment.
I recommend you use StandardResolutionReasons.DEFAULT since the presence of a cohort isn't enough to determine whether the evaluated value is the result of a targeting match.
| errorCode: ErrorCode.GENERAL, | ||
| errorMessage: err.message, | ||
| }; | ||
| cache.set(cacheKey, details); |
There was a problem hiding this comment.
It looks like errors will be cached indefinitely. Is that okay for your use case?
There was a problem hiding this comment.
Each time a flag is requested, it will be served from the cache if possible. Asynchronously, another request is made to the api (on every call) which will update the cache.
There was a problem hiding this comment.
Please add some basic usage information, include a link to relevant Rocketflag pages, and mention limitations like that the provider only supports booleans. It doesn't have to be too elaborate but we typically direct folks to the readme from the OpenFeature Ecosystem page.
There was a problem hiding this comment.
Good to know thank you. I will update the readme soon.
|
Thank you for your review @beeme1mr, I greatly appreciate it! 😄 |
There was a problem hiding this comment.
Thanks again for the PR!
Finally got to review, I left two comments, after resolving those and the ones from @beeme1mr this is ready to approve from my side :)
| const cacheKey = JSON.stringify({ flagKey, context }); | ||
|
|
||
| // Fetch in the background. | ||
| fetchFlagAndUpdateCache(flagKey, defaultValue, context, cacheKey, client, cache, emitter, logger); |
There was a problem hiding this comment.
Depending on how the app is structured, this could lead to a large number of requests.
The React and Angular SDK e.g. will do many requests as e.g the Angular SDK has a directive that conditionally renders based on the flag. If this is several times in the same templates or e.g. because a view is composed of several components that each change based on the flag, for each of them a request will be done. For React, if hooks are used in several sub components, this would accidentally trigger many requests too.
This can be fixed by only evaluating in the root of the view but it would be better if users did not have to worry too much.
Maybe you could assign something like a TTL to resolved values and only fetch on "non-existence" or stale flags.
But this would be up to you to decide.
| @@ -0,0 +1,17 @@ | |||
| { | |||
| "name": "@openfeature/rocketflag-provider", | |||
There was a problem hiding this comment.
If you plan to add a Node provider, it would be good to include in the name that this is meant for web.
The conventions tends to be suffixing the name with -web. So in this case rocketflag-web-provider
You can check others here: https://github.com/open-feature/js-sdk-contrib/tree/main/libs/providers
This PR
Related Issues
Go SDK provider: open-feature/go-sdk-contrib#725
Notes
This is my first contribution to this repository. I welcome any suggestions and nitpicks so this code can be as useful to the open feature community as possible.
Follow-up Tasks
I'll update the documentation with links to the providers once this is merged in and available.
How to test
npx nx run RocketFlag:testfrom the root of the project