Skip to content

Commit fddfa29

Browse files
committed
refactor(forms): address review feedback on select[multiple] prototype
- Remove $any() cast from the test — the TCB already enforces string[] for select[multiple], so no workaround is needed - Extract the CONTROL_BINDING_NAMES update loop into a shared applyControlStateBindings() helper in bindings.ts, used by both the native and select-multiple control paths - Throw a RuntimeError (code 1921) in setSelectMultipleControlValue when a non-array value is passed; the guard is unconditional so production never silently corrupts the selection state - Move the selectedOptions fallback comment to the fallback branch where it actually explains the divergence
1 parent fd2636e commit fddfa29

5 files changed

Lines changed: 49 additions & 23 deletions

File tree

‎packages/forms/signals/src/directive/bindings.ts‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -82,3 +82,21 @@ export function bindingUpdated<TKey extends string>(
8282
}
8383
return false;
8484
}
85+
86+
/**
87+
* Iterates over all control binding names, computes each value from field state, and calls
88+
* `onUpdate` for any binding whose value has changed since the last call. Shared by native
89+
* and select-multiple control implementations to avoid duplicating the update loop.
90+
*/
91+
export function applyControlStateBindings(
92+
bindings: {[key: string]: unknown},
93+
state: ReadonlyFieldState<unknown>,
94+
onUpdate: (name: ControlBindingKey, value: unknown) => void,
95+
): void {
96+
for (const name of CONTROL_BINDING_NAMES) {
97+
const value = readFieldStateBindingValue(state, name);
98+
if (bindingUpdated(bindings, name, value)) {
99+
onUpdate(name, value);
100+
}
101+
}
102+
}

‎packages/forms/signals/src/directive/control_native.ts‎

Lines changed: 6 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -13,10 +13,9 @@ import {
1313
import type {ValidationError} from '../api/rules';
1414
import {createParser} from '../util/parser';
1515
import {
16+
applyControlStateBindings,
1617
bindingUpdated,
17-
CONTROL_BINDING_NAMES,
1818
createBindings,
19-
readFieldStateBindingValue,
2019
type ControlBindingKey,
2120
} from './bindings';
2221
import type {FormField} from './form_field';
@@ -96,15 +95,12 @@ export function nativeControlCreate(
9695
setNativeControlValue(input, controlValue);
9796
}
9897

99-
for (const name of CONTROL_BINDING_NAMES) {
100-
const value = readFieldStateBindingValue(state, name);
101-
if (bindingUpdated(bindings, name, value)) {
102-
host.setInputOnDirectives(name, value);
103-
if (parent.elementAcceptsNativeProperty(name)) {
104-
setNativeDomProperty(parent.renderer, input, name, value as string | number | undefined);
105-
}
98+
applyControlStateBindings(bindings, state, (name, value) => {
99+
host.setInputOnDirectives(name, value);
100+
if (parent.elementAcceptsNativeProperty(name)) {
101+
setNativeDomProperty(parent.renderer, input, name, value as string | number | undefined);
106102
}
107-
}
103+
});
108104

109105
updateMode = true;
110106
};

‎packages/forms/signals/src/directive/control_select_multiple.ts‎

Lines changed: 23 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -5,12 +5,15 @@
55
* Use of this source code is governed by an MIT-style license that can be
66
* found in the LICENSE file at https://angular.dev/license
77
*/
8-
import {type ɵControlDirectiveHost as ControlDirectiveHost} from '@angular/core';
98
import {
9+
ɵRuntimeError as RuntimeError,
10+
type ɵControlDirectiveHost as ControlDirectiveHost,
11+
} from '@angular/core';
12+
import {RuntimeErrorCode} from '../errors';
13+
import {
14+
applyControlStateBindings,
1015
bindingUpdated,
11-
CONTROL_BINDING_NAMES,
1216
createBindings,
13-
readFieldStateBindingValue,
1417
type ControlBindingKey,
1518
} from './bindings';
1619
import type {FormField} from './form_field';
@@ -51,15 +54,12 @@ export function selectMultipleControlCreate(
5154
setSelectMultipleControlValue(select, controlValue);
5255
}
5356

54-
for (const name of CONTROL_BINDING_NAMES) {
55-
const value = readFieldStateBindingValue(state, name);
56-
if (bindingUpdated(bindings, name, value)) {
57-
host.setInputOnDirectives(name, value);
58-
if (parent.elementAcceptsNativeProperty(name)) {
59-
setNativeDomProperty(parent.renderer, select, name, value as string | number | undefined);
60-
}
57+
applyControlStateBindings(bindings, state, (name, value) => {
58+
host.setInputOnDirectives(name, value);
59+
if (parent.elementAcceptsNativeProperty(name)) {
60+
setNativeDomProperty(parent.renderer, select, name, value as string | number | undefined);
6161
}
62-
}
62+
});
6363

6464
updateMode = true;
6565
};
@@ -76,6 +76,9 @@ function getSelectMultipleControlValue(select: HTMLSelectElement): string[] {
7676
return selected;
7777
}
7878

79+
// `selectedOptions` is not available in some environments (e.g. older jsdom versions used
80+
// in Node.js tests). Fall back to iterating the full `options` list and filtering by the
81+
// `selected` flag.
7982
for (let i = 0; i < select.options.length; i++) {
8083
const option = select.options[i];
8184
if (option.selected) {
@@ -87,7 +90,15 @@ function getSelectMultipleControlValue(select: HTMLSelectElement): string[] {
8790
}
8891

8992
function setSelectMultipleControlValue(select: HTMLSelectElement, value: unknown): void {
90-
const selectedValues = new Set(Array.isArray(value) ? value : []);
93+
if (!Array.isArray(value)) {
94+
throw new RuntimeError(
95+
RuntimeErrorCode.SELECT_MULTIPLE_NON_ARRAY_VALUE,
96+
ngDevMode &&
97+
`Expected an array value for select[multiple], but got ${typeof value}. ` +
98+
`Bind a signal holding a string array: e.g. form(signal<string[]>([]))`,
99+
);
100+
}
101+
const selectedValues = new Set<string>(value);
91102
for (let i = 0; i < select.options.length; i++) {
92103
const option = select.options[i];
93104
option.selected = selectedValues.has(option.value);

‎packages/forms/signals/src/errors.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,4 +30,5 @@ export const enum RuntimeErrorCode {
3030
MISSING_SUBMIT_ACTION = 1915,
3131
RENDERED_HIDDEN_FIELD = 1916,
3232
UNSUPPORTED_FEATURE = 1920,
33+
SELECT_MULTIPLE_NON_ARRAY_VALUE = 1921,
3334
}

‎packages/forms/signals/test/web/form_field.spec.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3927,7 +3927,7 @@ describe('field directive', () => {
39273927
@Component({
39283928
imports: [FormField],
39293929
template: `
3930-
<select #select multiple [formField]="$any(f)">
3930+
<select #select multiple [formField]="f">
39313931
<option value="one">One</option>
39323932
<option value="two">Two</option>
39333933
<option value="three">Three</option>

0 commit comments

Comments
 (0)