fix(core): fix change tracking for Resource#hasValue (#62595)

lizclipse · NothingEverHappens · commit 2c522efbe500 · 2025-07-18T10:01:44.000-04:00
When using `hasValue()` I would expect it to behave like any other reactive value such that changes to the internal `value()` that do not cause `hasValue()` to return anything different do not trigger change detection, but this was not the case. This change wraps the value checking in a `computed` such that it behaves as expected again while still preserving the type narrowing. PR Close #62595
diff --git a/packages/core/src/resource/resource.ts b/packages/core/src/resource/resource.ts
@@ -130,15 +130,19 @@ abstract class BaseWritableResource<T> implements WritableResource<T> {
 
   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.
+  // Use a computed here to avoid triggering reactive consumers if the value changes while staying
+  // either defined or undefined.
+  private readonly isValueDefined = computed(() => {
+    // Check if it's in an error state first to prevent the error from bubbling up.
     if (this.isError()) {
       return false;
     }
 
     return this.value() !== undefined;
+  });
+
+  hasValue(): this is ResourceRef<Exclude<T, undefined>> {
+    return this.isValueDefined();
   }
 
   asReadonly(): Resource<T> {
diff --git a/packages/core/test/resource/resource_spec.ts b/packages/core/test/resource/resource_spec.ts
@@ -236,6 +236,44 @@ describe('resource', () => {
     expect(effectRuns).toBe(1);
   });
 
+  it('should not trigger consumers on every value change via hasValue()', async () => {
+    const testResource = resource<number | undefined, unknown>({
+      loader: () => Promise.resolve(undefined),
+      injector: TestBed.inject(Injector),
+    });
+
+    let effectRuns = 0;
+    effect(
+      () => {
+        testResource.hasValue();
+        effectRuns++;
+      },
+      {injector: TestBed.inject(Injector)},
+    );
+
+    TestBed.tick();
+    // Starts off without a value
+    expect(testResource.hasValue()).toBeFalse();
+    // Effect should run the first time.
+    expect(effectRuns).toBe(1);
+
+    // Set value to something defined
+    testResource.set(0);
+    TestBed.tick();
+    // Value is now defined
+    expect(testResource.hasValue()).toBeTrue();
+    // The effect should have been re-run.
+    expect(effectRuns).toBe(2);
+
+    // Set value to something else defined
+    testResource.set(1);
+    TestBed.tick();
+    // Value is still defined
+    expect(testResource.hasValue()).toBeTrue();
+    // The effect should not rerun.
+    expect(effectRuns).toBe(2);
+  });
+
   it('should update computed signals', async () => {
     const backend = new MockEchoBackend();
     const counter = signal(0);
