Repository navigation
fix(forms): safely chain debounced async validation resources - #71414
benjavicente wants to merge 1 commit into
Conversation
Pass debounced params to async validator factories without exposing resource status sentinels. Compose snapshots separately to preserve pending and error states, and delegate reloads after debounce settles. Add regression coverage for effects, custom snapshots, params error recovery, and reloadValidation. Fixes angular#70884
JeanMeche
left a comment
There was a problem hiding this comment.
Thanks for digging into #70884 and putting together the test cases!
Stepping back, there are two levels of concerns with this approach:
-
Architectural layer (
@angular/forms/signalsvs@angular/core):validateAsyncis currently the only consumer ofɵchainin the repo (removing the import here leaveschain as ɵchaininpackages/core/src/core_private_export.tsunused).- At the same time, the underlying gap isn't specific to Signal Forms: anyone composing
debounced()with a non-resource()Resourceimplementation (such as@tanstack/angular-queryor a customresourceFromSnapshots()resource) hits the exact same wall becausechain()relies on thrownResourceParamsStatusmarkers only caught byResourceImpl, while@angular/corehas no snapshot-based chaining primitive.
-
Behavioral & correctness regressions in
debouncedParams+safeChainResources:- Keeping the previous settled value in
debouncedParamswhile debouncing and unmaskingtarget.snapshot()as soon asdebouncedResourcesettles isn't semantically equivalent toɵchain: it introduces a stale-result/onSuccessrace foreffect-driven custom resources, breaksA -> B -> Aretry after transient errors, keeps stale loads/streams active during debounce, and silently dropsreloadValidation()calls made while debouncing.
- Keeping the previous settled value in
Left detailed notes inline on each of these.
| * When debouncing, params keeps its previous value until the debounce completes, while validation | ||
| * remains pending. |
There was a problem hiding this comment.
Two concerns with adding this to @publicApi 22.0 JSDoc:
- Inaccurate on errors: When
opts.paramsthrows,debouncedParamsbecomesundefined, not its previous value. - Premature API contract lock-in: The preceding sentence intentionally treats
paramsas opaque ("The given params should be used as is to create the resource."). Documenting thatparamsretains its stale value during debounce locks an implementation workaround into the v22 public contract and prevents future fixes (e.g., emittingundefinedwhile debouncing or addressing chaining in@angular/core).
| const debouncedParams = computed(() => { | ||
| const snapshot = debouncedResource.snapshot(); | ||
| return snapshot.status === 'error' ? undefined : snapshot.value; | ||
| }); | ||
| const factoryResource = opts.factory(debouncedParams); | ||
| return safeChainResources(debouncedResource, factoryResource); |
There was a problem hiding this comment.
Keeping the previous settled snapshot.value in debouncedParams while debouncedResource is 'loading' introduces three behavioral issues:
-
Stale snapshot window &
onSuccess(oldResult, newCtx)mismatch foreffect-driven resources:
Indebounce.ts,state.set({status: 'resolved', value})runs inside a Promise.then()microtask. At that instant,debouncedResource.snapshot()becomes'resolved'andsafeChainResourcesimmediately unmaskstarget.snapshot()—before Angular'sEffectSchedulerflushes effects.ResourceImpl(vialinkedSignal) and@tanstack/angular-query(via synchronousobserver.getOptimisticResultincomputed) update synchronously on read.- However, any custom resource whose snapshot transitions to
'loading'inside aneffect(() => { const p = params(); ... })+resourceFromSnapshots(state)(the exact pattern from Validate async signal leaks resource internals #70884) still holds the previous value's'resolved'/'error'snapshot until effects flush. Any read in between (e.g. in a microtask or an earlier-scheduledeffectwatching the form) seespending() === false, flashes stale validity, and triggersopts.onSuccess(res.value()!, ctx)whereres.value()is the old result whilectx.value()is already the new unvalidated field value (which poisons caches likeif (res.available) validated.add(ctx.value())).
-
A -> B -> Abounce-back within the debounce window no longer retries failed requests:
Suppose validating'alice'fails with a transient 500 error, and the user types'alice2'(pending()becomestrue, error clears) then backspaces to'alice'before the debounce timer fires.debouncedParams()was'alice'before,'alice'during debounce, and'alice'after debounce (Object.ismemoized incomputed), sofactoryResourcenever re-runs and the stale 500 error resurfaces without making a request. (Previously withɵchain,extRequesttransitioned through{status: 'loading'}, so settling back to'alice'triggered a fresh load.) -
In-flight loads & subscriptions keep running with stale
paramsduring debounce:
Even when the input changes—or becomes synchronously invalid /disabled/when() === false(where rawparams()is alreadyundefined)—factoryResourcecontinues seeing the old value as active for the entire debounce window (shouldDiscard()inResourceImplstaysfalse, allowing stale completions and SSRTransferStatewrites, and keeping factoryeffects / stream subscriptions alive on stale params).
(Note: returning undefined whenever snapshot.status !== 'resolved' && snapshot.status !== 'local' would still avoid throwing ResourceParamsStatus while fixing (2) and cleaning up/discarding stale work in (3), though (1) still needs care so an effect-driven resource doesn't briefly blip 'idle' before its effect sets 'loading'.)
| case 'error': | ||
| return snapshot; |
There was a problem hiding this comment.
Two error-handling edge cases when forwarding source.snapshot() directly into resourceFromSnapshots:
- Unlike
ResourceImpl,debounced()castserr as Errorwithout callingencapsulateResourceError(err)(previouslyɵchainwrapped errors inResourceDependencyError). Ifopts.paramsthrows a non-Errorvalue (e.g.throw undefinedorthrow 'bad'),SnapshotResourcestores a non-Errorinstate.error, violatingResourceSnapshot'sErrorinvariant and crashingres.value()insidenew ResourceValueError(this.state.error)(Cannot read properties of undefined (reading 'message')). - If
opts.paramsitself chains another resource viachain(...), thrownResourceParamsStatus.LOADING/IDLEmarkers work withoutdebounce(caught byResourceImpl), whereas withdebouncedebounced()catches them as errors and forwardsSymbol(ResourceParamsStatus.LOADING)intoopts.onError.
| reload: () => { | ||
| if (source.isLoading() || source.status() === 'error') { | ||
| return false; | ||
| } | ||
| return 'reload' in target && typeof target.reload === 'function' ? target.reload() : false; | ||
| }, |
There was a problem hiding this comment.
The commit message mentions "delegate reloads after debounce settles", but reload() doesn't defer or queue a reload—it returns false and drops any reloadValidation() call made while source.isLoading(). In an A -> B -> A edit during debounce where reloadValidation() is called while pending, settling back to A leaves debouncedParams unchanged and the reload is lost.
| snapshot.set({status: 'error', error: new Error('stale request failed')}); | ||
| await appRef.whenStable(); | ||
| expect(validationForm().pending()).toBe(true); | ||
| expect(validationForm().errors()).toEqual([]); | ||
|
|
||
| resolveDebounce(); | ||
| await waitFor(() => { | ||
| throwUnless(validationForm().pending()).toBe(false); | ||
| }); | ||
| await appRef.whenStable(); | ||
| expect(validationForm().pending()).toBe(false); | ||
| expect(validationForm().errors()).toEqual([{kind: 'error', fieldTree: validationForm}]); |
There was a problem hiding this comment.
Notice what this sequence actually demonstrates: Error('stale request failed') was produced while 'updated' was still debouncing (i.e. from the stale 'initial' request), yet as soon as resolveDebounce() resolves, that stale error becomes the active validation error on 'updated'. That's the exact unmasking race an effect-driven resourceFromSnapshots factory hits before its effect flushes for the settled params.
| validationForm().value.set('updated'); | ||
| await appRef.whenStable(); | ||
| validationForm().reloadValidation(); | ||
| await appRef.whenStable(); | ||
| expect(loader).toHaveBeenCalledTimes(2); | ||
| expect(validationForm().pending()).toBe(true); | ||
|
|
||
| resolveDebounce(); | ||
| await waitFor(() => { | ||
| throwUnless(loader).toHaveBeenCalledTimes(3); | ||
| }); | ||
| await appRef.whenStable(); | ||
| expect(loader).toHaveBeenCalledTimes(3); | ||
| expect(loader.calls.mostRecent().args[0].params).toBe('updated'); |
There was a problem hiding this comment.
This test is a false positive for reloadValidation() during debounce: loader is called a 3rd time only because value changed from 'initial' to 'updated' (deleting validationForm().reloadValidation() on line 632 still results in 3 calls). If value bounces 'initial' -> 'updated' -> 'initial' before resolveDebounce() and reloadValidation() is called during debounce, loader stays at 2 calls.
|
Hm, yes, this seems to be unfixable without breaking changes or creating a new API. Ignoring the effect driven resources, since those introduce timing problems that are out of scope of the main goal here, we have: Given that we want to remove the usage of
We need to change how the Option 1: Don't throw on debounced and retain the previous value: Doesn't work because it introduces behavioural issues, especially point 2 in your comment of Option 2: Don't throw on debounced and return undefined: Doesn't work because it will make resource start in an idle state, breaking current tests, because it makes the resulting resource Option 3: Throw, and add something to catch it: We could introduce mechanisms to catch the pending state. Automatically handling these markers across reactive consumers would be a broader architectural change (like Solid v2), something Angular does not do at the moment. So instead we can provide an explicit function, let's say For option 1 and 2, we also have other issues with how chain works, like the forwarding issues. Those are the only 3 options that look available without changing the API shape. If we agree with that, the preferable path forward might be to create a new API like: // Name TBD
validateResource(path.username, {
params: ({value}) => value(),
debounce: 300,
// Params is a resource instead
factory: (paramsResource) =>
resource({
params: ({chain}) => chain(paramsResource), // Explicitly call `chain`
loader: ({params}) => checkUsername(params),
}),
});For factories that do not use the resource API: // Could be part of Angular
function settledResourceValue<T>(resource: Resource<T>): Signal<T | undefined> {
return computed(() => {
const snapshot = resource.snapshot();
return snapshot.status === 'resolved' || snapshot.status === 'local'
? snapshot.value
: undefined;
});
}
validateResource(path.username, {
params: ({value}) => value(),
debounce: 300,
factory: (paramsResource) => {
const params = settledResourceValue(paramsResource);
const query = injectQuery(() => {
const username = params();
return {
queryKey: ['check-username', username],
queryFn: username === undefined ? skipToken : () => checkUsername(username),
};
});
return toResource(query);
},
});Ensuring proper chaining could be implemented internally or externally, since for example, the resource might have a resolved state while the debounced resource is still pending. function chainValidationResources<TParams, TResult>(
paramsResource: Resource<TParams | undefined>,
target: Resource<TResult | undefined> & {reload?: () => boolean},
) {
return Object.assign(
resourceFromSnapshots<TResult | undefined>(() => {
const params = paramsResource.snapshot();
switch (params.status) {
case 'error':
return params;
case 'resolved':
case 'local':
return params.value === undefined
? {status: 'idle', value: undefined}
: target.snapshot();
default:
return {status: params.status, value: undefined};
}
}),
{reload: () => target.reload?.() ?? false},
);
}
// Option 1: Inside validateResource, simpler for consumers without the Resource API
// This might be preferred to guarantee the behaviour validateResource needs internally
const target = opts.factory(paramsResource);
const validationResource = chainValidationResources(paramsResource, target);
// Option 2: Outside, in factory, less work for the happy path of the Resource API
const target = toResource(query);
const validationResource = chainValidationResources(paramsResource, target);The pending questions would be if Maybe it would be also useful to export a new // Name TBD
validateAsyncLoader(path.username, {
params: ({value}) => value(),
debounce: 300,
loader: ({params}) => checkUsername(params),
});Both of those functions would not call I will take look on how those ideas would look in a separate PR, keeping this PR open in the meantime. |
|
Closing since this isn't something that seems to be posible to continue. |
PR Checklist
PR Type
What kind of change does this PR introduce?
What is the current behavior?
In signal forms,
validateAsyncwithdebounceconfigured passes a params signal that can throw the internalResourceParamsStatus.LOADING. Factories that consume that signal in an effect or useresourceFromSnapshotscan fail outside theresource()params handler.The
ɵchainfunction seems only used in that way in signal forms (ɵchain(has only 1 result in the codebase), and the docs mention that chain is to be used on the params function, giving the consumer the option to chain resources, while thevalidateAsyncfunction hides that and can lead to unexpected errors when the param function throws in an unexpected context. That creates a problem for libraries that can return the Resource interface but don't use the Resource API, since it's the Resource API param's function that catches the issue, not the Interface that libraries can implement to adapt into Angular libraries.Issue Number: #70884
What is the new behavior?
Pass a safe debounced params signal to the factory, retaining the previous value during debounce, and compose the debounce resource and factory resource safely, without
ɵchain. The safe chain forwards reloads to preservesreloadValidation()functionality.Does this PR introduce a breaking change?
Other information
In the future, a resource type that declares the
Resource & { reload: () => boolean }could be used as a union to better declare the usage of the reload method, see #69443 (alternatives considered forvalidateAsyncinterface) and #71299 (Router'snonBlockingutility type).