Skip to content

fix(forms): safely chain debounced async validation resources - #71414

Closed
benjavicente wants to merge 1 commit into
angular:mainfrom
benjavicente:fix/safe-chain-resources-async-validation
Closed

benjavicente wants to merge 1 commit into
angular:mainfrom
benjavicente:fix/safe-chain-resources-async-validation

Conversation

@benjavicente

@benjavicente benjavicente commented Oct 17, 2026 •

Copy link
Copy Markdown

PR Checklist

PR Type

What kind of change does this PR introduce?

  • Bugfix

What is the current behavior?

In signal forms, validateAsync with debounce configured passes a params signal that can throw the internal ResourceParamsStatus.LOADING. Factories that consume that signal in an effect or use resourceFromSnapshots can fail outside the resource() params handler.

The ɵchain function 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 the validateAsync function 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 preserves reloadValidation() functionality.

Does this PR introduce a breaking change?

  • Yes
  • No

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 for validateAsync interface) and #71299 (Router's nonBlocking utility type).

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
@pullapprove
pullapprove Bot requested a review from atscott October 17, 2026 23:20
@ngbot ngbot Bot added this to the Backlog milestone Oct 17, 2026
@JeanMeche
JeanMeche requested review from JeanMeche and leonsenft and removed request for atscott October 23, 2026 01:32

@JeanMeche JeanMeche left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for digging into #70884 and putting together the test cases!

Stepping back, there are two levels of concerns with this approach:

  1. Architectural layer (@angular/forms/signals vs @angular/core):

    • validateAsync is currently the only consumer of ɵchain in the repo (removing the import here leaves chain as ɵchain in packages/core/src/core_private_export.ts unused).
    • At the same time, the underlying gap isn't specific to Signal Forms: anyone composing debounced() with a non-resource() Resource implementation (such as @tanstack/angular-query or a custom resourceFromSnapshots() resource) hits the exact same wall because chain() relies on thrown ResourceParamsStatus markers only caught by ResourceImpl, while @angular/core has no snapshot-based chaining primitive.
  2. Behavioral & correctness regressions in debouncedParams + safeChainResources:

    • Keeping the previous settled value in debouncedParams while debouncing and unmasking target.snapshot() as soon as debouncedResource settles isn't semantically equivalent to ɵchain: it introduces a stale-result/onSuccess race for effect-driven custom resources, breaks A -> B -> A retry after transient errors, keeps stale loads/streams active during debounce, and silently drops reloadValidation() calls made while debouncing.

Left detailed notes inline on each of these.

Comment on lines +91 to +92
* When debouncing, params keeps its previous value until the debounce completes, while validation
* remains pending.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two concerns with adding this to @publicApi 22.0 JSDoc:

  1. Inaccurate on errors: When opts.params throws, debouncedParams becomes undefined, not its previous value.
  2. Premature API contract lock-in: The preceding sentence intentionally treats params as opaque ("The given params should be used as is to create the resource."). Documenting that params retains its stale value during debounce locks an implementation workaround into the v22 public contract and prevents future fixes (e.g., emitting undefined while debouncing or addressing chaining in @angular/core).

Comment on lines +151 to +156
const debouncedParams = computed(() => {
const snapshot = debouncedResource.snapshot();
return snapshot.status === 'error' ? undefined : snapshot.value;
});
const factoryResource = opts.factory(debouncedParams);
return safeChainResources(debouncedResource, factoryResource);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keeping the previous settled snapshot.value in debouncedParams while debouncedResource is 'loading' introduces three behavioral issues:

  1. Stale snapshot window & onSuccess(oldResult, newCtx) mismatch for effect-driven resources:
    In debounce.ts, state.set({status: 'resolved', value}) runs inside a Promise .then() microtask. At that instant, debouncedResource.snapshot() becomes 'resolved' and safeChainResources immediately unmasks target.snapshot()—before Angular's EffectScheduler flushes effects.

    • ResourceImpl (via linkedSignal) and @tanstack/angular-query (via synchronous observer.getOptimisticResult in computed) update synchronously on read.
    • However, any custom resource whose snapshot transitions to 'loading' inside an effect(() => { 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-scheduled effect watching the form) sees pending() === false, flashes stale validity, and triggers opts.onSuccess(res.value()!, ctx) where res.value() is the old result while ctx.value() is already the new unvalidated field value (which poisons caches like if (res.available) validated.add(ctx.value())).
  2. A -> B -> A bounce-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() becomes true, error clears) then backspaces to 'alice' before the debounce timer fires. debouncedParams() was 'alice' before, 'alice' during debounce, and 'alice' after debounce (Object.is memoized in computed), so factoryResource never re-runs and the stale 500 error resurfaces without making a request. (Previously with ɵchain, extRequest transitioned through {status: 'loading'}, so settling back to 'alice' triggered a fresh load.)

  3. In-flight loads & subscriptions keep running with stale params during debounce:
    Even when the input changes—or becomes synchronously invalid / disabled / when() === false (where raw params() is already undefined)—factoryResource continues seeing the old value as active for the entire debounce window (shouldDiscard() in ResourceImpl stays false, allowing stale completions and SSR TransferState writes, and keeping factory effects / 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'.)

Comment on lines +210 to +211
case 'error':
return snapshot;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two error-handling edge cases when forwarding source.snapshot() directly into resourceFromSnapshots:

  1. Unlike ResourceImpl, debounced() casts err as Error without calling encapsulateResourceError(err) (previously ɵchain wrapped errors in ResourceDependencyError). If opts.params throws a non-Error value (e.g. throw undefined or throw 'bad'), SnapshotResource stores a non-Error in state.error, violating ResourceSnapshot's Error invariant and crashing res.value() inside new ResourceValueError(this.state.error) (Cannot read properties of undefined (reading 'message')).
  2. If opts.params itself chains another resource via chain(...), thrown ResourceParamsStatus.LOADING / IDLE markers work without debounce (caught by ResourceImpl), whereas with debounce debounced() catches them as errors and forwards Symbol(ResourceParamsStatus.LOADING) into opts.onError.

Comment on lines +217 to +222
reload: () => {
if (source.isLoading() || source.status() === 'error') {
return false;
}
return 'reload' in target && typeof target.reload === 'function' ? target.reload() : false;
},

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +531 to +542
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}]);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +630 to +643
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');

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@benjavicente

Copy link
Copy Markdown
Author

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 ɵchain inside the implementation of validateAsync

Why: In the docs chain, is available in the params context object of the resource API. By calling it internally in signal forms, it forces consumers to use the Resource API:

validateAsync(path.username, {
  params: ({value}) => value(),
  debounce: 300,
  factory: (params) =>
    resource({
      // Implicitly catches `chain`
      // For users, this might seem redundant with params above doing the same thing.
      // Why can't I read params directly in loader, if it is a Signal?
      params: () => params(),
      loader: ({params}) => checkUsername(params),
    }),
});

Factories that return the Resource Interface but do not use the Resource API don't work with validateAsync because they don't have a way of catching the implicit chain.
No other utility in Angular calls ɵchain outside of the params function.

We need to change how the params are given to the factory.

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 A -> B -> A chains.

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 idle instead of loading.

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 wrapChained that takes a chained signal and returns a Resource, where the usage of that would only be required in validateAsync for non resource API usage. Both options don't seem great from an API design standpoint.

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 validateResource should exist, the name of it, and what should happen to validateAsync (should it be deprecated)?

Maybe it would be also useful to export a new validateAsyncLoader that simplifies the resource chaining with loaders, like with this example in the docs.

// Name TBD
validateAsyncLoader(path.username, {
  params: ({value}) => value(),
  debounce: 300,
  loader: ({params}) => checkUsername(params),
});

Both of those functions would not call ɵchain directly like validateAsync does.

I will take look on how those ideas would look in a separate PR, keeping this PR open in the meantime.

@benjavicente

Copy link
Copy Markdown
Author

Closing since this isn't something that seems to be posible to continue.

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