refactor(select): migrate select to signal forms#2310
Conversation
Use signal form controls for select and align the value model with the new API. BREAKING CHANGE: drop NG_VALUE_ACCESSOR from select. Changed select to use Angular signal form models instead of ControlValueAccessor. Update consumers to bind through signal form APIs. BREAKING CHANGE: single-value select now includes undefined. Changed the single-value select model to represent an unselected state explicitly. Update code that assumed the value was always defined. BREAKING CHANGE: multi-value select no longer accepts undefined. Changed the multi-value select model to always use an array. Initialize callers with [] instead of undefined and remove undefined handling from related code.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the select component's selection strategies to use Angular's new signal-based form controls (FormValueControl and ModelSignal) instead of the traditional ControlValueAccessor. It also updates related components, tests, and examples to support nullable types and the new signal APIs. The review feedback highlights a style guide violation where effect() is used to propagate state changes in SiSelectSelectionStrategy, which should be refactored to use computed(). Additionally, a misleading JSDoc comment regarding the default value in SiSelectSingleValueDirective needs to be corrected.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| constructor() { | ||
| effect(() => { | ||
| const arrayValue = this.toArrayValue(this.value()); | ||
| // That way the effect only tracks value(), and the reads/writes of selectedRows/rows inside onValueChange no longer feed back into it. | ||
| untracked(() => this.selectOptions.onValueChange(arrayValue)); | ||
| }); | ||
| } |
There was a problem hiding this comment.
According to the Repository Style Guide (line 38), effect() must not be used for propagation of state changes, as this can lead to ExpressionChangedAfterItHasBeenChecked errors, infinite circular updates, or unnecessary change detection cycles.
Instead of pushing updates from the selection strategy to the options strategy using an effect, the options strategy (SiSelectOptionsStrategy) should reactively compute its selected state by reading the selection strategy's value signal directly (e.g., using computed()).
References
- Do NOT use effect() for propagation of state changes — this leads to ExpressionChangedAfterItHasBeenChecked errors, infinite circular updates, or unnecessary change detection cycles. Use computed() instead to model state that depends on other state (link)
| */ | ||
| override allowMultiple = false; | ||
|
|
||
| /** @defaultValue [] */ |
There was a problem hiding this comment.
The JSDoc comment @defaultValue [] is misleading because the default value of model<T>() is undefined (not an empty array []). Please update the documentation to reflect the correct default value.
| /** @defaultValue [] */ | |
| /** @defaultValue undefined */ |
1 participant
Summary\nUse signal form controls for select and align the value model with the new API.\n\n## Breaking changes\n- Drop NG_VALUE_ACCESSOR from select.\n- Single-value select now includes undefined.\n- Multi-value select no longer accepts undefined.\n\n## Notes\n- The select-specific signal form migration required small follow-up adjustments in nearby consumers and examples.\n- The test suite was updated to reflect the new select behavior.\n\n
Checklist\n- [x] Relevant tests updated\n- [x] Breaking changes documented