Skip to content

fix(forms): prevent duplicate events and staleness on parseErrors updates #71132

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.

Sign up for GitHub

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

Open
JeanMeche wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(forms): prevent duplicate events and staleness on parseErrors updates #71132

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
174 changes: 174 additions & 0 deletions packages/forms/signals/test/web/reactive_fvc.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -429,6 +429,132 @@ describe('error bindings', () => {
expect(fixture.componentInstance.ctrl.errors).toBeNull();
});

it('should not emit duplicate valueChanges when parseErrors change alongside value', () => {
@Component({
selector: 'my-parsing-input',
template: '<input #i [value]="value()" (input)="value.set(i.value)" />',
})
class MyParsingInput {
readonly value = model('');
constructor() {
const parseErrorsToken = inject(ɵFORM_CONTROL_INTEGRATION, {optional: true, self: true});
if (parseErrorsToken) {
parseErrorsToken.setParseErrors(
computed(() => {
return this.value() === 'INVALID' ? [{kind: 'parse', reason: 'cannot parse'}] : [];
}),
);
}
}
}

@Component({
template: `<my-parsing-input [formControl]="ctrl" />`,
imports: [MyParsingInput, ReactiveFormsModule],
})
class TestCmp {
ctrl = new FormControl('');
}

const fixture = act(() => TestBed.createComponent(TestCmp));
const fvc = fixture.debugElement.query(By.directive(MyParsingInput)).componentInstance;

let valueChangesCount = 0;
fixture.componentInstance.ctrl.valueChanges.subscribe(() => valueChangesCount++);

// Set value that triggers a parse error
// This updates the value AND updates the parseErrors
act(() => fvc.value.set('INVALID'));

// Value should only emit once, even though parseErrors also changed
expect(valueChangesCount).toBe(1);
});

it('should emit statusChanges with the correct status when parseErrors change alongside value', () => {
@Component({
selector: 'my-parsing-input',
template: '<input #i [value]="value()" (input)="value.set(i.value)" />',
})
class MyParsingInput {
readonly value = model('');
constructor() {
const parseErrorsToken = inject(ɵFORM_CONTROL_INTEGRATION, {optional: true, self: true});
if (parseErrorsToken) {
parseErrorsToken.setParseErrors(
computed(() => {
return this.value() === 'INVALID' ? [{kind: 'parse', reason: 'cannot parse'}] : [];
}),
);
}
}
}

@Component({
template: `<my-parsing-input [formControl]="ctrl" />`,
imports: [MyParsingInput, ReactiveFormsModule],
})
class TestCmp {
ctrl = new FormControl('');
}

const fixture = act(() => TestBed.createComponent(TestCmp));
const fvc = fixture.debugElement.query(By.directive(MyParsingInput)).componentInstance;

const emittedStatuses: string[] = [];
fixture.componentInstance.ctrl.statusChanges.subscribe((s) => emittedStatuses.push(s));

act(() => fvc.value.set('INVALID'));

// The emitted status should be INVALID.
// In the original code (with a stale validator), this would have incorrectly emitted 'VALID'.
expect(emittedStatuses).toEqual(['INVALID']);
});

it('should not emit valueChanges when parseErrors update independently of value', () => {
@Component({
selector: 'my-parsing-input',
template: '<input #i [value]="value()" (input)="value.set(i.value)" />',
})
class MyParsingInput {
readonly value = model('');
readonly forceError = signal(false);
constructor() {
const parseErrorsToken = inject(ɵFORM_CONTROL_INTEGRATION, {optional: true, self: true});
if (parseErrorsToken) {
parseErrorsToken.setParseErrors(
computed(() => {
if (this.forceError()) return [{kind: 'parse', reason: 'forced error'}];
return this.value() === 'INVALID' ? [{kind: 'parse', reason: 'cannot parse'}] : [];
}),
);
}
}
}

@Component({
template: `<my-parsing-input [formControl]="ctrl" />`,
imports: [MyParsingInput, ReactiveFormsModule],
})
class TestCmp {
ctrl = new FormControl('');
}

const fixture = act(() => TestBed.createComponent(TestCmp));
const fvc = fixture.debugElement.query(By.directive(MyParsingInput)).componentInstance;

let valueChangesCount = 0;
fixture.componentInstance.ctrl.valueChanges.subscribe(() => valueChangesCount++);

// Force a parse error without changing the value
act(() => fvc.forceError.set(true));

expect(fixture.componentInstance.ctrl.errors).toEqual({
parse: {kind: 'parse', reason: 'forced error'},
});
// NO value emission should happen because only the validation state changed
expect(valueChangesCount).toBe(0);
});

it('should merge parseErrors with validator errors', () => {
@Component({
selector: 'my-parsing-input',
Expand Down Expand Up @@ -475,6 +601,54 @@ describe('error bindings', () => {
expect(fixture.componentInstance.ctrl.errors).toBeNull();
});

it('should immediately pass parse error to control errors input (issue #71127)', () => {
@Component({
selector: 'my-parsing-input-71127',
template: '<input #i [value]="value()" (input)="value.set(i.value)" />',
})
class MyParsingInput {
readonly value = model('');
readonly errors = input<any>(null);

constructor() {
const parseErrorsToken = inject(ɵFORM_CONTROL_INTEGRATION, {optional: true, self: true});
if (parseErrorsToken) {
parseErrorsToken.setParseErrors(
computed(() => {
return this.value() === 'INVALID' ? [{kind: 'parse', reason: 'cannot parse'}] : [];
}),
);
}
}
}

@Component({
template: `
<my-parsing-input-71127 [formControl]="ctrl" #myInput />
<p>{{ myInput.errors()?.length ? 'HAS_ERRORS' : 'NO_ERRORS' }}</p>
`,
imports: [MyParsingInput, ReactiveFormsModule],
})
class TestCmp {
ctrl = new FormControl('');
}

const fixture = act(() => TestBed.createComponent(TestCmp));
const fvc = fixture.debugElement.query(By.directive(MyParsingInput)).componentInstance;

// Initially no errors
expect(fixture.componentInstance.ctrl.errors).toBeNull();
let text = fixture.nativeElement.querySelector('p').textContent;
expect(text).toContain('NO_ERRORS');

// Set value that triggers a parse error
act(() => fvc.value.set('INVALID'));
fixture.detectChanges();

text = fixture.nativeElement.querySelector('p').textContent;
expect(text).toContain('HAS_ERRORS'); // Should immediately update the rendered error count
});

it('should switch parseErrors to new FormControl when swapped', () => {
@Component({
selector: 'my-parsing-input',
Expand Down
25 changes: 16 additions & 9 deletions packages/forms/src/directives/ng_control.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,14 @@
import {
ChangeDetectorRef,
DestroyRef,
Provider,
Signal,
computed,
effect,
type Injector,
type Renderer2,
Signal,
inject,
type ɵControlDirectiveHost as ControlDirectiveHost,
Provider,
type Injector,
type Renderer2,
} from '@angular/core';
import {Subscription} from 'rxjs';

Expand All @@ -28,8 +28,8 @@ import {ControlContainer} from './control_container';
import {ControlValueAccessor} from './control_value_accessor';
import {isNativeFormElement, setNativeDomProperty, type NativeFormControl} from './native';
import {ReactiveValidationError} from './reactive_validation_error';
import {ɵFORM_CONTROL_INTEGRATION as FORM_CONTROL_INTEGRATION, selectValueAccessor} from './shared';
import {RequiredValidator, ValidationErrors, ValidatorFn} from './validators';
import {selectValueAccessor, ɵFORM_CONTROL_INTEGRATION as FORM_CONTROL_INTEGRATION} from './shared';

type ParseError = {readonly kind: string};

Expand Down Expand Up @@ -348,7 +348,7 @@ export abstract class NgControl extends AbstractControlDirective {
return;
}

let convertedErrors: ValidationErrors | null = null;
let lastSeenErrors: ValidationErrors | null = null;
const convertedParseErrors = computed(() => {
const rawErrors = parseErrors();
if (rawErrors.length === 0) {
Expand All @@ -365,13 +365,20 @@ export abstract class NgControl extends AbstractControlDirective {
});

// Create validator that returns current parse errors
this.parseErrorsValidator = (() => convertedErrors).bind(this);
this.parseErrorsValidator = () => {
lastSeenErrors = convertedParseErrors();
return lastSeenErrors;
};

// Setup effect to watch parseErrors and trigger revalidation
effect(
() => {
convertedErrors = convertedParseErrors();
this.control?.updateValueAndValidity({emitEvent: false});
const currentErrors = convertedParseErrors();
if (currentErrors !== lastSeenErrors) {
lastSeenErrors = currentErrors;
this.control?.updateValueAndValidity({emitEvent: false});
this.injector?.get(ChangeDetectorRef)?.markForCheck();
}
},
{injector: this.injector},
);
Expand Down
Loading