Skip to content

Commit

Permalink
fix(signals): remove state checks for better DX (#4124)
Browse files Browse the repository at this point in the history
  • Loading branch information
markostanimirovic committed Nov 13, 2023
1 parent 18227cc commit 5749543
Show file tree
Hide file tree
Showing 9 changed files with 155 additions and 148 deletions.
2 changes: 1 addition & 1 deletion modules/signals/entities/src/with-entities.ts
Expand Up @@ -54,7 +54,7 @@ export function withEntities<Entity>(config?: {
withState({
[entityMapKey]: {},
[idsKey]: [],
} as any),
}),
withComputed((store) => ({
[entitiesKey]: computed(() => {
const entityMap = store[entityMapKey]() as EntityMap<Entity>;
Expand Down
23 changes: 21 additions & 2 deletions modules/signals/spec/signal-state.spec.ts
@@ -1,8 +1,8 @@
import { effect, isSignal } from '@angular/core';
import * as angular from '@angular/core';
import { TestBed } from '@angular/core/testing';
import { patchState, signalState } from '../src';
import { STATE_SIGNAL } from '../src/signal-state';
import { TestBed } from '@angular/core/testing';

describe('signalState', () => {
const initialState = {
Expand Down Expand Up @@ -85,6 +85,25 @@ describe('signalState', () => {
expect((state[STATE_SIGNAL] as any).ngrx).toBe(undefined);
});

it('overrides Function properties if state keys have the same name', () => {
const initialState = { name: { length: { length: 'ngrx' }, name: 20 } };
const state = signalState(initialState);

expect(state()).toBe(initialState);

expect(state.name()).toBe(initialState.name);
expect(isSignal(state.name)).toBe(true);

expect(state.name.name()).toBe(20);
expect(isSignal(state.name.name)).toBe(true);

expect(state.name.length()).toBe(initialState.name.length);
expect(isSignal(state.name.length)).toBe(true);

expect(state.name.length.length()).toBe('ngrx');
expect(isSignal(state.name.length.length)).toBe(true);
});

it('emits new values only for affected signals', () => {
TestBed.runInInjectionContext(() => {
const state = signalState(initialState);
Expand Down Expand Up @@ -144,7 +163,7 @@ describe('signalState', () => {
});
});

it('should not emit if there was no change', () =>
it('does not emit if there was no change', () =>
TestBed.runInInjectionContext(() => {
let stateCounter = 0;
let userCounter = 0;
Expand Down
30 changes: 30 additions & 0 deletions modules/signals/spec/signal-store.spec.ts
@@ -1,6 +1,7 @@
import { inject, InjectionToken, isSignal, signal } from '@angular/core';
import { TestBed } from '@angular/core/testing';
import {
patchState,
signalStore,
withComputed,
withHooks,
Expand Down Expand Up @@ -70,6 +71,35 @@ describe('signalStore', () => {
expect(store.x.y.z()).toBe(10);
});

it('overrides Function properties if nested state keys have the same name', () => {
const Store = signalStore(
withState({ name: { length: { name: false } } })
);
const store = new Store();

expect(store.name()).toEqual({ length: { name: false } });
expect(isSignal(store.name)).toBe(true);

expect(store.name.length()).toEqual({ name: false });
expect(isSignal(store.name.length)).toBe(true);

expect(store.name.length.name()).toBe(false);
expect(isSignal(store.name.length.name)).toBe(true);
});

it('does not create signals for optional state slices without initial value', () => {
type State = { x?: number; y?: { z: number } };

const Store = signalStore(withState<State>({ x: 10 }));
const store = new Store();

expect(store.x!()).toBe(10);
expect(store.y).toBe(undefined);

patchState(store, { y: { z: 100 } });
expect(store.y).toBe(undefined);
});

it('executes withState factory in injection context', () => {
const TOKEN = new InjectionToken('TOKEN', {
providedIn: 'root',
Expand Down
66 changes: 21 additions & 45 deletions modules/signals/spec/types/signal-state.types.spec.ts
Expand Up @@ -169,7 +169,7 @@ describe('signalState', () => {

expectSnippet(snippet).toInfer(
'state1Keys',
'unique symbol | unique symbol'
'unique symbol | keyof Signal<{ [key: string]: number; }>'
);

expectSnippet(snippet).toInfer(
Expand All @@ -179,7 +179,7 @@ describe('signalState', () => {

expectSnippet(snippet).toInfer(
'state2Keys',
'unique symbol | unique symbol'
'unique symbol | keyof Signal<{ [key: number]: { foo: string; }; }>'
);

expectSnippet(snippet).toInfer(
Expand All @@ -189,7 +189,7 @@ describe('signalState', () => {

expectSnippet(snippet).toInfer(
'state3Keys',
'unique symbol | unique symbol'
'unique symbol | keyof Signal<Record<string, { bar: number; }>>'
);

expectSnippet(snippet).toInfer(
Expand Down Expand Up @@ -270,50 +270,26 @@ describe('signalState', () => {
expectSnippet(snippet).toInfer('z', 'Signal<boolean | undefined>');
});

it('fails when state contains Function properties', () => {
expectSnippet(`const state = signalState({ name: '' })`).toFail(
/@ngrx\/signals: signal state cannot contain `Function` property or method names/
);

expectSnippet(
`const state = signalState({ foo: { arguments: [] } })`
).toFail(
/@ngrx\/signals: signal state cannot contain `Function` property or method names/
);

expectSnippet(`
type State = { foo: { bar: { call?: boolean }; baz: number } };
const state = signalState<State>({ foo: { bar: {}, baz: 1 } });
`).toFail(
/@ngrx\/signals: signal state cannot contain `Function` property or method names/
);

expectSnippet(
`const state = signalState({ foo: { apply: 'apply', bar: true } })`
).toFail(
/@ngrx\/signals: signal state cannot contain `Function` property or method names/
);

expectSnippet(`
type State = { bind?: { foo: string } };
const state = signalState<State>({ bind: { foo: 'bar' } });
`).toFail(
/@ngrx\/signals: signal state cannot contain `Function` property or method names/
);

expectSnippet(
`const state = signalState({ foo: { bar: { prototype: [] }; baz: 1 } })`
).toFail(
/@ngrx\/signals: signal state cannot contain `Function` property or method names/
);

expectSnippet(`const state = signalState({ foo: { length: 10 } })`).toFail(
/@ngrx\/signals: signal state cannot contain `Function` property or method names/
);
it('succeeds when state contains Function properties', () => {
const snippet = `
const state1 = signalState({ name: 0 });
const state2 = signalState({ foo: { length: [] as boolean[] } });
const state3 = signalState({ name: { length: '' } });
const name = state1.name;
const length1 = state2.foo.length;
const name2 = state3.name;
const length2 = state3.name.length;
`;

expectSnippet(`const state = signalState({ caller: '' })`).toFail(
/@ngrx\/signals: signal state cannot contain `Function` property or method names/
expectSnippet(snippet).toSucceed();
expectSnippet(snippet).toInfer('name', 'Signal<number>');
expectSnippet(snippet).toInfer('length1', 'Signal<boolean[]>');
expectSnippet(snippet).toInfer(
'name2',
'Signal<{ length: string; }> & Readonly<{ length: Signal<string>; }>'
);
expectSnippet(snippet).toInfer('length2', 'Signal<string>');
});

it('fails when state is not an object', () => {
Expand Down
126 changes: 62 additions & 64 deletions modules/signals/spec/types/signal-store.types.spec.ts
Expand Up @@ -253,62 +253,33 @@ describe('signalStore', () => {
);
});

it('fails when nested state slices contain Function properties', () => {
expectSnippet(`
const Store = signalStore(withState({ x: { name?: '' } }));
`).toFail(
/@ngrx\/signals: nested state slices cannot contain `Function` property or method names/
);

expectSnippet(`
const Store = signalStore(withState({ x: { arguments: [] } }));
`).toFail(
/@ngrx\/signals: nested state slices cannot contain `Function` property or method names/
);

expectSnippet(`
const Store = signalStore(
withState({ x: { bar: { call: false }, baz: 1 } })
);
`).toFail(
/@ngrx\/signals: nested state slices cannot contain `Function` property or method names/
);

expectSnippet(`
const Store = signalStore(
withState({ x: { apply: 'apply', bar: true } })
)
`).toFail(
/@ngrx\/signals: nested state slices cannot contain `Function` property or method names/
);

expectSnippet(`
const Store = signalStore(
withState({ x: { bind: { foo: 'bar' } } })
);
`).toFail(
/@ngrx\/signals: nested state slices cannot contain `Function` property or method names/
it('succeeds when nested state slices contain Function properties', () => {
const snippet1 = `
type State = { x: { name?: string } };
const Store = signalStore(withState<State>({ x: { name: '' } }));
const store = new Store();
const name = store.x.name;
`;
expectSnippet(snippet1).toSucceed();
expectSnippet(snippet1).toInfer(
'name',
'Signal<string | undefined> | undefined'
);

expectSnippet(`
const snippet2 = `
const Store = signalStore(
withState({ x: { bar: { prototype: [] }; baz: 1 } })
withState({ x: { length: { name: false }, baz: 1 } })
);
`).toFail(
/@ngrx\/signals: nested state slices cannot contain `Function` property or method names/
);

expectSnippet(`
const Store = signalStore(withState({ x: { length: 10 } }));
`).toFail(
/@ngrx\/signals: nested state slices cannot contain `Function` property or method names/
);

expectSnippet(`
const Store = signalStore(withState({ x: { caller: '' } }));
`).toFail(
/@ngrx\/signals: nested state slices cannot contain `Function` property or method names/
const store = new Store();
const length = store.x.length;
const name = store.x.length.name;
`;
expectSnippet(snippet2).toSucceed();
expectSnippet(snippet2).toInfer(
'length',
'Signal<{ name: boolean; }> & Readonly<{ name: Signal<boolean>; }>'
);
expectSnippet(snippet2).toInfer('name', 'Signal<boolean>');
});

it('succeeds when nested state slices are optional', () => {
Expand Down Expand Up @@ -355,8 +326,8 @@ describe('signalStore', () => {
);
});

it('fails when root state slices are optional', () => {
expectSnippet(`
it('succeeds when root state slices are optional', () => {
const snippet = `
type State = {
foo?: { s: string };
bar: number;
Expand All @@ -365,26 +336,53 @@ describe('signalStore', () => {
const Store = signalStore(
withState<State>({ foo: { s: '' }, bar: 1 })
);
`).toFail(/@ngrx\/signals: root state slices cannot be optional/);
const store = new Store();
const foo = store.foo;
`;

expectSnippet(snippet).toSucceed();
expectSnippet(snippet).toInfer(
'foo',
'Signal<{ s: string; } | undefined> | undefined'
);
});

it('fails when state is an unknown record', () => {
expectSnippet(`
const Store1 = signalStore(withState<{ [key: string]: number }>({}));
`).toFail(/@ngrx\/signals: root state keys must be string literals/);
it('succeeds when state is an unknown record', () => {
const snippet1 = `
const Store = signalStore(withState<{ [key: string]: number }>({}));
const store = new Store();
expectSnippet(`
const Store2 = signalStore(withState<{ [key: number]: { bar: string } }>({}));
`).toFail(/@ngrx\/signals: root state keys must be string literals/);
const x = store.x;
const y = store.y;
`;
expectSnippet(snippet1).toSucceed();
expectSnippet(snippet1).toInfer('x', 'Signal<number>');
expectSnippet(snippet1).toInfer('y', 'Signal<number>');

expectSnippet(`
const Store3 = signalStore(
const snippet2 = `
const Store = signalStore(
withState<{ [key: number]: { bar: string } }>({})
);
const store = new Store();
const x = store[0];
const y = store[1];
`;
expectSnippet(snippet2).toSucceed();
expectSnippet(snippet2).toInfer('x', 'DeepSignal<{ bar: string; }>');
expectSnippet(snippet2).toInfer('y', 'DeepSignal<{ bar: string; }>');

const snippet3 = `
const Store = signalStore(
withState<Record<string, { foo: boolean } | number>>({
x: { foo: true },
y: 1,
})
);
`).toFail(/@ngrx\/signals: root state keys must be string literals/);
const store = new Store();
const m = store.m;
`;
expectSnippet(snippet3).toSucceed();
expectSnippet(snippet3).toInfer('m', 'Signal<number | { foo: boolean; }>');
});

it('fails when state is not an object', () => {
Expand Down
20 changes: 17 additions & 3 deletions modules/signals/src/deep-signal.ts
@@ -1,6 +1,18 @@
import { computed, Signal, untracked } from '@angular/core';
import {
computed,
isSignal,
Signal as NgSignal,
untracked,
} from '@angular/core';
import { IsUnknownRecord } from './ts-helpers';

// An extended Signal type that enables the correct typing
// of nested signals with the `name' or `length' key.
interface Signal<T> extends NgSignal<T> {
name: unknown;
length: unknown;
}

export type DeepSignal<T> = Signal<T> &
(T extends Record<string, unknown>
? IsUnknownRecord<T> extends true
Expand All @@ -26,8 +38,10 @@ export function toDeepSignal<T>(signal: Signal<T>): DeepSignal<T> {
return target[prop];
}

if (!target[prop]) {
target[prop] = computed(() => target()[prop]);
if (!isSignal(target[prop])) {
Object.defineProperty(target, prop, {
value: computed(() => target()[prop]),
});
}

return toDeepSignal(target[prop]);
Expand Down

0 comments on commit 5749543

Please sign in to comment.