-
Notifications
You must be signed in to change notification settings - Fork 54
feat: nestjs async factory #1328
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?
feat: nestjs async factory #1328
Conversation
Summary of ChangesHello @imranismail, 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 significantly enhances the OpenFeature NestJS module by introducing robust asynchronous configuration capabilities. It allows developers to dynamically configure OpenFeature providers and settings using NestJS's powerful dependency injection, making the module more flexible and adaptable to complex application setups where configuration values might not be immediately available at module registration time. The changes leverage NestJS's Highlights
Using Gemini Code AssistThe 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 by creating a comment using either
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 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
|
0be5fc6 to
7f20700
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 significant and valuable feature by adding asynchronous module configuration capabilities to the OpenFeature NestJS module. The implementation correctly leverages NestJS's ConfigurableModuleBuilder and properly handles asynchronous provider initialization. My review includes a few suggestions to enhance maintainability and test quality, such as using a constant for a repeated injection token, renaming a parameter for better clarity, and improving the robustness of a test case for the logger functionality.
|
Hey @imranismail, thanks for the PR. This is a great addition. |
37cf4aa to
1e148fd
Compare
1e148fd to
ba4b0d6
Compare
Signed-off-by: imranismail <hey@imranismail.dev>
ba4b0d6 to
b657a92
Compare
|
@lukas-reining I've changed the implementation slightly, also ensure existing test passes. I'm a bit concerned about having to specify the domains again in the extras, happy to hear some ideas if you have. |
|
Sorry for the delay @imranismail! |
Signed-off-by: Imran Ismail <hey@imranismail.dev>
Adds
forRootAsync()method to enable asynchronous configuration of the OpenFeature NestJS module using dependency injection. This allows OpenFeature to be configured with values from other providers (e.g.,ConfigService) that are only available at runtime.Changes
Implementation:
ConfigurableModuleBuilderpattern for standardized async module supportforRootAsync()method that supportsuseFactory,useClass, anduseExistingpatternsforRoot()to use the same internalbuildModule()method for consistencysetProviderAndWait()instead of synchronoussetProvider()for proper async handlingNew Features:
forRootAsync()with full DI support (useFactory, inject, imports, etc.)domainsconfiguration in extras for domain-scoped client injection, unfortunately this is required to be able to build the client token for domains, since dependency tree needs to be determined synchronously at "build" time.isGlobalconfiguration option (defaults totrue)Migration Guide
Before (still works):
After (with async config):
Breaking Changes
Minor: The
useGlobalInterceptoroption moved from the main options to extras. However, this is backward compatible because the ConfigurableModuleBuilder merges extras into the options type.