fix(core): remove signal equality check short-circuit (#53446)

pkozlowski-opensource · alxhub · commit 42f4f70e97b4 · 2023-12-08T07:05:34.000-08:00
The PR #52465 introduced short-circuit for the signal equality invocation - with the reasoning that the equality function should never return false for arguments with the same references. In practice it turned out that it is rather surprising and the subsequent PR #52532 added a warning when the short-circuit was taking priority over the equality function. Still, the presence of the short-circuit prevents people from mutating objects in place and based on #52735 this is a common and desired scenario. This change removes the short-circuit altogether and thus fixes the mentioned issue. We do recognize that removing short-circuit exposes developers to the potentially surprising logic where mutated in-place change won't be propagated throug the reactivity graph (due to the deault equality function). But we assume that this might be less surprising / more desirable as compared to the short-circuit logic. Fixes #52735 PR Close #53446
diff --git a/packages/core/primitives/signals/src/signal.ts b/packages/core/primitives/signals/src/signal.ts
@@ -66,14 +66,7 @@ export function signalSetFn<T>(node: SignalNode<T>, newValue: T) {
     throwInvalidWriteToSignalError();
   }
 
-  const value = node.value;
-  if (Object.is(value, newValue)) {
-    if (typeof ngDevMode !== 'undefined' && ngDevMode && !node.equal(value, newValue)) {
-      console.warn(
-          'Signal value equality implementations should always return `true` for' +
-          ' values that are the same according to `Object.is` but returned `false` instead.');
-    }
-  } else if (!node.equal(value, newValue)) {
+  if (!node.equal(node.value, newValue)) {
     node.value = newValue;
     signalValueChanged(node);
   }
diff --git a/packages/core/test/signals/signal_spec.ts b/packages/core/test/signals/signal_spec.ts
@@ -88,7 +88,7 @@ describe('signals', () => {
        expect(derived()).toEqual('object:3');
      });
 
-  it('should consider referentially equal values as always equal', () => {
+  it('should invoke custom equality function even if old / new references are the same', () => {
     const state = {value: 0};
     const stateSignal = signal(state, {equal: (a, b) => false});
 
@@ -98,13 +98,13 @@ describe('signals', () => {
     // derived is re-computed initially
     expect(derived()).toBe('0:1');
 
-    // setting signal with the same reference should not propagate change
+    // setting signal with the same reference should propagate change due to the custom equality
     stateSignal.set(state);
-    expect(derived()).toBe('0:1');
+    expect(derived()).toBe('0:2');
 
-    // updating signal with the same reference should not propagate change
+    // updating signal with the same reference should propagate change as well
     stateSignal.update(state => state);
-    expect(derived()).toBe('0:1');
+    expect(derived()).toBe('0:3');
   });
 
   it('should allow converting writable signals to their readonly counterpart', () => {
