-
Notifications
You must be signed in to change notification settings - Fork 21
0028 - Adopt @ngrx/signals for Component State Stores #632
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?
Changes from 5 commits
da0f44b
e7e876f
fe6f2b2
e24abd2
1f4753d
ce35d0c
8bb9e0f
25e7090
fb8c3fe
004bce1
527639b
4b42a90
58effff
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,10 @@ | ||||||
| --- | ||||||
| adr: "0027" | ||||||
| status: "Draft" | ||||||
|
||||||
| status: "Draft" | |
| status: "Proposed" |
Outdated
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.
❓ What's happening with this whole file? Not understanding. You can have one proposed ADR as 27 most likely.
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.
It was a placeholder for the 0027 - Angular signals ADR. Now that there is an active pull request, #638, I can remove the file
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,110 @@ | ||||||
| --- | ||||||
| adr: "0028" | ||||||
| status: "Proposed" | ||||||
| date: 2025-07-08 | ||||||
| tags: [client] | ||||||
| --- | ||||||
|
|
||||||
| # 0028 - Adopt `@ngrx/signals` for Component State Stores | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎨 Can your code reference be simplified somehow? Maybe just say "Angular Signals"? A lot of title usages can't render the preformatted text, but you can use the code in paragraphs. |
||||||
|
|
||||||
| <AdrTable frontMatter={frontMatter}></AdrTable> | ||||||
|
|
||||||
| ## Prior Reading | ||||||
|
|
||||||
| - [ADR Draft - Adopt Angular Signals for Component State](https://bitwarden.atlassian.net/wiki/spaces/EN/pages/1538326529) | ||||||
|
||||||
| - [Angular Signals](https://angular.dev/guide/signals) | ||||||
| - [NgRx Docs](https://ngrx.io/guide/signals) | ||||||
|
|
||||||
| ## Context and Problem Statement | ||||||
|
|
||||||
| Building on | ||||||
| [ADR Draft - Adopt Angular Signals for Component State](https://bitwarden.atlassian.net/wiki/spaces/EN/pages/1538326529) | ||||||
|
||||||
| decision to adopt Angular signals for component state, this ADR addresses state management patterns | ||||||
| across our application on the component level. | ||||||
|
|
||||||
| The DIRT team frequently interacts with multiple services on the domain layer and exposes this data | ||||||
| to various components. Current implementation shows duplicate patterns for managing service data, | ||||||
| transforming it for UI consumption, and sharing it between components. | ||||||
|
|
||||||
| ### Problem | ||||||
|
|
||||||
| We need a standardized approach to manage cross-component state that leverages Angular signals while | ||||||
| reducing code duplication and improving developer experience. | ||||||
|
|
||||||
| ### Context | ||||||
|
|
||||||
| @ngrx/signals provides a signal store that captures these repeating patterns and centralizes state | ||||||
| management in a way that complements our existing signal adoption | ||||||
|
|
||||||
| ## Considered Options | ||||||
|
|
||||||
| - **Continue with Angular Signals only** | ||||||
| - Maintain current approach using signals within individual components | ||||||
| - Does not provide the state management features and cross-component patterns that @ngrx/signals | ||||||
| offers through the signal store | ||||||
| - **Alternative state management solutions** | ||||||
| - Explore other state management libraries | ||||||
| - Existing options like the original NgRx store involve significant boilerplate, don’t leverage | ||||||
| signals, and lack team support | ||||||
| - **Adopt @ngrx/signals package** | ||||||
| - Implement the signal store to capture repeating patterns and manage state across components | ||||||
| - Builds upon our existing signal adoption | ||||||
|
|
||||||
| ## Decision Outcome | ||||||
|
|
||||||
| **Adopt @ngrx/signals for state management on the component level** | ||||||
|
||||||
| **Adopt @ngrx/signals for state management on the component level** | |
| Chosen option: **Adopt @ngrx/signals for state management on the component level.** |
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.
If we're not storing decrypted data. What would we be using it for? Purely concerns like "is drawer open"?
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.
Good question! We should still store the encrypted data in the stores. There are a few methods we can use to expose the decrypted data while it's in flight. When we start the migrations, we should and plan to explore the different methods to determine which best suites our security policies. This statement is to cover the overall idea that encrypted data is encrypted at rest. The main two methods to explore are using signal store computed signals, could be considered in flight but these are still exposed from the store, or expose a feature to signal stores that provides the functions to encrypt and decrypt within the component itself. The main difference is where we want the responsibility of encrypting and decrypting to lie architecturally.
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.
❓ Did lint need this? Your only usage is in a link which wasn't checked I thought.
We keep this alphabetically sorted by the way.
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.
I did, it's specifically the cspell check that causes the failure. Thanks for catching that it wasn't sorted!