refactor(generator/angular): extend FeatureFlagDirective<T> instead of using hostDirectives#218
Conversation
Summary of ChangesHello @tomflenner, 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 introduces a significant architectural change to the Angular generator, moving away from Highlights
Changelog
Activity
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
|
Signed-off-by: backtrack5r3 <flennertom@gmail.com>
This test is currently failing and that what we want. First write a test that fail, then fix it. Signed-off-by: backtrack5r3 <flennertom@gmail.com>
d2fccf6 to
db2db4d
Compare
There was a problem hiding this comment.
Code Review
This is a great refactoring that addresses a fundamental limitation with Angular's handling of hostDirectives and required inputs. The new approach of extending FeatureFlagDirective<T> is much more robust and cleaner. The addition of a compile-time integration test using ngtsc is an excellent way to ensure this issue is resolved and doesn't regress. I have one suggestion to improve the generated code.
|
On hold, waiting for : open-feature/js-sdk#1346 to be merged and a new angular SDK version in order to have access to |
c555841 to
e8222dc
Compare
Remove the usage of typed directive exposed in angular SDK in flavor of custom forked directive. Signed-off-by: backtrack5r3 <flennertom@gmail.com>
Signed-off-by: backtrack5r3 <flennertom@gmail.com>
e8222dc to
a690065
Compare
|
After running into several issues while integrating the generated directive, I decided to update the generated JSDoc accordingly. The goal is to better guide end users on the correct usage, especially clarifying microsyntax vs. property binding. We also updated the name of the generated directive to explicitly include FeatureFlag, in order to avoid naming conflicts with end-user, domain-specific directives that may already exist in applications. For example, in production we have an event-logs feature flag covering the event log system, while the frontend already exposes an existing EventLogDirective. Without this change, the generated directive name could easily conflict with domain-level concepts. Additionally, a note was added documenting the minimum required Angular version for this code to work reliably. Finally, I completed a series of smoke tests covering multiple scenarios we encountered when using the directive in production. These tests reflect real-world usage patterns, and based on the results, the current implementation appears stable and ready for use ✅ |
lukas-reining
left a comment
There was a problem hiding this comment.
Thank you so much @tomflenner!
I have tried generating the flags and it looks great!
The docs are very good, I think is gives all information needed to just use it!
Left some small nits, but its only formatting and docs!
Signed-off-by: backtrack5r3 <flennertom@gmail.com>
c7283b6 to
58d21b5
Compare
|
Another note for history : we did not spot issue in the first iteration of angular due to the way vitest is running test. I am not sure about that but it looks like he doesn't compile same way as a production code so no warning/errors on the |
This PR
Refactors the Angular generator to generate directives extending
FeatureFlagDirective<T>instead of wrappingBooleanFeatureFlagDirectiveviahostDirectives.Previously, the generator relied on
hostDirectivesto map inputs from a generated directive to<Type>FeatureFlagDirective.This approach fails when the underlying directive defines required inputs (
@Input({ required: true })), because Angular enforces required inputs at compile time even if values are set programmatically in the constructor or via lifecycle hooks such asngOnInit.This resulted in compilation errors such as:
To resolve this limitation, the generator now:
FeatureFlagDirective<T>hostDirectivesThis approach avoids Angular required input validation issues and provides a more robust long-term solution for generated structural directives.
Related Issues
Fixes #217
Notes
FeatureFlagDirectiveContext<T>to be exported from the Angular SDK (see [FEATURE] export FeatureFlagDirectiveContext for directive extension support js-sdk#1345).Thanks for the DataGalaxy frontend folks that helped on that refactor 🙏🏻
Follow-up Tasks
FeatureFlagDirectiveContext<T>to be exposed).How to test
make test-integration