fix(core): drop mutate function from the signals public API (angular#51821)

pkozlowski-opensource · atscott · commit c7ff9dff2c14 · 2023-10-05T14:40:50.000-07:00
This change removes the `mutate` method from the `WritableSignal` interface and completely drops it from the public API surface. The initial API proposal for Angular signals included the mutate method, allowing in-place modification of JS objects, without changing their references (identity). This was based on the reasoning that identity change on modification is not necessary as we can send the “modified” notification through the signals graph. Unfortunately the signal-specific change notification is lost as soon as we read signal value outside of a reactive context (outside of a reactive graph). In other words - any code outside of the Angular signals library can’t know that an object is modified. Secondly, to make the mutate method work, we’ve defaulted the signal value equality function to the one that considers non-primitive values as always different. This is unfortunate for people working with immutable data structures (this is notably the case for the popular state management libraries) as the default equality function de-optimizes memoization in computed, making the application less performant. Given the above reasons we prefer to remove the mutate method in the signals library - at least for now. There are just too many sharp edges and tradeoffs that we don’t fully understand yet. BREAKING CHANGE: The `mutate` method was removed from the `WritableSignal` interface and completely dropped from the public API surface. As an alternative please use the update method and make immutable changes to the object. Example before: ```typescript items.mutate(itemsArray => itemsArray.push(newItem)); ``` Example after: ```typescript items.update(itemsArray => [itemsArray, …newItem]); ``` PR Close angular#51821
diff --git a/goldens/public-api/core/index.md b/goldens/public-api/core/index.md
@@ -1629,7 +1629,6 @@ export abstract class ViewRef extends ChangeDetectorRef {
 // @public
 export interface WritableSignal<T> extends Signal<T> {
     asReadonly(): Signal<T>;
-    mutate(mutatorFn: (value: T) => void): void;
     set(value: T): void;
     update(updateFn: (value: T) => T): void;
 }
diff --git a/packages/core/src/signals/src/api.ts b/packages/core/src/signals/src/api.ts
@@ -53,10 +53,5 @@ export type ValueEqualityFn<T> = (a: T, b: T) => boolean;
  * @developerPreview
  */
 export function defaultEquals<T>(a: T, b: T) {
-  // `Object.is` compares two values using identity semantics which is desired behavior for
-  // primitive values. If `Object.is` determines two values to be equal we need to make sure that
-  // those don't represent objects (we want to make sure that 2 objects are always considered
-  // "unequal"). The null check is needed for the special case of JavaScript reporting null values
-  // as objects (`typeof null === 'object'`).
-  return (a === null || typeof a !== 'object') && Object.is(a, b);
+  return Object.is(a, b);
 }
diff --git a/packages/core/src/signals/src/signal.ts b/packages/core/src/signals/src/signal.ts
@@ -35,12 +35,6 @@ export interface WritableSignal<T> extends Signal<T> {
    */
   update(updateFn: (value: T) => T): void;
 
-  /**
-   * Update the current value by mutating it in-place, and
-   * notify any dependents.
-   */
-  mutate(mutatorFn: (value: T) => void): void;
-
   /**
    * Returns a readonly version of this signal. Readonly signals can be accessed to read their value
    * but can't be changed using set, update or mutate methods. The readonly signals do _not_ have
@@ -79,7 +73,6 @@ export function signal<T>(initialValue: T, options?: CreateSignalOptions<T>): Wr
 
   signalFn.set = signalSetFn;
   signalFn.update = signalUpdateFn;
-  signalFn.mutate = signalMutateFn;
   signalFn.asReadonly = signalAsReadonlyFn;
   (signalFn as any)[SIGNAL] = node;
 
@@ -133,23 +126,9 @@ function signalSetFn<T>(this: SignalFn<T>, newValue: T) {
 }
 
 function signalUpdateFn<T>(this: SignalFn<T>, updater: (value: T) => T): void {
-  if (!producerUpdatesAllowed()) {
-    throwInvalidWriteToSignalError();
-  }
-
   signalSetFn.call(this as any, updater(this[SIGNAL].value) as any);
 }
 
-function signalMutateFn<T>(this: SignalFn<T>, mutator: (value: T) => void): void {
-  const node = this[SIGNAL];
-  if (!producerUpdatesAllowed()) {
-    throwInvalidWriteToSignalError();
-  }
-  // Mutate bypasses equality checks as it's by definition changing the value.
-  mutator(node.value);
-  signalValueChanged(node);
-}
-
 function signalAsReadonlyFn<T>(this: SignalFn<T>) {
   const node = this[SIGNAL];
   if (node.readonlyFn === undefined) {
diff --git a/packages/core/test/signals/signal_spec.ts b/packages/core/test/signals/signal_spec.ts
@@ -24,18 +24,6 @@ describe('signals', () => {
     expect(counter()).toEqual(1);
   });
 
-  it('should have mutate function for mutable, out of bound updates', () => {
-    const state = signal<string[]>(['a']);
-    const derived = computed(() => state().join(':'));
-
-    expect(derived()).toEqual('a');
-
-    state.mutate((s) => {
-      s.push('b');
-    });
-    expect(derived()).toEqual('a:b');
-  });
-
   it('should not update signal when new value is equal to the previous one', () => {
     const state = signal('aaa', {equal: (a, b) => a.length === b.length});
     expect(state()).toEqual('aaa');
@@ -72,31 +60,32 @@ describe('signals', () => {
     expect(upper()).toEqual('D');
   });
 
-  it('should consider objects as non-equal with the default equality function', () => {
-    let stateValue: unknown = {};
-    const state = signal(stateValue);
-    let computeCount = 0;
-    const derived = computed(() => `${typeof state()}:${++computeCount}`);
-    expect(derived()).toEqual('object:1');
-
-    // reset signal value to the same object instance, expect change notification
-    state.set(stateValue);
-    expect(derived()).toEqual('object:2');
-
-    // reset signal value to a different object instance, expect change notification
-    stateValue = {};
-    state.set(stateValue);
-    expect(derived()).toEqual('object:3');
-
-    // reset signal value to a different object type, expect change notification
-    stateValue = [];
-    state.set(stateValue);
-    expect(derived()).toEqual('object:4');
-
-    // reset signal value to the same array instance, expect change notification
-    state.set(stateValue);
-    expect(derived()).toEqual('object:5');
-  });
+  it('should consider objects as equal based on their identity with the default equality function',
+     () => {
+       let stateValue: unknown = {};
+       const state = signal(stateValue);
+       let computeCount = 0;
+       const derived = computed(() => `${typeof state()}:${++computeCount}`);
+       expect(derived()).toEqual('object:1');
+
+       // reset signal value to the same object instance, expect NO change notification
+       state.set(stateValue);
+       expect(derived()).toEqual('object:1');
+
+       // reset signal value to a different object instance, expect change notification
+       stateValue = {};
+       state.set(stateValue);
+       expect(derived()).toEqual('object:2');
+
+       // reset signal value to a different object type, expect change notification
+       stateValue = [];
+       state.set(stateValue);
+       expect(derived()).toEqual('object:3');
+
+       // reset signal value to the same array instance, expect NO change notification
+       state.set(stateValue);
+       expect(derived()).toEqual('object:3');
+     });
 
   it('should allow converting writable signals to their readonly counterpart', () => {
     const counter = signal(0);
@@ -106,8 +95,6 @@ describe('signals', () => {
     expect(readOnlyCounter.set).toBeUndefined();
     // @ts-expect-error
     expect(readOnlyCounter.update).toBeUndefined();
-    // @ts-expect-error
-    expect(readOnlyCounter.mutate).toBeUndefined();
 
     const double = computed(() => readOnlyCounter() * 2);
     expect(double()).toBe(0);
@@ -142,13 +129,6 @@ describe('signals', () => {
       expect(log).toBe(1);
     });
 
-    it('should call the post-signal-set fn when invoking .mutate()', () => {
-      const counter = signal(0);
-
-      counter.mutate(() => {});
-      expect(log).toBe(1);
-    });
-
     it('should not call the post-signal-set fn when the value doesn\'t change', () => {
       const counter = signal(0);
 

Original file line number	Diff line number	Diff line change
`@@ -1629,7 +1629,6 @@ export abstract class ViewRef extends ChangeDetectorRef {`
`1629`	`1629`	`// @public`
`1630`	`1630`	`export interface WritableSignal<T> extends Signal<T> {`
`1631`	`1631`	`asReadonly(): Signal<T>;`
`1632`		`- mutate(mutatorFn: (value: T) => void): void;`
`1633`	`1632`	`set(value: T): void;`
`1634`	`1633`	`update(updateFn: (value: T) => T): void;`
`1635`	`1634`	`}`