[CL-875] 0028 - Adopt Angular Signals for Component State #638
Conversation
Deploying contributing-docs with
|
| Latest commit: |
c977fa5
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://c235cbb4.contributing-docs.pages.dev |
| Branch Preview URL: | https://signals-adr.contributing-docs.pages.dev |
|
Great job! No new security vulnerabilities introduced in this pull request |
willmartian
changed the title
willmartian
changed the title
|
Claude finished @willmartian's task —— View job Review completeThis ADR documents an important architectural decision for Angular development at Bitwarden. The structure follows the established ADR pattern well, and the decision is clearly explained. Summary of findingsFinding 1: Performance claim needs supporting evidence or clarification (💭) Finding 2: Grammatical issue with punctuation in line 17 ( Finding 3: Consider adding concrete examples to distinguish UI-coupled vs business logic services (🎨) Finding 4: Typo - "adhoc" should be "ad hoc" ( Finding 5: Migration strategy could be more detailed, similar to ADR 0027's implementation plan (💭) PR metadataThe PR title and description are clear and appropriate for a documentation change. The objective clearly explains that this adds an ADR about preferring signals over RxJS in Angular components. What was done wellThe ADR effectively articulates the rationale for adopting signals, provides clear API migration paths, and thoughtfully addresses portability concerns for non-Angular environments. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
|
||
| ## Context and Problem Statement | ||
|
|
||
| Angular has adopted a new reactive primitive, signals. Signals have various improvements over RxJS: |
There was a problem hiding this comment.
💭 The claim about "performance" as an improvement needs clarification or evidence. While signals can optimize change detection, the performance benefit depends heavily on usage patterns. Consider either:
- Adding a reference/link to Angular docs or benchmarks that demonstrate this
- Softening the claim (e.g., "potential performance improvements in certain scenarios")
- Providing specific context about what performance aspects improve
| Angular has adopted a new reactive primitive, signals. Signals have various improvements over RxJS: | ||
| performance, simplicity, and deeper integrations into the rest of the framework. | ||
|
|
||
| RxJS will become an optional dependency of Angular. Certain asynchronous workflows will still |
There was a problem hiding this comment.
| RxJS will become an optional dependency of Angular. Certain asynchronous workflows will still | |
| benefit from RxJS—signals are synchronous. Furthermore, being a part of the core Angular library, |
or
| RxJS will become an optional dependency of Angular. Certain asynchronous workflows will still | |
| benefit from RxJS. Signals are synchronous. Furthermore, being a part of the core Angular library, |
| As such, Signals should be the default when operating _in the view layer_: components, directives, | ||
| pipes, and services that are tightly coupled to the UI/Angular. Services that primarily deal with | ||
| business logic should prefer RxJS to maximize portability (or, even better, be moved to the Rust | ||
| SDK). |
There was a problem hiding this comment.
🎨 The guidance here is good but could benefit from examples of "services tightly coupled to the UI/Angular" vs "services with business logic." This would help developers make the right choice when implementing. Consider adding a brief example or two in a <details> block.
|
|
||
| - Disallow usage of signals and only use RxJS for reactivity. | ||
| - This is a non-starter. Signals are being built into Angular. | ||
| - Continue the status quo of adhoc usage. |
There was a problem hiding this comment.
| - Continue the status quo of adhoc usage. | |
| - Continue the status quo of ad hoc usage. |
| Services tightly coupled to Angular should use signals. Services with business logic should prefer | ||
| RxJS for portability. Use `toSignal()` and `toObservable()` to bridge between the two when needed. | ||
|
|
||
| Existing code will be migrated gradually. New code must use signal-based APIs. |
There was a problem hiding this comment.
💭 Consider adding guidance on the migration strategy. Should this happen incrementally per feature area? Are there any teams or components that should be prioritized? ADR 0027 had a detailed "Implementation Plan" section which was helpful.
There was a problem hiding this comment.
🎨 I agree that this ADR needs an Implementation Plan section. I do not think that it needs to be overwhelming and could even point to some of the specific sections in the Angular Migrations Wiki
There was a problem hiding this comment.
My suggested approach would be for all teams to run the signal related migrations for all code they own, ideally feature by feature. But if we define a plan we also need to put weight behind it, we have plenty of existing ADRs with implementation plans that are pending.
There was a problem hiding this comment.
IMO, yes, the primary intent of a plan in an ADR is the steps by which each team will go through to accomplish the goal. We cannot set the timing of when the plan is done, but it does help to influence the timing when we can approach stakeholders showing a prescriptive plan on how to do the work.
|
I like the Claude feedback as of this writing so let's continue improving that. There's also https://contributing.bitwarden.com/contributing/code-style/web/angular/#reactivity-adr-0003--adr-0028 which can get a formal link. |
|
|
||
| As such, Signals should be the default when operating _in the view layer_: components, directives, | ||
| pipes, and services that are tightly coupled to the UI/Angular. Services that primarily deal with | ||
| business logic should prefer RxJS to maximize portability (or, even better, be moved to the Rust |
There was a problem hiding this comment.
🎨 I am good with the comment that suggests the usage of the Rust SDK.
That being said, I think it is important that an ADR be written to clearly describe that process. I do not currently see one published to our ADR Wiki.
I think it important for a few reasons:
A.) The codifying of the decision and migration plan to the Rust SDK aligns perfectly with our ADR mission statement
B.) Crafting it leaves behind a clear trail for teammates new and old to follow; minimized tribal knowledge around the migration
C.) Claude Code (or other agents) will hopefully be able to leverage the ADR to make suggestions to teammates working in the clients repo to move said business logic to the Rust SDK.
| - `@Output()` → `output()` | ||
| - `@ViewChild`/`@ContentChild` → `viewChild()`/`contentChild()` | ||
|
|
||
| Services tightly coupled to Angular should use signals. Services with business logic should prefer |
There was a problem hiding this comment.
I think it's worth calling out it's non presentational logic.
| Services tightly coupled to Angular should use signals. Services with business logic should prefer | |
| Services tightly coupled to Angular should use signals. Services with non-presentational business logic should prefer |
| Services tightly coupled to Angular should use signals. Services with business logic should prefer | ||
| RxJS for portability. Use `toSignal()` and `toObservable()` to bridge between the two when needed. | ||
|
|
||
| Existing code will be migrated gradually. New code must use signal-based APIs. |
There was a problem hiding this comment.
My suggested approach would be for all teams to run the signal related migrations for all code they own, ideally feature by feature. But if we define a plan we also need to put weight behind it, we have plenty of existing ADRs with implementation plans that are pending.
|
Can we add a admonitions to https://contributing.bitwarden.com/architecture/adr/observable-data-services linking to this ADR? I think it slightly supersedes the existing one. |
|
ADR 28 has just been taken by the way: https://contributing.bitwarden.com/architecture/adr/adopt-fusion-cache |
🎟️ Tracking
https://bitwarden.atlassian.net/browse/CL-875
📔 Objective
Adds an ADR that describes our preference for signals over RxJS in Angular components.
⏰ Reminders before review
team
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmedissue and could potentially benefit from discussion
:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes