From c7ff9dff2c14aba70e92b9e216a2d4d97d6ef71e Mon Sep 17 00:00:00 2001 From: Pawel Kozlowski Date: Tue, 19 Sep 2023 13:40:43 +0200 Subject: [PATCH] fix(core): drop mutate function from the signals public API (#51821) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 #51821 --- goldens/public-api/core/index.md | 1 - packages/core/src/signals/src/api.ts | 7 +-- packages/core/src/signals/src/signal.ts | 21 -------- packages/core/test/signals/signal_spec.ts | 64 ++++++++--------------- 4 files changed, 23 insertions(+), 70 deletions(-) diff --git a/goldens/public-api/core/index.md b/goldens/public-api/core/index.md index 0d7cea177c7..191cec0c74b 100644 --- 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 extends Signal { asReadonly(): Signal; - 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 index be956b1b2ca..5a220c6d380 100644 --- a/packages/core/src/signals/src/api.ts +++ b/packages/core/src/signals/src/api.ts @@ -53,10 +53,5 @@ export type ValueEqualityFn = (a: T, b: T) => boolean; * @developerPreview */ export function defaultEquals(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 index e22d8788504..102ceb945ba 100644 --- a/packages/core/src/signals/src/signal.ts +++ b/packages/core/src/signals/src/signal.ts @@ -35,12 +35,6 @@ export interface WritableSignal extends Signal { */ 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(initialValue: T, options?: CreateSignalOptions): Wr signalFn.set = signalSetFn; signalFn.update = signalUpdateFn; - signalFn.mutate = signalMutateFn; signalFn.asReadonly = signalAsReadonlyFn; (signalFn as any)[SIGNAL] = node; @@ -133,23 +126,9 @@ function signalSetFn(this: SignalFn, newValue: T) { } function signalUpdateFn(this: SignalFn, updater: (value: T) => T): void { - if (!producerUpdatesAllowed()) { - throwInvalidWriteToSignalError(); - } - signalSetFn.call(this as any, updater(this[SIGNAL].value) as any); } -function signalMutateFn(this: SignalFn, 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(this: SignalFn) { 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 index d805b98d99a..51c837a6af0 100644 --- 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(['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'); + 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 change notification - state.set(stateValue); - expect(derived()).toEqual('object:2'); + // 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:3'); + // 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:4'); + // 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 change notification - state.set(stateValue); - expect(derived()).toEqual('object:5'); - }); + // 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);