Skip to content
This repository has been archived by the owner on Nov 21, 2023. It is now read-only.

freeze state in signalState #2

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
92 changes: 0 additions & 92 deletions src/lib/signal-state.spec.ts

This file was deleted.

6 changes: 5 additions & 1 deletion src/lib/signal-state.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { signal, WritableSignal } from '@angular/core';
import { DeepSignal, toDeepSignal } from './deep-signal';
import { defaultEqualityFn } from './select-signal';
import { deepFreeze, isFunction, isObjectLike } from './utils';

export type SignalState<State extends Record<string, unknown>> =
DeepSignal<State> & SignalStateUpdate<State>;
Expand All @@ -16,6 +17,7 @@ export type SignalStateUpdate<State extends Record<string, unknown>> = {
export function signalState<State extends Record<string, unknown>>(
initialState: State
): SignalState<State> {
deepFreeze(initialState);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should perform deepFreeze only in dev mode for better performance

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still working on that

const stateSignal = signal(initialState, { equal: defaultEqualityFn });
const deepSignal = toDeepSignal(stateSignal.asReadonly());
(deepSignal as SignalState<State>).$update =
Expand All @@ -32,7 +34,9 @@ export function signalStateUpdateFactory<State extends Record<string, unknown>>(
updaters.reduce(
(currentState: State, updater) => ({
...currentState,
...(typeof updater === 'function' ? updater(currentState) : updater),
...(typeof updater === 'function'
? updater(deepFreeze(currentState))
: updater),
}),
state
)
Expand Down
40 changes: 40 additions & 0 deletions src/lib/utils.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
export function deepFreeze<T extends object>(target: T): T {
markostanimirovic marked this conversation as resolved.
Show resolved Hide resolved
Object.freeze(target);

const targetIsFunction = isFunction(target);

for (const prop of Object.getOwnPropertyNames(target)) {
// Do not freeze Ivy properties (e.g. router state)
// https://github.com/ngrx/platform/issues/2109#issuecomment-582689060
if (prop.startsWith('ɵ')) {
continue;
}

if (
Object.hasOwn(target, prop) &&
(targetIsFunction
? prop !== 'caller' && prop !== 'callee' && prop !== 'arguments'
: true)
) {
// @ts-ignore
const propValue = target[prop];
markostanimirovic marked this conversation as resolved.
Show resolved Hide resolved

if (
(isObjectLike(propValue) || isFunction(propValue)) &&
!Object.isFrozen(propValue)
) {
deepFreeze(propValue);
}
}
}

return target;
}

export function isFunction(target: unknown): target is () => unknown {
return typeof target === 'function';
}

export function isObjectLike(target: unknown): target is object {
return typeof target === 'object' && target !== null;
}
152 changes: 152 additions & 0 deletions src/tests/signal-state.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,152 @@
import { effect } from '@angular/core';
import { withEffect } from './with-effect';
import { signalState } from '../lib/signal-state';
import { selectSignal } from '../lib/select-signal';

describe('Signal State', () => {
const initialState = {
user: {
firstName: 'John',
lastName: 'Smith',
},
foo: 'bar',
numbers: [1, 2, 3],
};

const setup = () => signalState(initialState);

it('should support nested signals', () => {
const state = setup();

expect(state()).toBe(initialState);
expect(state.user()).toBe(initialState.user);
expect(state.user.firstName()).toBe(initialState.user.firstName);
});

it('should allow updates', () => {
const state = setup();
state.$update((state) => ({
...state,
user: { firstName: 'Johannes', lastName: 'Schmidt' },
}));
expect(state()).toEqual({
...initialState,
user: { firstName: 'Johannes', lastName: 'Schmidt' },
});
});

it('should update immutably', () => {
const state = setup();
state.$update((state) => ({
...state,
foo: 'bar',
numbers: [3, 2, 1],
}));
expect(state.user()).toBe(initialState.user);
expect(state.foo()).toBe(initialState.foo);
expect(state.numbers()).not.toBe(initialState.numbers);
});

describe('equal checks', () => {
it(
'should not fire unchanged signals on update',
withEffect((detectChanges) => {
const state = setup();

const numberEffect = jest.fn(() => state.numbers());
effect(numberEffect);

const userEffect = jest.fn(() => state.user());
effect(userEffect);

expect(numberEffect).toHaveBeenCalledTimes(0);
expect(userEffect).toHaveBeenCalledTimes(0);

// run effects for the first time
detectChanges();
expect(numberEffect).toHaveBeenCalledTimes(1);
expect(userEffect).toHaveBeenCalledTimes(1);

// update state with effect run
state.$update((state) => ({ ...state, numbers: [4, 5, 6] }));
detectChanges();
expect(numberEffect).toHaveBeenCalledTimes(2);
expect(userEffect).toHaveBeenCalledTimes(1);
})
);

it(
'should not fire for unchanged derived signals',
withEffect((detectChanges) => {
const state = setup();

const numberCount = selectSignal(
state,
(state) => state.numbers.length
);

const numberEffect = jest.fn(() => numberCount());
effect(numberEffect);

// run effects for the first time
detectChanges();
expect(numberEffect).toHaveBeenCalledTimes(1);

// update user
state.$update({
user: { firstName: 'Susanne', lastName: 'Taylor' },
});
detectChanges();
expect(numberEffect).toHaveBeenCalledTimes(1);

// update numbers
state.$update({ numbers: [1] });
detectChanges();
expect(numberEffect).toHaveBeenCalledTimes(2);
expect(numberCount()).toBe(1);
})
);
});

describe('immutability', () => {
it(
'should throw on mutable changes',
withEffect((detectChanges) => {
let numberCounter = 0;
const state = setup();
const effectFn = jest.fn(() => state.numbers());
effect(effectFn);
detectChanges();
expect(effectFn).toHaveBeenCalledTimes(1);

expect(() =>
state.$update((state) => {
const { numbers } = state;
numbers.push(4);
return { ...state };
})
).toThrow();
})
);

it(
'should throw on a single mutable change',
withEffect((detectChanges) => {
let numberCounter = 0;
const state = setup();
const effectFn = jest.fn(() => state.numbers());
effect(effectFn);
detectChanges();
expect(effectFn).toHaveBeenCalledTimes(1);

expect(() =>
state.$update({ foo: 'foobar' }, (state) => {
const { numbers } = state;
numbers.push(4);
return { ...state };
})
).toThrow();
})
);
});
});
1 change: 0 additions & 1 deletion src/tests/test2.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import {
selectSignal,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test1.ts and test2.ts files contain "type tests 😅", to make sure that typing works fine after refactoring. When we move this lib to the NgRx repo, they will be converted into real type tests. For now, they contain a few commented lines that use selectSignal so feel free to revert this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done. I've left testEffects there as it is more a testing file and not a test

signalStore,
signalStoreFeature,
type,
Expand Down
26 changes: 26 additions & 0 deletions src/tests/with-effect.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
import { Component } from '@angular/core';
import { TestBed } from '@angular/core/testing';

/**
* Testing function which executes inside of an injectionContext and provides Change Detection trigger.
*
* It looks like, we need to have at least one component to execute effects.
* AppRef::tick() does not work. Only ComponentFixture::detectChanges() seems to do the job.
* withEffects renders a TestComponent and executes the tests inside an injectionContext.
* This allows us to generate effects very easily.
*/
export const withEffect =
markostanimirovic marked this conversation as resolved.
Show resolved Hide resolved
(testFn: (detectChanges: () => void) => void): (() => void) =>
() => {
const fixture = TestBed.configureTestingModule({
imports: [TestComponent],
}).createComponent(TestComponent);

TestBed.runInInjectionContext(() => testFn(() => fixture.detectChanges()));
};

@Component({
template: '',
standalone: true,
})
class TestComponent {}