refactor(core): port resource() to linkedSignal() (#59024)

When the reactive `request` of a resource() notifies, it transitions to the
Loading state, fires the loader, and eventually transitions to Resolved.
With the prior implementation, a change of the `request` will queue the
effect, but the state remains unchanged until the effect actually runs. For
a brief period, the resource is in a state where the request has changed,
but the state has yet to update.

This is problematic if we want to use resources in certain contexts where we
care about the state of the resource in a synchronous way. For example, an
async validator backed by a resource might be checked after an update:

```
value.set(123);

if (validator.value()) {
  // value is still valid, even though the resource is dirty and will soon
  // flip to loading state (returning value(): undefined) while revalidating
}
```

To address this timing concern, `linkedSignal()` is used within the
`resource()` to synchronously transition the state in response to the
request changing. This ensures any followup reads see a consistent view of
the resource regardless of whether the effect has run.

This also addresses a race condition where `.set()` behaves differently on a
`resource()` depending on whether or not the effect has run.

PR Close #59024

packages/core/rxjs-interop/test/rx_resource_spec.ts

@@ -26,12 +26,14 @@ describe('rxResource()', () => {
   it('should cancel the fetch when a new request comes in', async () => {
     const injector = TestBed.inject(Injector);
     const appRef = TestBed.inject(ApplicationRef);
-    let unsub = false;
     const request = signal(1);
-    const res = rxResource({
+    let unsub = false;
+    let lastSeenRequest: number = 0;
+    rxResource({
       request,
-      loader: ({request}) =>
-        new Observable((sub) => {
+      loader: ({request}) => {
+        lastSeenRequest = request;
+        return new Observable((sub) => {
           if (request === 2) {
             sub.next(true);
           }
@@ -40,12 +42,13 @@ describe('rxResource()', () => {
               unsub = true;
             }
           };
-        }),
+        });
+      },
       injector,
     });
 
     // Wait for the resource to reach loading state.
-    await waitFor(() => res.isLoading());
+    await waitFor(() => lastSeenRequest === 1);
 
     // Setting request = 2 should cancel request = 1
     request.set(2);

packages/core/src/resource/resource.ts

@@ -8,7 +8,7 @@
 
 import {untracked} from '../render3/reactivity/untracked';
 import {computed} from '../render3/reactivity/computed';
-import {signal, WritableSignal} from '../render3/reactivity/signal';
+import {signal, signalAsReadonlyFn, WritableSignal} from '../render3/reactivity/signal';
 import {Signal} from '../render3/reactivity/api';
 import {effect, EffectRef} from '../render3/reactivity/effect';
 import {
@@ -19,12 +19,13 @@ import {
   Resource,
   ResourceRef,
 } from './api';
-import {ValueEqualityFn, SIGNAL, SignalNode} from '@angular/core/primitives/signals';
+import {ValueEqualityFn} from '@angular/core/primitives/signals';
 import {Injector} from '../di/injector';
 import {assertInInjectionContext} from '../di/contextual';
 import {inject} from '../di/injector_compatibility';
 import {PendingTasks} from '../pending_tasks';
-import {DestroyRef} from '../linker';
+import {linkedSignal} from '../render3/reactivity/linked_signal';
+import {DestroyRef} from '../linker/destroy_ref';
 
 /**
  * Constructs a `Resource` that projects a reactive request to an asynchronous operation defined by
@@ -39,57 +40,45 @@ import {DestroyRef} from '../linker';
 export function resource<T, R>(options: ResourceOptions<T, R>): ResourceRef<T | undefined> {
   options?.injector || assertInInjectionContext(resource);
   const request = (options.request ?? (() => null)) as () => R;
-  return new WritableResourceImpl<T | undefined, R>(
+  return new ResourceImpl<T | undefined, R>(
     request,
     options.loader,
     undefined,
     options.equal ? wrapEqualityFn(options.equal) : undefined,
-    options.injector,
+    options.injector ?? inject(Injector),
   );
 }
 
 /**
- * Base class for `WritableResource` which handles the state operations and is unopinionated on the
- * actual async operation.
- *
- * Mainly factored out for better readability.
+ * Internal state of a resource.
+ */
+interface ResourceState<T> {
+  status: ResourceStatus;
+  previousStatus: ResourceStatus;
+  value: T;
+  error: unknown | undefined;
+}
+
+/**
+ * Base class which implements `.value` as a `WritableSignal` by delegating `.set` and `.update`.
  */
 abstract class BaseWritableResource<T> implements WritableResource<T> {
   readonly value: WritableSignal<T>;
-  readonly status = signal<ResourceStatus>(ResourceStatus.Idle);
-  readonly error = signal<unknown>(undefined);
+  abstract readonly status: Signal<ResourceStatus>;
+  abstract readonly error: Signal<unknown>;
+  abstract reload(): boolean;
 
-  protected readonly rawSetValue: (value: T) => void;
-
-  constructor(
-    protected defaultValue: T,
-    equal: ValueEqualityFn<T> | undefined,
-  ) {
-    this.value = signal<T>(this.defaultValue, {equal});
-    this.rawSetValue = this.value.set;
-    this.value.set = (value: T) => this.set(value);
-    this.value.update = (fn: (value: T) => T) => this.set(fn(untracked(this.value)));
+  constructor(value: Signal<T>) {
+    this.value = value as WritableSignal<T>;
+    this.value.set = this.set.bind(this);
+    this.value.update = this.update.bind(this);
+    this.value.asReadonly = signalAsReadonlyFn;
   }
 
-  set(value: T): void {
-    // Set the value signal and check whether its `version` changes. This will tell us
-    // if the value signal actually updated or not.
-    const prevVersion = (this.value[SIGNAL] as SignalNode<T>).version;
-    this.rawSetValue(value);
-    if ((this.value[SIGNAL] as SignalNode<T>).version === prevVersion) {
-      // The value must've been equal to the previous, so no need to change states.
-      return;
-    }
+  abstract set(value: T): void;
 
-    // We're departing from whatever state the resource was in previously, and entering
-    // Local state.
-    this.onLocalValue();
-    this.status.set(ResourceStatus.Local);
-    this.error.set(undefined);
-  }
-
-  update(updater: (value: T) => T): void {
-    this.value.update(updater);
+  update(updateFn: (value: T) => T): void {
+    this.set(updateFn(untracked(this.value)));
   }
 
   readonly isLoading = computed(
@@ -103,71 +92,117 @@ abstract class BaseWritableResource<T> implements WritableResource<T> {
   asReadonly(): Resource<T> {
     return this;
   }
-
-  /**
-   * Put the resource in a state with a given value.
-   */
-  protected setValueState(status: ResourceStatus, value: T = this.defaultValue): void {
-    this.status.set(status);
-    this.rawSetValue(value);
-    this.error.set(undefined);
-  }
-
-  /**
-   * Put the resource into the error state.
-   */
-  protected setErrorState(err: unknown): void {
-    this.value.set(this.defaultValue);
-    // The previous line will set the status to `Local`, so we need to update it.
-    this.status.set(ResourceStatus.Error);
-    this.error.set(err);
-  }
-
-  /**
-   * Called when the resource is transitioning to local state.
-   *
-   * For example, this can be used to cancel any in-progress loading operations.
-   */
-  protected abstract onLocalValue(): void;
-
-  public abstract reload(): boolean;
 }
 
-class WritableResourceImpl<T, R> extends BaseWritableResource<T> implements ResourceRef<T> {
-  private readonly request: Signal<{request: R; reload: WritableSignal<number>}>;
+/**
+ * Implementation for `resource()` which uses a `linkedSignal` to manage the resource's state.
+ */
+class ResourceImpl<T, R> extends BaseWritableResource<T> implements ResourceRef<T> {
+  /**
+   * The current state of the resource. Status, value, and error are derived from this.
+   */
+  private readonly state: WritableSignal<ResourceState<T>>;
+
+  /**
+   * Signal of both the request value `R` and a writable `reload` signal that's linked/associated
+   * to the given request. Changing the value of the `reload` signal causes the resource to reload.
+   */
+  private readonly extendedRequest: Signal<{request: R; reload: WritableSignal<number>}>;
+
   private readonly pendingTasks: PendingTasks;
   private readonly effectRef: EffectRef;
 
   private pendingController: AbortController | undefined;
   private resolvePendingTask: (() => void) | undefined = undefined;
+  private destroyed = false;
 
   constructor(
-    requestFn: () => R,
+    request: () => R,
     private readonly loaderFn: ResourceLoader<T, R>,
-    defaultValue: T,
-    equal: ValueEqualityFn<T> | undefined,
-    injector: Injector | undefined,
+    private readonly defaultValue: T,
+    private readonly equal: ValueEqualityFn<T> | undefined,
+    injector: Injector,
   ) {
-    super(defaultValue, equal);
-    injector = injector ?? inject(Injector);
+    // Feed a computed signal for the value to `BaseWritableResource`, which will upgrade it to a
+    // `WritableSignal` that delegates to `ResourceImpl.set`.
+    super(computed(() => this.state().value, {equal}));
     this.pendingTasks = injector.get(PendingTasks);
 
-    this.request = computed(() => ({
-      // The current request as defined for this resource.
-      request: requestFn(),
-
-      // A counter signal which increments from 0, re-initialized for each request (similar to the
-      // `linkedSignal` pattern). A value other than 0 indicates a refresh operation.
+    // Extend `request()` to include a writable reload signal.
+    this.extendedRequest = computed(() => ({
+      request: request(),
       reload: signal(0),
     }));
 
-    // The actual data-fetching effect.
-    this.effectRef = effect(this.loadEffect.bind(this), {injector, manualCleanup: true});
+    // The main resource state is managed in a `linkedSignal`, which allows the resource to change
+    // state instantaneously when the request signal changes.
+    this.state = linkedSignal<ResourceStatus, ResourceState<T>>({
+      // We use the request (as well as its reload signal) to derive the initial status of the
+      // resource (Idle, Loading, or Reloading) in response to request changes. From this initial
+      // status, the resource's effect will then trigger the loader and update to a Resolved or
+      // Error state as appropriate.
+      source: () => {
+        const {request, reload} = this.extendedRequest();
+        if (request === undefined || this.destroyed) {
+          return ResourceStatus.Idle;
+        }
+        return reload() === 0 ? ResourceStatus.Loading : ResourceStatus.Reloading;
+      },
+      // Compute the state of the resource given a change in status.
+      computation: (status, previous) =>
+        ({
+          status,
+          // When the state of the resource changes due to the request, remember the previous status
+          // for the loader to consider.
+          previousStatus: previous?.value.status ?? ResourceStatus.Idle,
+          // In `Reloading` state, we keep the previous value if there is one, since the identity of
+          // the request hasn't changed. Otherwise, we switch back to the default value.
+          value:
+            previous && status === ResourceStatus.Reloading
+              ? previous.value.value
+              : this.defaultValue,
+          error: undefined,
+        }) satisfies ResourceState<T>,
+    });
+
+    this.effectRef = effect(this.loadEffect.bind(this), {
+      injector,
+      manualCleanup: true,
+    });
 
     // Cancel any pending request when the resource itself is destroyed.
     injector.get(DestroyRef).onDestroy(() => this.destroy());
   }
 
+  override readonly status = computed(() => this.state().status);
+  override readonly error = computed(() => this.state().error);
+
+  /**
+   * Called either directly via `WritableResource.set` or via `.value.set()`.
+   */
+  override set(value: T): void {
+    if (this.destroyed) {
+      return;
+    }
+
+    const currentState = untracked(this.state);
+    if (this.equal ? this.equal(currentState.value, value) : currentState.value === value) {
+      return;
+    }
+
+    // Enter Local state with the user-defined value.
+    this.state.set({
+      status: ResourceStatus.Local,
+      previousStatus: ResourceStatus.Local,
+      value,
+      error: undefined,
+    });
+
+    // We're departing from whatever state the resource was in previously, so cancel any in-progress
+    // loading operations.
+    this.abortInProgressLoad();
+  }
+
   override reload(): boolean {
     // We don't want to restart in-progress loads.
     const status = untracked(this.status);
@@ -179,38 +214,50 @@ class WritableResourceImpl<T, R> extends BaseWritableResource<T> implements Reso
       return false;
     }
 
-    untracked(this.request).reload.update((v) => v + 1);
+    // Increment the reload signal to trigger the `state` linked signal to switch us to `Reload`
+    untracked(this.extendedRequest).reload.update((v) => v + 1);
     return true;
   }
 
   destroy(): void {
+    this.destroyed = true;
     this.effectRef.destroy();
-
     this.abortInProgressLoad();
-    this.setValueState(ResourceStatus.Idle);
+
+    // Destroyed resources enter Idle state.
+    this.state.set({
+      status: ResourceStatus.Idle,
+      previousStatus: ResourceStatus.Idle,
+      value: this.defaultValue,
+      error: undefined,
+    });
   }
 
   private async loadEffect(): Promise<void> {
-    // Capture the status before any state transitions.
-    const previousStatus = untracked(this.status);
+    // Capture the previous status before any state transitions. Note that this is `untracked` since
+    // we do not want the effect to depend on the state of the resource, only on the request.
+    const {status: previousStatus} = untracked(this.state);
 
-    // Cancel any previous loading attempts.
-    this.abortInProgressLoad();
+    const {request, reload: reloadCounter} = this.extendedRequest();
+    // Subscribe side-effectfully to `reloadCounter`, although we don't actually care about its
+    // value. This is used to rerun the effect when `reload()` is triggered.
+    reloadCounter();
 
-    const request = this.request();
-    if (request.request === undefined) {
-      // An undefined request means there's nothing to load.
-      this.setValueState(ResourceStatus.Idle);
+    if (request === undefined) {
+      // Nothing to load (and we should already be in a non-loading state).
+      return;
+    } else if (
+      previousStatus !== ResourceStatus.Loading &&
+      previousStatus !== ResourceStatus.Reloading
+    ) {
+      // We might've transitioned into a loading state, but has since been overwritten (likely via
+      // `.set`).
+      // In this case, the resource has nothing to do.
       return;
     }
 
-    // Subscribing here allows us to refresh the load later by updating the refresh signal. At the
-    // same time, we update the status according to whether we're reloading or loading.
-    if (request.reload() === 0) {
-      this.setValueState(ResourceStatus.Loading); // value becomes undefined
-    } else {
-      this.status.set(ResourceStatus.Reloading); // value persists
-    }
+    // Cancel any previous loading attempts.
+    this.abortInProgressLoad();
 
     // Capturing _this_ load's pending task in a local variable is important here. We may attempt to
     // resolve it twice:
@@ -231,7 +278,7 @@ class WritableResourceImpl<T, R> extends BaseWritableResource<T> implements Reso
       const result = await untracked(() =>
         this.loaderFn({
           abortSignal,
-          request: request.request as Exclude<R, undefined>,
+          request: request as Exclude<R, undefined>,
           previous: {
             status: previousStatus,
           },
@@ -242,32 +289,41 @@ class WritableResourceImpl<T, R> extends BaseWritableResource<T> implements Reso
         return;
       }
       // Success :)
-      this.setValueState(ResourceStatus.Resolved, result);
+      this.state.set({
+        status: ResourceStatus.Resolved,
+        previousStatus: ResourceStatus.Resolved,
+        value: result,
+        error: undefined,
+      });
     } catch (err) {
       if (abortSignal.aborted) {
         // This load operation was cancelled.
         return;
       }
       // Fail :(
-      this.setErrorState(err);
+      this.state.set({
+        status: ResourceStatus.Error,
+        previousStatus: ResourceStatus.Error,
+        value: this.defaultValue,
+        error: err,
+      });
     } finally {
       // Resolve the pending task now that loading is done.
       resolvePendingTask();
+
+      // Free the abort controller to drop any registered 'abort' callbacks.
+      this.pendingController = undefined;
     }
   }
 
   private abortInProgressLoad(): void {
-    this.pendingController?.abort();
+    untracked(() => this.pendingController?.abort());
     this.pendingController = undefined;
 
     // Once the load is aborted, we no longer want to block stability on its resolution.
     this.resolvePendingTask?.();
     this.resolvePendingTask = undefined;
   }
-
-  protected override onLocalValue(): void {
-    this.abortInProgressLoad();
-  }
 }
 
 /**

packages/core/test/resource/resource_spec.ts

@@ -7,6 +7,7 @@
  */
 
 import {
+  ApplicationRef,
   createEnvironmentInjector,
   EnvironmentInjector,
   Injector,
@@ -81,22 +82,13 @@ describe('resource', () => {
       injector: TestBed.inject(Injector),
     });
 
-    // a freshly created resource is in the idle state
-    expect(echoResource.status()).toBe(ResourceStatus.Idle);
-    expect(echoResource.isLoading()).toBeFalse();
-    expect(echoResource.hasValue()).toBeFalse();
-    expect(echoResource.value()).toBeUndefined();
-    expect(echoResource.error()).toBe(undefined);
-
-    // flush effect to kick off a request
-    // THINK: testing patterns around a resource?
-    TestBed.flushEffects();
+    // a freshly created resource is in the loading state
     expect(echoResource.status()).toBe(ResourceStatus.Loading);
     expect(echoResource.isLoading()).toBeTrue();
     expect(echoResource.hasValue()).toBeFalse();
     expect(echoResource.value()).toBeUndefined();
     expect(echoResource.error()).toBe(undefined);
-
+    TestBed.flushEffects();
     await backend.flush();
     expect(echoResource.status()).toBe(ResourceStatus.Resolved);
     expect(echoResource.isLoading()).toBeFalse();
@@ -362,6 +354,9 @@ describe('resource', () => {
     expect(res.error()).toBe(undefined);
 
     res.reload();
+    expect(res.status()).toBe(ResourceStatus.Reloading);
+    expect(res.value()).toBe('0:0');
+
     TestBed.flushEffects();
     await backend.flush();
     expect(res.status()).toBe(ResourceStatus.Resolved);
@@ -411,4 +406,94 @@ describe('resource', () => {
     // @ts-expect-error
     readonlyRes.value.set;
   });
+
+  it('should synchronously change states', async () => {
+    const request = signal<number | undefined>(undefined);
+    const backend = new MockEchoBackend();
+    const echoResource = resource({
+      request,
+      loader: (params) => backend.fetch(params.request),
+      injector: TestBed.inject(Injector),
+    });
+    // Idle to start.
+    expect(echoResource.status()).toBe(ResourceStatus.Idle);
+    // Switch to loading state should be synchronous.
+    request.set(1);
+    expect(echoResource.status()).toBe(ResourceStatus.Loading);
+    // And back to idle.
+    request.set(undefined);
+    expect(echoResource.status()).toBe(ResourceStatus.Idle);
+    // Allow the load to proceed.
+    request.set(2);
+    TestBed.flushEffects();
+    await backend.flush();
+    expect(echoResource.status()).toBe(ResourceStatus.Resolved);
+    // Reload state should be synchronous.
+    echoResource.reload();
+    expect(echoResource.status()).toBe(ResourceStatus.Reloading);
+    // Back to idle.
+    request.set(undefined);
+    expect(echoResource.status()).toBe(ResourceStatus.Idle);
+  });
+  it('set() should abort a pending load', async () => {
+    const request = signal<number | undefined>(1);
+    const backend = new MockEchoBackend();
+    const echoResource = resource({
+      request,
+      loader: (params) => backend.fetch(params.request),
+      injector: TestBed.inject(Injector),
+    });
+    const appRef = TestBed.inject(ApplicationRef);
+    // Fully resolve the resource to start.
+    TestBed.flushEffects();
+    await backend.flush();
+    expect(echoResource.status()).toBe(ResourceStatus.Resolved);
+    // Trigger loading state.
+    request.set(2);
+    expect(echoResource.status()).toBe(ResourceStatus.Loading);
+    // Set the resource to a new value.
+    echoResource.set(3);
+    // Now run the effect, which should be a no-op as the resource was set to a local value.
+    TestBed.flushEffects();
+    // We should still be in local state.
+    expect(echoResource.status()).toBe(ResourceStatus.Local);
+    expect(echoResource.value()).toBe(3);
+    // Flush the resource
+    await backend.flush();
+    await appRef.whenStable();
+    // We should still be in local state.
+    expect(echoResource.status()).toBe(ResourceStatus.Local);
+    expect(echoResource.value()).toBe(3);
+  });
+
+  it('set() should abort a pending reload', async () => {
+    const request = signal<number | undefined>(1);
+    const backend = new MockEchoBackend();
+    const echoResource = resource({
+      request,
+      loader: (params) => backend.fetch(params.request),
+      injector: TestBed.inject(Injector),
+    });
+    const appRef = TestBed.inject(ApplicationRef);
+    // Fully resolve the resource to start.
+    TestBed.flushEffects();
+    await backend.flush();
+    expect(echoResource.status()).toBe(ResourceStatus.Resolved);
+    // Trigger reloading state.
+    echoResource.reload();
+    expect(echoResource.status()).toBe(ResourceStatus.Reloading);
+    // Set the resource to a new value.
+    echoResource.set(3);
+    // Now run the effect, which should be a no-op as the resource was set to a local value.
+    TestBed.flushEffects();
+    // We should still be in local state.
+    expect(echoResource.status()).toBe(ResourceStatus.Local);
+    expect(echoResource.value()).toBe(3);
+    // Flush the resource
+    await backend.flush();
+    await appRef.whenStable();
+    // We should still be in local state.
+    expect(echoResource.status()).toBe(ResourceStatus.Local);
+    expect(echoResource.value()).toBe(3);
+  });
 });