Skip to content

test(forms): add repro specs for compat parse errors - #71628

Closed
Flarette wants to merge 1 commit into
angular:mainfrom
Flarette:triage/forms-compat-parse-errors
Closed

Flarette wants to merge 1 commit into
angular:mainfrom
Flarette:triage/forms-compat-parse-errors

Conversation

@Flarette

@Flarette Flarette commented Oct 26, 2026 •

Copy link
Copy Markdown
Contributor

Issue #71633 | Triage notes | Repro specs

Why the change

A compatForm() field drops the parse errors its controls report, so bad input leaves the field valid and submit() runs with the last good value; this draft adds a failing repro spec and asks you to settle how compat validity should work before I write the fix.

Special things to note

  • This is a triage draft with no fix. The repro spec fails on purpose, so CI is red.
  • The fix makes a compat field report invalid while its FormControl still reports valid. That
    split is the main decision below.
  • A related bug, FormControl errors missing from errorSummary(), has its own triage PR:
    test(forms): add repro specs for compat errorSummary #71630. Both fixes change CompatValidationState, so whichever lands second needs a rebase.

Change outline

What the branch adds:

packages/forms/signals/
├── test/web/compat_parse_errors.spec.ts   # 1 failing repro, 1 passing control spec
└── triage/compat-parse-errors.md          # observed output, root cause, history

Where a parse error goes, for <input type="number" [formField]="f.age"> with 5e typed:

 FormField.parseErrors()              → [{kind: 'parse'}]
 CompatValidationState
-  parseErrors                        → []  (hardcoded)
-  errors, valid, invalid, syncValid  → the FormControl only
+  parseErrors                        → the parse errors of every binding
+  errors                             → parse errors, then the FormControl's errors
+  valid, syncValid                   → the FormControl's value, and no parse errors
+  invalid                            → the FormControl's value, or any parse error
 FormField.errors()                   (filters the field's errors())
-  → []
+  → [{kind: 'parse'}]
 submit(f, action)
-  runs the action; the model still holds 5
+  doesn't run the action

Recommended fix: merge binding parse errors in the compat layer

Do what FieldValidationState does (compat/src/compat_validation_state.ts):

-  readonly errors: Signal<CompatValidationError[]>;
+  readonly errors: Signal<ValidationError.WithFieldTree[]>;

-  readonly parseErrors: Signal<ValidationError.WithFormField[]> = computed(() => []);
+  readonly parseErrors: Signal<ValidationError.WithFormField[]> = computed(
+    () => this.node.formFieldBindings().flatMap((binding) => binding.parseErrors()),
+    {equal: shallowArrayEquals},
+  );

   constructor(private readonly node: CompatFieldNode, options: CompatFieldNodeOptions) {
-    this.syncValid = getControlStatusSignal(options, (c) => c.status === 'VALID');
-    this.errors = getControlStatusSignal(options, extractNestedReactiveErrors);
-    this.valid = getControlStatusSignal(options, (c) => c.valid);
-    this.invalid = getControlStatusSignal(options, (c) => c.invalid);
+    const controlSyncValid = getControlStatusSignal(options, (c) => c.status === 'VALID');
+    const controlErrors = getControlStatusSignal(options, extractNestedReactiveErrors);
+    const controlValid = getControlStatusSignal(options, (c) => c.valid);
+    const controlInvalid = getControlStatusSignal(options, (c) => c.invalid);
+    const hasParseErrors = computed(() => this.parseErrors().length > 0);
+
+    this.errors = computed(() => [...this.parseErrors(), ...controlErrors()], {
+      equal: shallowArrayEquals,
+    });
+    this.syncValid = computed(() => controlSyncValid() && !hasParseErrors());
+    this.valid = computed(() => controlValid() && !hasParseErrors());
+    this.invalid = computed(() => controlInvalid() || hasParseErrors());
     this.pending = getControlStatusSignal(options, (c) => c.pending);
   }

status already derives from errors() and pending(). That covers both submit paths: a compat
root reads its own invalid and valid, and a plain root reads each compat child's status().

Alternatives considered

  • Write parse errors into the FormControl with setErrors(). The next
    updateValueAndValidity() wipes them, and it fights reactive forms' ownership of the control.
    Not recommended.
  • Only document the limitation. This is the cheapest option, but it leaves a silent submit with
    the wrong value.

Design decisions

  1. Signal-layer validity and FormControl validity disagree. After the fix,
    f.age().invalid() is true while FormControl.valid and its parent FormGroup.valid stay
    true. Code that gates on the reactive status disagrees with submit(). The migration guide says
    "Reactive state is proxied automatically" (migration.md:46), so the guide needs a sentence on
    this. Today compat is worse than both APIs it bridges: reactive forms' NumberValueAccessor
    writes null for bad input, so Validators.required catches it, and plain signal forms block the
    submit.
  2. Error types in errors(). The guide says errors() returns CompatValidationError
    (migration.md:48). After the fix, parse errors appear too, and they have no control or
    context. The public type doesn't change, because compatForm() returns a plain FieldTree.
    The errorMessage() helper the guide added on main already handles this: it checks
    instanceof CompatValidationError and falls back to error.message ?? error.kind.
  3. Order. Parse errors come first, as in FieldValidationState.

Questions for maintainers

  1. Is it acceptable for a compat field to report invalid while its FormControl reports valid,
    with a sentence in the migration guide?
  2. Should parse errors come before the FormControl's errors in errors(), as they do on plain
    fields?

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.dev application / infrastructure changes
  • Other... Please describe: triage. Failing repro specs and triage notes, with no library changes.

What is the current behavior?

Issue Number: #71633

What is the new behavior?

No behavior changes yet. This PR adds failing repro specs and triage notes. The fix will replace the
notes on this branch once the questions above are answered.

Does this PR introduce a breaking change?

  • Yes
  • No

A `compatForm()` field drops parse errors from its bindings, so a
number input with bad input leaves the field valid and `submit()` runs
with the last good value.

These specs assert the expected behavior and fail on purpose. The
triage notes explain the root cause. A fix will replace the notes once
maintainers settle the design questions.
@JeanMeche

Copy link
Copy Markdown
Member

Going to close the PRs as they're won't me merged as is. Let's move the discussions to the issues.

@JeanMeche JeanMeche closed this Oct 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants