-
Notifications
You must be signed in to change notification settings - Fork 59
feat: add flagsmith-node-js provider #1374
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
feat: add flagsmith-node-js provider #1374
Conversation
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.
Summary of Changes
Hello @Zaimwa9, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request delivers a new Flagsmith NodeJS Provider for OpenFeature, allowing developers to seamlessly integrate Flagsmith feature flags into their server-side applications. The provider offers flexible configuration for flag evaluation behavior, robust type conversion, and detailed error reporting, ensuring reliable feature flag management. It also includes all necessary project setup and testing infrastructure.
Highlights
- New Flagsmith NodeJS Provider: Introduced a new OpenFeature provider for Flagsmith, enabling server-side applications to integrate with Flagsmith for feature flag management.
- Configurable Flag Resolution: Added configuration options to control how flags are resolved, including handling disabled flags (
returnValueForDisabledFlags), using Flagsmith default values (useFlagsmithDefaults), and determining boolean flag values (useBooleanConfigValue). - Robust Type Conversion and Error Handling: Implemented a
typeFactoryutility for converting flag values to the requested types (boolean, string, number, object) and enhanced error handling for type mismatches, flag not found scenarios, and disabled flags. - Comprehensive Testing: Included extensive unit tests for the new provider, covering various scenarios for initialization, status changes, configuration options, and flag resolution across different data types.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
ef518f3 to
e79eae9
Compare
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.
Code Review
This pull request introduces a new Flagsmith provider for the OpenFeature Node.js server-side SDK. The implementation is well-structured, with clear separation of concerns for the provider logic, type conversions, and error handling. The test coverage is also quite comprehensive, which is excellent.
I've identified a critical issue that could lead to a runtime crash, and another high-severity issue related to unsafe type casting. I've also included a few medium-severity suggestions to improve error handling and code clarity.
Regarding your questions:
- TypeMismatch handling: Your current approach of returning a resolution with an
ERRORreason andTYPE_MISMATCHerror code is correct and aligns with the OpenFeature specification. - Sharing
type-factory.ts: This is a great idea for consistency across providers. It seems generic enough to be moved to a shared location. This would be a good follow-up task to discuss with the repository maintainers.
Overall, this is a solid contribution. Once the critical and high-severity issues are addressed, this provider will be a great addition.
|
@beeme1mr , sorry for the nudge! Is there anything I can do to make this move forward as I am expecting a couple of back and forth :) Additionally, i'm curious about how to make the CI pass. 2 problems here:
|
beeme1mr
left a comment
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.
Hey @Zaimwa9, sorry the delay. Feel free to ping me whenever something get stuck. We have ~50 repos, so it can be difficult to track all the places that need feedback. Sorry about that!
I've left some feedback. The only blockers I've noticed is a missing readme and the provider needs to be registered with Release Please (config, manifest). Could you also please add yourself as the component owner?
Please review my feedback and resolve the threads so I know I can rereview. Thanks!
…pen-feature#1367) Signed-off-by: Patryk Zdunowski <[email protected]> Signed-off-by: wadii <[email protected]>
Signed-off-by: wadii <[email protected]>
Signed-off-by: wadii <[email protected]>
Signed-off-by: wadii <[email protected]>
Signed-off-by: wadii <[email protected]>
Signed-off-by: wadii <[email protected]>
Signed-off-by: wadii <[email protected]>
Signed-off-by: wadii <[email protected]>
Signed-off-by: wadii <[email protected]>
Signed-off-by: wadii <[email protected]>
Signed-off-by: wadii <[email protected]>
…r. (open-feature#1368) Signed-off-by: Ryan Lamb <[email protected]> Signed-off-by: wadii <[email protected]>
…t SDK (open-feature#1371) Signed-off-by: Adam Simon <[email protected]> Co-authored-by: Adam Simon <[email protected]> Signed-off-by: wadii <[email protected]>
…ature#1373) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Signed-off-by: wadii <[email protected]>
…#1370) Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Signed-off-by: wadii <[email protected]>
…pen-feature#1362) Signed-off-by: wadii <[email protected]> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Signed-off-by: wadii <[email protected]>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Signed-off-by: wadii <[email protected]>
Signed-off-by: Michael Beemer <[email protected]> Signed-off-by: wadii <[email protected]>
|
Hey @Zaimwa9, it looks like there may have been an issue with a rebase. Could you take a look when you have a moment? |
…add-flagsmith-server-sdk
Signed-off-by: wadii <[email protected]>
|
@beeme1mr, thanks for noticing, fork was out of sync :) And again thanks for the time you are taking to maintain this project 👍 |
|
The lint step is failing but I can't tell why. Does |
Signed-off-by: wadii <[email protected]>
Signed-off-by: wadii <[email protected]>
Signed-off-by: wadii <[email protected]>
Signed-off-by: wadii <[email protected]>
Signed-off-by: wadii <[email protected]>
Signed-off-by: Thomas Poignant <[email protected]>
This PR
Notes
A couple of questions / point of attentions:
type-factory.tscould be shared with our flagsmith client provider. Any recommended approach?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 providers-flagsmith:testto run the tests