Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(signals): enable withProps to handle Symbols #4656

Merged
Merged
Show file tree
Hide file tree
Changes from 5 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
37 changes: 36 additions & 1 deletion modules/signals/spec/deep-freeze.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@ import { TestBed } from '@angular/core/testing';
import { withState } from '../src/with-state';

describe('deepFreeze', () => {
const DIRECT_SECRET = Symbol('direct secret');
const SECRET = Symbol('secret');

const initialState = {
user: {
firstName: 'John',
Expand All @@ -13,6 +16,14 @@ describe('deepFreeze', () => {
foo: 'bar',
numbers: [1, 2, 3],
ngrx: 'signals',
[DIRECT_SECRET]: 'secret',
nestedSymbol: {
[SECRET]: 'another secret',
},
[SECRET]: {
code: 'secret',
value: '123',
},
};

for (const { stateFactory, name } of [
Expand Down Expand Up @@ -71,7 +82,7 @@ describe('deepFreeze', () => {
);
});

it('throws when mutable change happens for', () => {
it('throws when mutable change happens', () => {
const state = stateFactory();
const s = { user: { firstName: 'M', lastName: 'S' } };
patchState(state, s);
Expand All @@ -82,6 +93,30 @@ describe('deepFreeze', () => {
"Cannot assign to read only property 'firstName' of object"
);
});

describe('symbol', () => {
it('throws on a mutable change on property of a frozen symbol', () => {
const state = stateFactory();
const s = getState(state);

expect(() => {
s[SECRET].code = 'mutable change';
}).toThrowError(
"Cannot assign to read only property 'code' of object"
);
});

it('throws on a mutable change on nested symbol', () => {
const state = stateFactory();
const s = getState(state);

expect(() => {
s.nestedSymbol[SECRET] = 'mutable change';
}).toThrowError(
"Cannot assign to read only property 'Symbol(secret)' of object"
);
});
});
});
});
}
Expand Down
93 changes: 92 additions & 1 deletion modules/signals/spec/signal-store.spec.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,10 @@
import { inject, InjectionToken, isSignal, signal } from '@angular/core';
import {
computed,
inject,
InjectionToken,
isSignal,
signal,
} from '@angular/core';
import { TestBed } from '@angular/core/testing';
import {
patchState,
Expand Down Expand Up @@ -145,6 +151,14 @@ describe('signalStore', () => {

expect(store.foo()).toBe('foo');
});

it('can have symbols as keys as well', () => {
const SECRET = Symbol('SECRET');
const Store = signalStore(withState({ [SECRET]: 'bar' }));
const store = new Store();

expect(store[SECRET]()).toBe('bar');
});
});

describe('withProps', () => {
Expand Down Expand Up @@ -183,6 +197,50 @@ describe('signalStore', () => {

expect(store.foo).toBe('bar');
});

it('allows symbols as props', () => {
const SECRET = Symbol('SECRET');

const Store = signalStore(withProps(() => ({ [SECRET]: 'secret' })));
const store = TestBed.configureTestingModule({
providers: [Store],
}).inject(Store);

expect(store[SECRET]).toBe('secret');
});

it('allows numbers as props', () => {
const Store = signalStore(withProps(() => ({ 1: 'Number One' })));
const store = TestBed.configureTestingModule({
providers: [Store],
}).inject(Store);

expect(store[1]).toBe('Number One');
});

it('passes on a symbol to the features', () => {
const SECRET = Symbol('SECRET');
const SecretStore = signalStore(
{ providedIn: 'root' },
withProps(() => ({
[SECRET]: 'not your business',
})),
withMethods((store) => ({
reveil() {
return store[SECRET];
},
})),
withComputed((state) => ({
secret: computed(() => state[SECRET]),
}))
);

const secretStore = TestBed.inject(SecretStore);

expect(secretStore.reveil()).toBe('not your business');
expect(secretStore.secret()).toBe('not your business');
expect(secretStore[SECRET]).toBe('not your business');
});
});

describe('withComputed', () => {
Expand Down Expand Up @@ -221,6 +279,26 @@ describe('signalStore', () => {

expect(store.bar()).toBe('bar');
});

it('can also expose a symbol', () => {
const SECRET = Symbol('SECRET');
const SecretStore = signalStore(
{ providedIn: 'root' },
withComputed(() => ({
[SECRET]: computed(() => 'secret'),
})),
withMethods((store) => ({
reveil() {
return store[SECRET];
},
}))
);

const secretStore = TestBed.inject(SecretStore);
const secretSignal = secretStore.reveil();

expect(secretSignal()).toBe('secret');
});
});

describe('withMethods', () => {
Expand Down Expand Up @@ -263,6 +341,19 @@ describe('signalStore', () => {

expect(store.baz()).toBe('baz');
});

it('can also expose a symbol', () => {
const SECRET = Symbol('SECRET');
const SecretStore = signalStore(
{ providedIn: 'root' },
withMethods(() => ({
[SECRET]: () => 'my secret',
}))
);
const secretStore = TestBed.inject(SecretStore);

expect(secretStore[SECRET]()).toBe('my secret');
});
});

describe('withHooks', () => {
Expand Down
10 changes: 10 additions & 0 deletions modules/signals/spec/state-source.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ import {
import { STATE_SOURCE } from '../src/state-source';
import { createLocalService } from './helpers';

const SECRET = Symbol('SECRET');

describe('StateSource', () => {
const initialState = {
user: {
Expand All @@ -27,6 +29,7 @@ describe('StateSource', () => {
foo: 'bar',
numbers: [1, 2, 3],
ngrx: 'signals',
[SECRET]: 'secret',
};

describe('patchState', () => {
Expand Down Expand Up @@ -78,6 +81,13 @@ describe('StateSource', () => {
});
});

it('patches property with symbol keys', () => {
const state = stateFactory();

patchState(state, { [SECRET]: 'another secret' });
expect(state[SECRET]()).toBe('another secret');
});

it('patches state via sequence of partial state objects and updater functions', () => {
const state = stateFactory();

Expand Down
8 changes: 7 additions & 1 deletion modules/signals/spec/with-methods.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,18 @@ describe('withMethods', () => {
});

it('logs warning if previously defined signal store members have the same name', () => {
const STATE_SECRET = Symbol('state_secret');
const COMPUTED_SECRET = Symbol('computed_secret');
const initialStore = [
withState({
p1: 'p1',
p2: false,
[STATE_SECRET]: 1,
}),
withComputed(() => ({
s1: signal(true).asReadonly(),
s2: signal({ s: 2 }).asReadonly(),
[COMPUTED_SECRET]: signal(1).asReadonly(),
})),
withMethods(() => ({
m1() {},
Expand All @@ -43,12 +47,14 @@ describe('withMethods', () => {
s1: () => 100,
m2,
m3: () => 'm3',
[STATE_SECRET]() {},
[COMPUTED_SECRET]() {},
}))(initialStore);

expect(console.warn).toHaveBeenCalledWith(
'@ngrx/signals: SignalStore members cannot be overridden.',
'Trying to override:',
'p2, s1, m2'
'p2, s1, m2, Symbol(state_secret), Symbol(computed_secret)'
);
});
});
13 changes: 6 additions & 7 deletions modules/signals/src/deep-freeze.ts
Original file line number Diff line number Diff line change
@@ -1,13 +1,12 @@
declare const ngDevMode: boolean;

export function deepFreeze<T>(target: T): T {
export function deepFreeze<T extends object>(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('ɵ')) {
Reflect.ownKeys(target).forEach((prop) => {
if (String(prop).startsWith('ɵ')) {
return;
}

Expand All @@ -31,14 +30,14 @@ export function deepFreeze<T>(target: T): T {
return target;
}

export function freezeInDevMode<T>(target: T): T {
export function freezeInDevMode<T extends object>(target: T): T {
return ngDevMode ? deepFreeze(target) : target;
}

function hasOwnProperty(
target: unknown,
propertyName: string
): target is { [propertyName: string]: unknown } {
propertyName: string | symbol
): target is { [propertyName: string | symbol]: unknown } {
return isObjectLike(target)
? Object.prototype.hasOwnProperty.call(target, propertyName)
: false;
Expand Down
6 changes: 3 additions & 3 deletions modules/signals/src/signal-store-assertions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ declare const ngDevMode: unknown;

export function assertUniqueStoreMembers(
store: InnerSignalStore,
newMemberKeys: string[]
newMemberKeys: (string | symbol)[]
): void {
if (!ngDevMode) {
return;
Expand All @@ -15,15 +15,15 @@ export function assertUniqueStoreMembers(
...store.props,
...store.methods,
};
const overriddenKeys = Object.keys(storeMembers).filter((memberKey) =>
const overriddenKeys = Reflect.ownKeys(storeMembers).filter((memberKey) =>
newMemberKeys.includes(memberKey)
);

if (overriddenKeys.length > 0) {
console.warn(
'@ngrx/signals: SignalStore members cannot be overridden.',
'Trying to override:',
overriddenKeys.join(', ')
overriddenKeys.map((key) => String(key)).join(', ')
);
}
}
4 changes: 2 additions & 2 deletions modules/signals/src/signal-store-models.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,9 @@ export type StateSignals<State> = IsKnownRecord<Prettify<State>> extends true
}
: {};

export type SignalsDictionary = Record<string, Signal<unknown>>;
export type SignalsDictionary = Record<string | symbol, Signal<unknown>>;

export type MethodsDictionary = Record<string, Function>;
export type MethodsDictionary = Record<string | symbol, Function>;

export type SignalStoreHooks = {
onInit?: () => void;
Expand Down
8 changes: 6 additions & 2 deletions modules/signals/src/signal-store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1355,11 +1355,15 @@ export function signalStore(
getInitialInnerStore()
);
const { stateSignals, props, methods, hooks } = innerStore;
const storeMembers = { ...stateSignals, ...props, ...methods };
const storeMembers: Record<string | symbol, unknown> = {
...stateSignals,
...props,
...methods,
};

(this as any)[STATE_SOURCE] = innerStore[STATE_SOURCE];

for (const key in storeMembers) {
for (const key of Reflect.ownKeys(storeMembers)) {
(this as any)[key] = storeMembers[key];
}

Expand Down
2 changes: 1 addition & 1 deletion modules/signals/src/with-methods.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ export function withMethods<
...store.props,
...store.methods,
});
assertUniqueStoreMembers(store, Object.keys(methods));
assertUniqueStoreMembers(store, Reflect.ownKeys(methods));

return {
...store,
Expand Down
4 changes: 2 additions & 2 deletions modules/signals/src/with-state.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ export function withState<State extends object>(
return (store) => {
const state =
typeof stateOrFactory === 'function' ? stateOrFactory() : stateOrFactory;
const stateKeys = Object.keys(state);
const stateKeys = Reflect.ownKeys(state);

assertUniqueStoreMembers(store, stateKeys);

Expand All @@ -45,7 +45,7 @@ export function withState<State extends object>(

const stateSignals = stateKeys.reduce((acc, key) => {
const sliceSignal = computed(
() => (store[STATE_SOURCE]() as Record<string, unknown>)[key]
() => (store[STATE_SOURCE]() as Record<string | symbol, unknown>)[key]
);
return { ...acc, [key]: toDeepSignal(sliceSignal) };
}, {} as SignalsDictionary);
Expand Down
Loading