From 7a842095cc0a5f081772458e968151b2e4f1faba Mon Sep 17 00:00:00 2001 From: Rainer Hahnekamp Date: Mon, 18 Nov 2024 17:51:33 +0100 Subject: [PATCH] feat(signals): throw error in dev mode on state mutation (#4526) BREAKING CHANGES: The `signalState`/`signalStore` state object is frozen in development mode. If a mutable change occurs to the state object, an error will be thrown. BEFORE: const userState = signalState(initialState); patchState(userState, (state) => { state.user.firstName = 'mutable change'; // mutable change which went through return state; }); AFTER: const userState = signalState(initialState); patchState(userState, (state) => { state.user.firstName = 'mutable change'; // throws in dev mode return state; }); --- modules/signals/spec/deep-freeze.spec.ts | 115 +++++++++++++++++++++++ modules/signals/src/deep-freeze.ts | 49 ++++++++++ modules/signals/src/signal-state.ts | 3 +- modules/signals/src/state-source.ts | 10 +- modules/signals/src/with-state.ts | 11 ++- 5 files changed, 179 insertions(+), 9 deletions(-) create mode 100644 modules/signals/spec/deep-freeze.spec.ts create mode 100644 modules/signals/src/deep-freeze.ts diff --git a/modules/signals/spec/deep-freeze.spec.ts b/modules/signals/spec/deep-freeze.spec.ts new file mode 100644 index 0000000000..a432c94e25 --- /dev/null +++ b/modules/signals/spec/deep-freeze.spec.ts @@ -0,0 +1,115 @@ +import { getState, patchState } from '../src/state-source'; +import { signalState } from '../src/signal-state'; +import { signalStore } from '../src/signal-store'; +import { TestBed } from '@angular/core/testing'; +import { withState } from '../src/with-state'; + +describe('deepFreeze', () => { + const initialState = { + user: { + firstName: 'John', + lastName: 'Smith', + }, + foo: 'bar', + numbers: [1, 2, 3], + ngrx: 'signals', + }; + + for (const { stateFactory, name } of [ + { + name: 'signalStore', + stateFactory: () => { + const Store = signalStore( + { protectedState: false }, + withState(initialState) + ); + return TestBed.configureTestingModule({ providers: [Store] }).inject( + Store + ); + }, + }, + { name: 'signalState', stateFactory: () => signalState(initialState) }, + ]) { + describe(name, () => { + it(`throws on a mutable change`, () => { + const state = stateFactory(); + expect(() => + patchState(state, (state) => { + state.ngrx = 'mutable change'; + return state; + }) + ).toThrowError("Cannot assign to read only property 'ngrx' of object"); + }); + + it('throws on a nested mutable change', () => { + const state = stateFactory(); + expect(() => + patchState(state, (state) => { + state.user.firstName = 'mutable change'; + return state; + }) + ).toThrowError( + "Cannot assign to read only property 'firstName' of object" + ); + }); + describe('mutable changes outside of patchState', () => { + it('throws on reassigned a property of the exposed state', () => { + const state = stateFactory(); + expect(() => { + state.user().firstName = 'mutable change 1'; + }).toThrowError( + "Cannot assign to read only property 'firstName' of object" + ); + }); + + it('throws when exposed state via getState is mutated', () => { + const state = stateFactory(); + const s = getState(state); + + expect(() => (s.ngrx = 'mutable change 2')).toThrowError( + "Cannot assign to read only property 'ngrx' of object" + ); + }); + + it('throws when mutable change happens for', () => { + const state = stateFactory(); + const s = { user: { firstName: 'M', lastName: 'S' } }; + patchState(state, s); + + expect(() => { + s.user.firstName = 'mutable change 3'; + }).toThrowError( + "Cannot assign to read only property 'firstName' of object" + ); + }); + }); + }); + } + + describe('special tests', () => { + for (const { name, mutationFn } of [ + { + name: 'location', + mutationFn: (state: { location: { city: string } }) => + (state.location.city = 'Paris'), + }, + { + name: 'user', + mutationFn: (state: { user: { firstName: string } }) => + (state.user.firstName = 'Jane'), + }, + ]) { + it(`throws on concatenated state (${name})`, () => { + const UserStore = signalStore( + { providedIn: 'root' }, + withState(initialState), + withState({ location: { city: 'London' } }) + ); + const store = TestBed.inject(UserStore); + const state = getState(store); + + expect(() => mutationFn(state)).toThrowError(); + }); + } + }); +}); diff --git a/modules/signals/src/deep-freeze.ts b/modules/signals/src/deep-freeze.ts new file mode 100644 index 0000000000..be2e5773ee --- /dev/null +++ b/modules/signals/src/deep-freeze.ts @@ -0,0 +1,49 @@ +declare const ngDevMode: boolean; + +export function deepFreeze(target: T): T { + Object.freeze(target); + + const targetIsFunction = typeof target === 'function'; + + Object.getOwnPropertyNames(target).forEach((prop) => { + // Ignore Ivy properties, ref: https://github.com/ngrx/platform/issues/2109#issuecomment-582689060 + if (prop.startsWith('ɵ')) { + return; + } + + if ( + hasOwnProperty(target, prop) && + (targetIsFunction + ? prop !== 'caller' && prop !== 'callee' && prop !== 'arguments' + : true) + ) { + const propValue = target[prop]; + + if ( + (isObjectLike(propValue) || typeof propValue === 'function') && + !Object.isFrozen(propValue) + ) { + deepFreeze(propValue); + } + } + }); + + return target; +} + +export function freezeInDevMode(target: T): T { + return ngDevMode ? deepFreeze(target) : target; +} + +function hasOwnProperty( + target: unknown, + propertyName: string +): target is { [propertyName: string]: unknown } { + return isObjectLike(target) + ? Object.prototype.hasOwnProperty.call(target, propertyName) + : false; +} + +function isObjectLike(target: unknown): target is object { + return typeof target === 'object' && target !== null; +} diff --git a/modules/signals/src/signal-state.ts b/modules/signals/src/signal-state.ts index d31521151e..72f1e02674 100644 --- a/modules/signals/src/signal-state.ts +++ b/modules/signals/src/signal-state.ts @@ -1,6 +1,7 @@ import { signal } from '@angular/core'; import { STATE_SOURCE, WritableStateSource } from './state-source'; import { DeepSignal, toDeepSignal } from './deep-signal'; +import { freezeInDevMode } from './deep-freeze'; export type SignalState = DeepSignal & WritableStateSource; @@ -8,7 +9,7 @@ export type SignalState = DeepSignal & export function signalState( initialState: State ): SignalState { - const stateSource = signal(initialState as State); + const stateSource = signal(freezeInDevMode(initialState as State)); const signalState = toDeepSignal(stateSource.asReadonly()); Object.defineProperty(signalState, STATE_SOURCE, { value: stateSource, diff --git a/modules/signals/src/state-source.ts b/modules/signals/src/state-source.ts index 4db6ec3a68..4db0e38881 100644 --- a/modules/signals/src/state-source.ts +++ b/modules/signals/src/state-source.ts @@ -8,6 +8,7 @@ import { WritableSignal, } from '@angular/core'; import { Prettify } from './ts-helpers'; +import { freezeInDevMode } from './deep-freeze'; const STATE_WATCHERS = new WeakMap, Array>>(); @@ -37,10 +38,11 @@ export function patchState( ): void { stateSource[STATE_SOURCE].update((currentState) => updaters.reduce( - (nextState: State, updater) => ({ - ...nextState, - ...(typeof updater === 'function' ? updater(nextState) : updater), - }), + (nextState: State, updater) => + freezeInDevMode({ + ...nextState, + ...(typeof updater === 'function' ? updater(nextState) : updater), + }), currentState ) ); diff --git a/modules/signals/src/with-state.ts b/modules/signals/src/with-state.ts index 2d116a2395..5b3cf94d68 100644 --- a/modules/signals/src/with-state.ts +++ b/modules/signals/src/with-state.ts @@ -9,6 +9,7 @@ import { SignalStoreFeature, SignalStoreFeatureResult, } from './signal-store-models'; +import { freezeInDevMode } from './deep-freeze'; export function withState( stateFactory: () => State @@ -35,10 +36,12 @@ export function withState( assertUniqueStoreMembers(store, stateKeys); - store[STATE_SOURCE].update((currentState) => ({ - ...currentState, - ...state, - })); + store[STATE_SOURCE].update((currentState) => + freezeInDevMode({ + ...currentState, + ...state, + }) + ); const stateSignals = stateKeys.reduce((acc, key) => { const sliceSignal = computed(