fix(core): getting resource value throws an error instead of returning undefined (#61441)

Humberd · atscott · commit 3ba39bc28f93 · 2025-05-21T12:06:42.000-07:00
When there is an underlying error state it would not be possible to swallow the error with: `computed(() => res.value()?.inner);` PR Close #61441
diff --git a/packages/core/src/resource/api.ts b/packages/core/src/resource/api.ts
@@ -46,7 +46,7 @@ export type ResourceStatus = 'idle' | 'error' | 'loading' | 'reloading' | 'resol
  */
 export interface Resource<T> {
   /**
-   * The current value of the `Resource`, or `undefined` if there is no current value.
+   * The current value of the `Resource`, or throws an error if the resource is in an error state.
    */
   readonly value: Signal<T>;
 
@@ -167,7 +167,7 @@ export interface BaseResourceOptions<T, R> {
 
   /**
    * The value which will be returned from the resource when a server value is unavailable, such as
-   * when the resource is still loading, or in an error state.
+   * when the resource is still loading.
    */
   defaultValue?: NoInfer<T>;
 
diff --git a/packages/core/src/resource/resource.ts b/packages/core/src/resource/resource.ts
@@ -32,6 +32,13 @@ import {PendingTasks} from '../pending_tasks';
 import {linkedSignal} from '../render3/reactivity/linked_signal';
 import {DestroyRef} from '../linker/destroy_ref';
 
+/**
+ * Whether a `Resource.value()` should throw an error when the resource is in the error state.
+ *
+ * This internal flag is being used to gradually roll out this behavior.
+ */
+const RESOURCE_VALUE_THROWS_ERRORS_DEFAULT = true;
+
 /**
  * Constructs a `Resource` that projects a reactive request to an asynchronous operation defined by
  * a loader function, which exposes the result of the loading operation via signals.
@@ -72,6 +79,7 @@ export function resource<T, R>(options: ResourceOptions<T, R>): ResourceRef<T |
     options.defaultValue,
     options.equal ? wrapEqualityFn(options.equal) : undefined,
     options.injector ?? inject(Injector),
+    RESOURCE_VALUE_THROWS_ERRORS_DEFAULT,
   );
 }
 
@@ -114,13 +122,22 @@ abstract class BaseWritableResource<T> implements WritableResource<T> {
 
   abstract set(value: T): void;
 
+  private readonly isError = computed(() => this.status() === 'error');
+
   update(updateFn: (value: T) => T): void {
     this.set(updateFn(untracked(this.value)));
   }
 
   readonly isLoading = computed(() => this.status() === 'loading' || this.status() === 'reloading');
 
   hasValue(): this is ResourceRef<Exclude<T, undefined>> {
+    // Note: we specifically read `isError()` instead of `status()` here to avoid triggering
+    // reactive consumers which read `hasValue()`. This way, if `hasValue()` is used inside of an
+    // effect, it doesn't cause the effect to rerun on every status change.
+    if (this.isError()) {
+      return false;
+    }
+
     return this.value() !== undefined;
   }
 
@@ -154,17 +171,31 @@ export class ResourceImpl<T, R> extends BaseWritableResource<T> implements Resou
   constructor(
     request: () => R,
     private readonly loaderFn: ResourceStreamingLoader<T, R>,
-    private readonly defaultValue: T,
+    defaultValue: T,
     private readonly equal: ValueEqualityFn<T> | undefined,
     injector: Injector,
+    throwErrorsFromValue: boolean = RESOURCE_VALUE_THROWS_ERRORS_DEFAULT,
   ) {
     super(
       // Feed a computed signal for the value to `BaseWritableResource`, which will upgrade it to a
       // `WritableSignal` that delegates to `ResourceImpl.set`.
       computed(
         () => {
           const streamValue = this.state().stream?.();
-          return streamValue && isResolved(streamValue) ? streamValue.value : this.defaultValue;
+
+          if (!streamValue) {
+            return defaultValue;
+          }
+
+          if (!isResolved(streamValue)) {
+            if (throwErrorsFromValue) {
+              throw new ResourceValueError(this.error()!);
+            } else {
+              return defaultValue;
+            }
+          }
+
+          return streamValue.value;
         },
         {equal},
       ),
@@ -401,7 +432,7 @@ function projectStatusOfState(state: ResourceState<unknown>): ResourceStatus {
     case 'loading':
       return state.extRequest.reload === 0 ? 'loading' : 'reloading';
     case 'resolved':
-      return isResolved(untracked(state.stream!)) ? 'resolved' : 'error';
+      return isResolved(state.stream!()) ? 'resolved' : 'error';
     default:
       return state.status;
   }
@@ -419,6 +450,17 @@ export function encapsulateResourceError(error: unknown): Error {
   return new ResourceWrappedError(error);
 }
 
+class ResourceValueError extends Error {
+  constructor(error: Error) {
+    super(
+      ngDevMode
+        ? `Resource is currently in an error state (see Error.cause for details): ${error.message}`
+        : error.message,
+      {cause: error},
+    );
+  }
+}
+
 class ResourceWrappedError extends Error {
   constructor(error: unknown) {
     super(
diff --git a/packages/core/test/resource/resource_spec.ts b/packages/core/test/resource/resource_spec.ts
@@ -8,7 +8,9 @@
 
 import {
   ApplicationRef,
+  computed,
   createEnvironmentInjector,
+  effect,
   EnvironmentInjector,
   Injector,
   resource,
@@ -140,16 +142,18 @@ describe('resource', () => {
     });
 
     TestBed.tick();
-    await backend.reject(requestParam, 'Something went wrong....');
+    await backend.reject(requestParam, new Error('Something went wrong....'));
 
     expect(echoResource.status()).toBe('error');
     expect(echoResource.isLoading()).toBeFalse();
     expect(echoResource.hasValue()).toBeFalse();
-    expect(echoResource.value()).toEqual(undefined);
-    expect(echoResource.error()).toEqual(
-      jasmine.objectContaining({cause: 'Something went wrong....'}),
-    ),
-      expect(echoResource.error()!.message).toContain('Resource');
+
+    const err = extractError(() => echoResource.value())!;
+    expect(err).not.toBeUndefined();
+    expect(err instanceof Error).toBeTrue();
+    expect(err!.message).toContain('Resource');
+    expect(err.cause).toEqual(new Error('Something went wrong....'));
+    expect(echoResource.error()).toEqual(new Error('Something went wrong....'));
   });
 
   it('should expose errors on reload', async () => {
@@ -183,8 +187,109 @@ describe('resource', () => {
     expect(echoResource.status()).toBe('error');
     expect(echoResource.isLoading()).toBeFalse();
     expect(echoResource.hasValue()).toBeFalse();
-    expect(echoResource.value()).toEqual(undefined);
+    const err = extractError(() => echoResource.value())!;
+    expect(err).not.toBeUndefined();
+    expect(err.message).toContain('Resource');
+    expect(err.message).toContain('KO');
+    expect(err.cause).toEqual(new Error('KO'));
+    expect(echoResource.error()).toEqual(Error('KO'));
+
+    counter.update((value) => value + 1);
+    TestBed.tick();
+    await backend.flush();
+
+    expect(echoResource.status()).toBe('resolved');
+    expect(echoResource.isLoading()).toBeFalse();
+    expect(echoResource.hasValue()).toBeTrue();
+    expect(echoResource.value()).toEqual('ok');
+    expect(echoResource.error()).toBe(undefined);
+  });
+
+  it('should not trigger consumers on every status change via hasValue()', async () => {
+    const params = signal<number | undefined>(undefined);
+    const testResource = resource({
+      params,
+      // Hanging promise, never resolves
+      loader: () => new Promise(() => {}),
+      injector: TestBed.inject(Injector),
+    });
+
+    let effectRuns = 0;
+    effect(
+      () => {
+        testResource.hasValue();
+        effectRuns++;
+      },
+      {injector: TestBed.inject(Injector)},
+    );
+
+    TestBed.tick();
+    // Params are undefined, status should be idle.
+    expect(testResource.status()).toBe('idle');
+    // Effect should run the first time.
+    expect(effectRuns).toBe(1);
+
+    // Set params, causing the stauts to become 'loading'.
+    params.set(0);
+    expect(testResource.status()).toBe('loading');
+    TestBed.tick();
+    // The effect should not rerun.
+    expect(effectRuns).toBe(1);
+  });
+
+  it('should update computed signals', async () => {
+    const backend = new MockEchoBackend();
+    const counter = signal(0);
+    const echoResource = resource({
+      params: () => ({counter: counter()}),
+      loader: (params) => {
+        if (params.params.counter % 2 === 0) {
+          return Promise.resolve(params.params.counter);
+        } else {
+          throw new Error('KO');
+        }
+      },
+      injector: TestBed.inject(Injector),
+    });
+    const computedValue = computed(() => {
+      if (!echoResource.hasValue()) {
+        return -1;
+      }
+      return echoResource.value();
+    });
+
+    TestBed.tick();
+    await backend.flush();
+
+    expect(echoResource.status()).toBe('resolved');
+    expect(echoResource.hasValue()).toBeTrue();
+    expect(echoResource.value()).toEqual(0);
+    expect(computedValue()).toEqual(0);
+    expect(echoResource.error()).toBe(undefined);
+
+    counter.update((value) => value + 1);
+    TestBed.tick();
+    await backend.flush();
+
+    expect(echoResource.status()).toBe('error');
+    expect(echoResource.hasValue()).toBeFalse();
+    const err = extractError(() => echoResource.value())!;
+    expect(err).not.toBeUndefined();
+    expect(err.message).toContain('Resource');
+    expect(err.message).toContain('KO');
+    expect(err.cause).toEqual(new Error('KO'));
+    expect(computedValue()).toEqual(-1);
     expect(echoResource.error()).toEqual(Error('KO'));
+
+    counter.update((value) => value + 1);
+    TestBed.tick();
+    await backend.flush();
+
+    expect(echoResource.status()).toBe('resolved');
+    expect(echoResource.hasValue()).toBeTrue();
+    expect(echoResource.value()).toEqual(2);
+    expect(computedValue()).toEqual(2);
+    expect(echoResource.error()).toBe(undefined);
   });
 
   it('should respond to a request that changes while loading', async () => {
@@ -231,7 +336,7 @@ describe('resource', () => {
     expect(res.value()).toBe(1);
   });
 
-  it('should return a default value if provided', async () => {
+  it('should throw an error when getting a value even when provided with a default value', async () => {
     const DEFAULT: string[] = [];
     const request = signal(0);
     const res = resource({
@@ -256,7 +361,11 @@ describe('resource', () => {
     request.set(2);
     await TestBed.inject(ApplicationRef).whenStable();
     expect(res.error()).not.toBeUndefined();
-    expect(res.value()).toBe(DEFAULT);
+    const err = extractError(() => res.value())!;
+    expect(err).not.toBeUndefined();
+    expect(err.message).toContain('Resource');
+    expect(err.message).toContain('err');
+    expect(err.cause).toEqual(new Error('err'));
   });
 
   it('should _not_ load if the request resolves to undefined', () => {
@@ -731,3 +840,12 @@ describe('resource', () => {
 function flushMicrotasks(): Promise<void> {
   return new Promise((resolve) => setTimeout(resolve, 0));
 }
+
+function extractError(fn: () => unknown): Error | undefined {
+  try {
+    fn();
+    return undefined;
+  } catch (err: unknown) {
+    return err as Error;
+  }
+}
