Repository navigation
Conversation
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.
4 of 14 tasks
Member
|
Going to close the PRs as they're won't me merged as is. Let's move the discussions to the issues. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 andsubmit()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
invalidwhile itsFormControlstill reports valid. Thatsplit is the main decision below.
FormControlerrors missing fromerrorSummary(), 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:
Where a parse error goes, for
<input type="number" [formField]="f.age">with5etyped: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 actionRecommended fix: merge binding parse errors in the compat layer
Do what
FieldValidationStatedoes (compat/src/compat_validation_state.ts):statusalready derives fromerrors()andpending(). That covers both submit paths: a compatroot reads its own
invalidandvalid, and a plain root reads each compat child'sstatus().Alternatives considered
FormControlwithsetErrors(). The nextupdateValueAndValidity()wipes them, and it fights reactive forms' ownership of the control.Not recommended.
the wrong value.
Design decisions
FormControlvalidity disagree. After the fix,f.age().invalid()istruewhileFormControl.validand its parentFormGroup.validstaytrue. Code that gates on the reactive status disagrees withsubmit(). The migration guide says"Reactive state is proxied automatically" (
migration.md:46), so the guide needs a sentence onthis. Today compat is worse than both APIs it bridges: reactive forms'
NumberValueAccessorwrites
nullfor bad input, soValidators.requiredcatches it, and plain signal forms block thesubmit.
errors(). The guide sayserrors()returnsCompatValidationError(
migration.md:48). After the fix, parse errors appear too, and they have nocontrolorcontext. The public type doesn't change, becausecompatForm()returns a plainFieldTree.The
errorMessage()helper the guide added onmainalready handles this: it checksinstanceof CompatValidationErrorand falls back toerror.message ?? error.kind.FieldValidationState.Questions for maintainers
invalidwhile itsFormControlreports valid,with a sentence in the migration guide?
FormControl's errors inerrors(), as they do on plainfields?
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
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?