Fix/ngsubmit await async validators#68661
Conversation
| if (this.awaitAsyncValidators && this.form.status === 'PENDING') { | ||
| this._pendingSubmitSub?.unsubscribe(); | ||
| this._pendingSubmitSub = this.form.statusChanges | ||
| .pipe( | ||
| filter((status) => status !== 'PENDING'), | ||
| take(1), | ||
| ) | ||
| .subscribe(() => { | ||
| this.ngSubmit.emit($event); | ||
| this.form._events.next(new FormSubmittedEvent(this.control)); | ||
| }); |
There was a problem hiding this comment.
This is generaly not a recommended practice to hide async work in a sync function.
Here you wouldn't handle consecutive submits that would cancel the pending one (if you'd like to cancel it).
There was a problem hiding this comment.
Thanks for the feedback! Refactored to address both concerns:
- Async in sync function: onSubmit() is now fully synchronous — it just pushes to a Subject. The async pipeline lives in the constructor where its nature is explicit.
- Consecutive submits: Replaced manual unsubscribe() with switchMap, which makes the "cancel previous pending submit" behavior idiomatic and explicit.
Let me know if you'd prefer a different approach for the consecutive-submit semantics (e.g. blocking subsequent submits instead of cancelling).
|
I have read the CLA Document and I hereby sign the CLA. |
The commits were co-authored with On the other hand, I see the tests are failing; could you please review them as well |
…sync validation completes Adds an opt-in `awaitAsyncValidators` boolean input to `NgForm` and `FormGroupDirective`. When set to `true`, the submit handler defers `ngSubmit` emission until all async validators have settled (transitioned from PENDING to VALID or INVALID). onSubmit() remains synchronous — it pushes to a Subject. The async pipeline lives in the constructor where its nature is explicit. Consecutive submits are handled via switchMap, which cancels any pending deferred submit when a new one arrives. Fixes angular#31021
manichandra
force-pushed
the
fix/ngsubmit-await-async-validators
branch
from
44d8dcc to
c3b6cfd
Compare
| this.submittedReactive.set(true); | ||
| syncPendingControls(this.form, this._directives); | ||
| this.ngSubmit.emit($event); | ||
| this.form._events.next(new FormSubmittedEvent(this.control)); | ||
|
|
||
| this._pendingSubmit$.next($event); | ||
|
|
||
| // Forms with `method="dialog"` have some special behavior | ||
| // that won't reload the page and that shouldn't be prevented. | ||
| return ($event?.target as HTMLFormElement | null)?.method === 'dialog'; |
There was a problem hiding this comment.
This isn't much better.
We're plugging an async API in a sync API. It will return before the async validators will have run and this is probably not what users would expect.
I don't think there is a trivial evolution here.
There was a problem hiding this comment.
Before pushing another update — the approach I'm considering is removing the constructor-based Subject pipeline entirely and instead handling the deferred emit directly inside onSubmit() with an inline statusChanges subscription (only when awaitAsyncValidators=true and the form is PENDING; default path stays fully synchronous). Does that direction address your concern, or did you have a different approach in mind?
There was a problem hiding this comment.
After further looking at it, I see the issue goes deeper than just where the async work lives — changing ngSubmit semantics makes a synchronous submit API behave asynchronously, which is probably not what users would expect.
I'll hold off on pushing more changes. Would the Angular team prefer this handled as documentation/workaround guidance, or would a separate explicit async submission API be worth discussing? @JeanMeche
There was a problem hiding this comment.
Instead of changing ngSubmit semantics, would a separate explicit event be more appropriate?
For example, ngSubmitValid could leave ngSubmit fully synchronous and emit only after pending async validators settle and the form is VALID. It would not emit if the form resolves to INVALID.
If that API direction is acceptable, I can update the PR with implementation, docs, and integration tests for both reactive and template-driven forms. @JeanMeche @SkyZeroZx
manichandra
force-pushed
the
fix/ngsubmit-await-async-validators
branch
2 times, most recently
from
14225af to
c3b6cfd
Compare
…nt:false
When addControl sets up async validators via updateValueAndValidity({emitEvent:false}),
the validator subscription captures emitEvent:false in its closure. This meant
statusChanges would not emit when the validator resolved, so the ngSubmit
deferral never fired.
Fix: before subscribing to statusChanges in the pending-submit pipeline,
call _updateTreeValidity({emitEvent:true}) to restart validators with
emitEvent:true so statusChanges properly emits when validation completes.
Also add take(1) to test validator observables so subscriptions complete
cleanly after each emission, preventing stale subscriptions on re-validation.
manichandra
force-pushed
the
fix/ngsubmit-await-async-validators
branch
from
d40a230 to
84e45d2
Compare
3 participants
PR Checklist
https://github.com/angular/angular/blob/main/contributing-docs/commit-message-guidelines.md
PR Type
What is the current behavior?
NgFormandFormGroupDirectiveemitngSubmitsynchronously on form submission even when the form has pending async validators(
status === 'PENDING'). Handlers that checkform.validreceive a false negative because async validation hasn't resolved yet.Issue Number: #31021
What is the new behavior?
Adds an opt-in
awaitAsyncValidatorsboolean input (defaultfalse) to bothNgFormandAbstractFormDirective. When enabled and theform is
PENDINGat submission time,ngSubmitis deferred untilstatusChangesemits a non-PENDINGvalue.Template-driven:
Does this PR introduce a breaking change?