Skip to content

Commit 2061fd8

Browse files
JeanMechethePunderWoman
JeanMeche
authored and
thePunderWoman
committed
fix(forms): Untrack setValue in reactive forms
Reasonably, writing to an `AbstractControl` shouldn't register signal reads. fixes #67073
1 parent fb16677 commit 2061fd8

File tree

6 files changed

+442
-28
lines changed

6 files changed

+442
-28
lines changed

‎packages/forms/src/model/form_array.ts‎

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@
66
* found in the LICENSE file at https://angular.dev/license
77
*/
88

9-
import {ɵWritable as Writable} from '@angular/core';
9+
import {untracked, ɵWritable as Writable} from '@angular/core';
1010

1111
import {AsyncValidatorFn, ValidatorFn} from '../directives/validators';
1212

@@ -321,12 +321,14 @@ export class FormArray<TControl extends AbstractControl<any> = any> extends Abst
321321
emitEvent?: boolean;
322322
} = {},
323323
): void {
324-
assertAllValuesPresent(this, false, value);
325-
value.forEach((newValue: any, index: number) => {
326-
assertControlPresent(this, false, index);
327-
this.at(index).setValue(newValue, {onlySelf: true, emitEvent: options.emitEvent});
324+
untracked(() => {
325+
assertAllValuesPresent(this, false, value);
326+
value.forEach((newValue: any, index: number) => {
327+
assertControlPresent(this, false, index);
328+
this.at(index).setValue(newValue, {onlySelf: true, emitEvent: options.emitEvent});
329+
});
330+
this.updateValueAndValidity(options);
328331
});
329-
this.updateValueAndValidity(options);
330332
}
331333

332334
/**

‎packages/forms/src/model/form_control.ts‎

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@
66
* found in the LICENSE file at https://angular.dev/license
77
*/
88

9-
import {ɵWritable as Writable} from '@angular/core';
9+
import {untracked, ɵWritable as Writable} from '@angular/core';
1010

1111
import {AsyncValidatorFn, ValidatorFn} from '../directives/validators';
1212
import {removeListItem} from '../util';
@@ -505,13 +505,15 @@ export const FormControl: ɵFormControlCtor = class FormControl<TValue = any>
505505
emitViewToModelChange?: boolean;
506506
} = {},
507507
): void {
508-
(this as Writable<this>).value = this._pendingValue = value;
509-
if (this._onChange.length && options.emitModelToViewChange !== false) {
510-
this._onChange.forEach((changeFn) =>
511-
changeFn(this.value, options.emitViewToModelChange !== false),
512-
);
513-
}
514-
this.updateValueAndValidity(options);
508+
untracked(() => {
509+
(this as Writable<this>).value = this._pendingValue = value;
510+
if (this._onChange.length && options.emitModelToViewChange !== false) {
511+
this._onChange.forEach((changeFn) =>
512+
changeFn(this.value, options.emitViewToModelChange !== false),
513+
);
514+
}
515+
this.updateValueAndValidity(options);
516+
});
515517
}
516518

517519
override patchValue(

‎packages/forms/src/model/form_group.ts‎

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@
66
* found in the LICENSE file at https://angular.dev/license
77
*/
88

9-
import {ɵWritable as Writable} from '@angular/core';
9+
import {untracked, ɵWritable as Writable} from '@angular/core';
1010

1111
import {AsyncValidatorFn, ValidatorFn} from '../directives/validators';
1212

@@ -430,15 +430,17 @@ export class FormGroup<
430430
emitEvent?: boolean;
431431
} = {},
432432
): void {
433-
assertAllValuesPresent(this, true, value);
434-
(Object.keys(value) as Array<keyof TControl>).forEach((name) => {
435-
assertControlPresent(this, true, name as any);
436-
(this.controls as any)[name].setValue((value as any)[name], {
437-
onlySelf: true,
438-
emitEvent: options.emitEvent,
433+
untracked(() => {
434+
assertAllValuesPresent(this, true, value);
435+
(Object.keys(value) as Array<keyof TControl>).forEach((name) => {
436+
assertControlPresent(this, true, name as any);
437+
(this.controls as any)[name].setValue((value as any)[name], {
438+
onlySelf: true,
439+
emitEvent: options.emitEvent,
440+
});
439441
});
442+
this.updateValueAndValidity(options);
440443
});
441-
this.updateValueAndValidity(options);
442444
}
443445

444446
/**

‎packages/forms/test/form_array_spec.ts‎

Lines changed: 134 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,18 +6,23 @@
66
* found in the LICENSE file at https://angular.dev/license
77
*/
88

9+
import {Component, Directive, effect, forwardRef, signal} from '@angular/core';
10+
import {TestBed} from '@angular/core/testing';
11+
import {of} from 'rxjs';
912
import {
1013
AbstractControl,
14+
ControlValueAccessor,
1115
FormArray,
1216
FormControl,
1317
FormGroup,
18+
NG_VALUE_ACCESSOR,
19+
ReactiveFormsModule,
1420
ValidationErrors,
1521
ValidatorFn,
1622
} from '../index';
1723
import {Validators} from '../src/validators';
18-
import {of} from 'rxjs';
1924

20-
import {useAutoTick, timeout} from '@angular/private/testing';
25+
import {timeout, useAutoTick} from '@angular/private/testing';
2126
import {asyncValidator} from './util';
2227

2328
(function () {
@@ -1626,5 +1631,132 @@ import {asyncValidator} from './util';
16261631
});
16271632
});
16281633
});
1634+
1635+
describe('FormArray.setValue is untracked', () => {
1636+
@Directive({
1637+
selector: '[testCva]',
1638+
providers: [
1639+
{
1640+
provide: NG_VALUE_ACCESSOR,
1641+
useExisting: forwardRef(() => TestCvaDirective),
1642+
multi: true,
1643+
},
1644+
],
1645+
})
1646+
class TestCvaDirective implements ControlValueAccessor {
1647+
// This is the “dangerous” signal read that must NOT get tracked by the caller of setValue().
1648+
static cvaSignal = signal(0);
1649+
1650+
writeValue(value: unknown): void {
1651+
// If setValue() is not untracked, the *caller* effect/computed may accidentally track this read.
1652+
TestCvaDirective.cvaSignal();
1653+
}
1654+
1655+
registerOnChange(_: (value: unknown) => void): void {}
1656+
registerOnTouched(_: () => void): void {}
1657+
setDisabledState(_: boolean): void {}
1658+
}
1659+
1660+
@Component({
1661+
imports: [ReactiveFormsModule, TestCvaDirective],
1662+
template: `<input testCva [formControl]="control.at(0)" />`,
1663+
})
1664+
class HostComponent {
1665+
control = new FormArray([new FormControl('')]);
1666+
}
1667+
1668+
it('should NOT track signals read inside CVA.writeValue when setValue is called inside an effect', async () => {
1669+
const fixture = TestBed.createComponent(HostComponent);
1670+
fixture.detectChanges(); // wires up FormControlDirective + CVA
1671+
1672+
const driver = signal('A');
1673+
let runs = 0;
1674+
1675+
// Create the effect inside the Angular injection context.
1676+
TestBed.runInInjectionContext(() => {
1677+
effect(() => {
1678+
runs++;
1679+
1680+
// Only dependency we *want* is `driver()`.
1681+
fixture.componentInstance.control.setValue([driver()]);
1682+
});
1683+
});
1684+
1685+
await fixture.whenStable();
1686+
expect(runs).toBe(1);
1687+
1688+
// Changing the CVA signal should NOT re-run the effect.
1689+
TestCvaDirective.cvaSignal.set(1);
1690+
await fixture.whenStable();
1691+
expect(runs).toBe(1);
1692+
1693+
// Changing the driver signal SHOULD re-run the effect.
1694+
driver.set('B');
1695+
await fixture.whenStable();
1696+
expect(runs).toBe(2);
1697+
});
1698+
1699+
it('should NOT track signals read inside CVA.writeValue when patchValue is called inside an effect', async () => {
1700+
const fixture = TestBed.createComponent(HostComponent);
1701+
fixture.detectChanges(); // wires up FormControlDirective + CVA
1702+
1703+
const driver = signal('A');
1704+
let runs = 0;
1705+
1706+
// Create the effect inside the Angular injection context.
1707+
TestBed.runInInjectionContext(() => {
1708+
effect(() => {
1709+
runs++;
1710+
1711+
// Only dependency we *want* is `driver()`.
1712+
fixture.componentInstance.control.patchValue([driver()]);
1713+
});
1714+
});
1715+
1716+
await fixture.whenStable();
1717+
expect(runs).toBe(1);
1718+
1719+
// Changing the CVA signal should NOT re-run the effect.
1720+
TestCvaDirective.cvaSignal.set(1);
1721+
await fixture.whenStable();
1722+
expect(runs).toBe(1);
1723+
1724+
// Changing the driver signal SHOULD re-run the effect.
1725+
driver.set('B');
1726+
await fixture.whenStable();
1727+
expect(runs).toBe(2);
1728+
});
1729+
1730+
it('should NOT track signals read inside CVA.writeValue when reset is called inside an effect', async () => {
1731+
const fixture = TestBed.createComponent(HostComponent);
1732+
fixture.detectChanges(); // wires up FormControlDirective + CVA
1733+
1734+
const driver = signal('A');
1735+
let runs = 0;
1736+
1737+
// Create the effect inside the Angular injection context.
1738+
TestBed.runInInjectionContext(() => {
1739+
effect(() => {
1740+
runs++;
1741+
1742+
// Only dependency we *want* is `driver()`.
1743+
fixture.componentInstance.control.reset([driver()]);
1744+
});
1745+
});
1746+
1747+
await fixture.whenStable();
1748+
expect(runs).toBe(1);
1749+
1750+
// Changing the CVA signal should NOT re-run the effect.
1751+
TestCvaDirective.cvaSignal.set(1);
1752+
await fixture.whenStable();
1753+
expect(runs).toBe(1);
1754+
1755+
// Changing the driver signal SHOULD re-run the effect.
1756+
driver.set('B');
1757+
await fixture.whenStable();
1758+
expect(runs).toBe(2);
1759+
});
1760+
});
16291761
});
16301762
})();

0 commit comments

Comments
 (0)