Skip to content
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
56 changes: 56 additions & 0 deletions modules/component-store/spec/types/component-store.types.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -273,5 +273,61 @@ describe('ComponentStore types', () => {
);
});
});

describe('catches excess properties', () => {
it('when extra property is returned with spread', () => {
expectSnippet(
`componentStore.updater((state, v: string) => ({...state, extraProp: 'bad'}))('test');`
).toFail(/Remove excess properties/);
});

it('when extra property is returned with explicit object', () => {
expectSnippet(
`componentStore.updater((state, v: string) => ({ prop: v, prop2: state.prop2, extraProp: 'bad' }))('test');`
).toFail(/Remove excess properties/);
});

it('when extra property is returned from void updater', () => {
expectSnippet(
`componentStore.updater((state) => ({...state, extraProp: true}))();`
).toFail(/Remove excess properties/);
});

it('when required property is missing', () => {
expectSnippet(
`componentStore.updater((state, v: string) => ({ prop: v }))('test');`
).toFail(/is missing in type/);
});

it('when property has wrong type', () => {
expectSnippet(
`componentStore.updater((state, v: string) => ({...state, prop: 123}))('test');`
).toFail(/not assignable to type/);
});

it('allows spread with override', () => {
expectSnippet(
`const sub = componentStore.updater((state, v: string) => ({...state, prop: v}))('test');`
).toInfer('sub', 'Subscription');
});

it('allows full explicit return matching all state keys', () => {
expectSnippet(
`const sub = componentStore.updater((state, v: string) => ({ prop: v, prop2: state.prop2 }))('test');`
).toInfer('sub', 'Subscription');
});

it('allows void updater with spread return', () => {
expectSnippet(
`const v = componentStore.updater((state) => ({...state, prop: 'updated'}))();`
).toInfer('v', 'void');
});

it('allows direct state return', () => {
expectSnippet(
`const v = componentStore.updater((state) => state)();`
).toInfer('v', 'void');
});
});
});
});
23 changes: 23 additions & 0 deletions modules/component-store/spec/types/regression.types.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,4 +41,27 @@ describe('regression component-store', () => {
`;
expectSnippet(effectTest).toSucceed();
});

describe('updater exact return type', () => {
it('should work with state containing optional properties', () => {
expectSnippet(`
const store = new ComponentStore<{ req: string; opt?: number }>({ req: 'a' });
store.updater((state) => ({ req: 'b' }))();
`).toSucceed();
});

it('should work with state containing index signature', () => {
expectSnippet(`
const store = new ComponentStore<{ [key: string]: number }>({});
store.updater((state, v: number) => ({...state, newKey: v}))(5);
`).toSucceed();
});

it('should catch excess properties with concrete state type', () => {
expectSnippet(`
const store = new ComponentStore<{ name: string }>({ name: 'test' });
store.updater((state, v: string) => ({...state, name: v, extra: true}))('test');
`).toFail(/Remove excess properties/);
});
});
});
21 changes: 17 additions & 4 deletions modules/component-store/src/component-store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,10 @@ import {
import { isOnStateInitDefined, isOnStoreInitDefined } from './lifecycle_hooks';
import { toSignal } from '@angular/core/rxjs-interop';

const excessPropertiesAreNotAllowedMsg =
'updater callback return type must exactly match the state type. Remove excess properties.';
type ExcessPropertiesAreNotAllowed = typeof excessPropertiesAreNotAllowedMsg;

export interface SelectConfig<T = unknown> {
debounce?: boolean;
equal?: ValueEqualityFn<T>;
Expand Down Expand Up @@ -132,7 +136,17 @@ export class ComponentStore<T extends object> implements OnDestroy {
ReturnType = OriginType extends void
? () => void
: (observableOrValue: ValueType | Observable<ValueType>) => Subscription,
>(updaterFn: (state: T, value: OriginType) => T): ReturnType {
// Captures the actual return type to enforce exact state shape
R extends T = T,
>(
updaterFn: (
state: T,
value: OriginType
) => R &
(Exclude<keyof R, keyof T> extends never
? unknown
: ExcessPropertiesAreNotAllowed)
): ReturnType {
return ((
observableOrValue?: OriginType | Observable<OriginType>
): Subscription => {
Expand Down Expand Up @@ -379,9 +393,8 @@ export class ComponentStore<T extends object> implements OnDestroy {
// This type quickly became part of effect 'API'
ProvidedType = void,
// The actual origin$ type, which could be unknown, when not specified
OriginType extends
| Observable<ProvidedType>
| unknown = Observable<ProvidedType>,
OriginType extends Observable<ProvidedType> | unknown =
Observable<ProvidedType>,
// Unwrapped actual type of the origin$ Observable, after default was applied
ObservableType = OriginType extends Observable<infer A> ? A : never,
// Return either an optional callback or a function requiring specific types as inputs
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,10 @@ export default createRule<Options, MessageIds>({
name: path.parse(__filename).name,
meta: {
type: 'problem',
deprecated: true,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is there a reason/advantage to deprecate the rules instead of removing them?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I suppose they actually don't have value when the return types are no longer needed. So might as well just remove.

docs: {
description: '`Updater` should have an explicit return type.',
description:
'`Updater` should have an explicit return type. Deprecated: `ComponentStore.updater` now enforces exact return types at the type level.',
ngrxModule: 'component-store',
},
schema: [],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,11 @@ export default createRule<Options, MessageIds>({
name: path.parse(__filename).name,
meta: {
type: 'suggestion',
deprecated: true,
hasSuggestions: true,
docs: {
description: '`On` function should have an explicit return type.',
description:
'`On` function should have an explicit return type. Deprecated: The `on` function now enforces exact return types at the type level.',
ngrxModule: 'store',
},
schema: [],
Expand Down
216 changes: 210 additions & 6 deletions modules/store/spec/types/reducer_creator.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -99,9 +99,7 @@ describe('createReducer()', () => {
`).toInfer(
'onFn',
`
ReducerTypes<{
name: string;
}, [ActionCreator<"FOO", (props: {
ReducerTypes<State, [ActionCreator<"FOO", (props: {
foo: string;
}) => {
foo: string;
Expand All @@ -118,11 +116,217 @@ describe('createReducer()', () => {
`).toInfer(
'onFn',
`
ReducerTypes<{
name: string;
}, [ActionCreator<"FOO", () => Action<"FOO">>]>
ReducerTypes<State, [ActionCreator<"FOO", () => Action<"FOO">>]>
`
);
});

describe('valid patterns', () => {
it('should allow spread with property override inside createReducer', () => {
expectSnippet(`
interface State { name: string; count: number };
const initialState: State = { name: 'test', count: 0 };
const setName = createAction('setName', props<{ name: string }>());

const reducer = createReducer(
initialState,
on(setName, (state, { name }) => ({ ...state, name })),
);
`).toSucceed();
});

it('should allow returning initialState inside createReducer', () => {
expectSnippet(`
interface State { name: string; count: number };
const initialState: State = { name: 'test', count: 0 };
const reset = createAction('reset');

const reducer = createReducer(
initialState,
on(reset, () => initialState),
);
`).toSucceed();
});

it('should allow returning state directly inside createReducer', () => {
expectSnippet(`
interface State { name: string; count: number };
const initialState: State = { name: 'test', count: 0 };
const noop = createAction('noop');

const reducer = createReducer(
initialState,
on(noop, (state) => state),
);
`).toSucceed();
});

it('should allow standalone on() with explicit state type', () => {
expectSnippet(`
interface State { name: string; count: number };
const setName = createAction('setName', props<{ name: string }>());

const onFn = on(setName, (state: State, { name }) => ({ ...state, name }));
`).toSucceed();
});

it('should allow explicit return of all properties inside createReducer', () => {
expectSnippet(`
interface State { name: string; count: number };
const initialState: State = { name: 'test', count: 0 };
const setName = createAction('setName', props<{ name: string }>());

const reducer = createReducer(
initialState,
on(setName, (state, { name }) => ({ name, count: state.count })),
);
`).toSucceed();
});

it('should allow on() with multiple action creators', () => {
expectSnippet(`
interface State { name: string; count: number };
const initialState: State = { name: 'test', count: 0 };
const action1 = createAction('action1');
const action2 = createAction('action2');

const reducer = createReducer(
initialState,
on(action1, action2, (state) => ({ ...state, count: state.count + 1 })),
);
`).toSucceed();
});
});

describe('catches excess properties', () => {
it('should catch excess properties in on() callback inside createReducer', () => {
expectSnippet(`
interface State { name: string; count: number };
const initialState: State = { name: 'test', count: 0 };
const setName = createAction('setName', props<{ name: string }>());

const reducer = createReducer(
initialState,
on(setName, (state, { name }) => ({ ...state, name, extra: true })),
);
`).toFail(/Remove excess properties/);
});

it('should catch excess properties in standalone on() with explicit state type', () => {
expectSnippet(`
interface State { name: string; count: number };
const setName = createAction('setName', props<{ name: string }>());

const onFn = on(setName, (state: State, { name }) => ({ ...state, name, extra: true }));
`).toFail(/Remove excess properties/);
});

it('should catch excess properties when returning explicit object', () => {
expectSnippet(`
interface State { name: string; count: number };
const initialState: State = { name: 'test', count: 0 };
const setName = createAction('setName', props<{ name: string }>());

const reducer = createReducer(
initialState,
on(setName, (state, { name }) => ({ name, count: state.count, extra: 'bad' })),
);
`).toFail(/Remove excess properties/);
});

it('should catch excess properties when explicit return type annotation is used', () => {
expectSnippet(`
interface State { name: string; count: number };
const initialState: State = { name: 'test', count: 0 };
const setName = createAction('setName', props<{ name: string }>());

const reducer = createReducer(
initialState,
on(setName, (state, { name }): State => ({ ...state, name, extra: true })),
);
`).toFail(/does not exist in type/);
});

it('should catch excess properties from void on() callback', () => {
expectSnippet(`
interface State { name: string; count: number };
const initialState: State = { name: 'test', count: 0 };
const noop = createAction('noop');

const reducer = createReducer(
initialState,
on(noop, (state) => ({ ...state, extra: true })),
);
`).toFail(/Remove excess properties/);
});
});

describe('edge cases', () => {
it('should work with state containing optional properties', () => {
expectSnippet(`
interface State { req: string; opt?: number };
const initialState: State = { req: 'a' };
const update = createAction('update');

const reducer = createReducer(
initialState,
on(update, (state) => ({ req: 'b' })),
);
`).toSucceed();
});

it('should work with state containing index signature', () => {
expectSnippet(`
const initialState: { [key: string]: number } = {};
const add = createAction('add', props<{ key: string; value: number }>());

const reducer = createReducer(
initialState,
on(add, (state, { key, value }) => ({ ...state, [key]: value })),
);
`).toSucceed();
});

it('should catch excess properties with index signature state', () => {
expectSnippet(`
interface State { name: string };
const initialState: State = { name: 'test' };
const update = createAction('update');

const reducer = createReducer(
initialState,
on(update, (state) => ({ ...state, extra: true })),
);
`).toFail(/Remove excess properties/);
});
});

describe('already enforced type checks', () => {
it('should catch missing required properties inside createReducer', () => {
expectSnippet(`
interface State { name: string; count: number };
const initialState: State = { name: 'test', count: 0 };
const setName = createAction('setName', props<{ name: string }>());

const reducer = createReducer(
initialState,
on(setName, (state, { name }) => ({ name })),
);
`).toFail(/is missing in type/);
});

it('should catch wrong property types inside createReducer', () => {
expectSnippet(`
interface State { name: string; count: number };
const initialState: State = { name: 'test', count: 0 };
const setName = createAction('setName', props<{ name: string }>());

const reducer = createReducer(
initialState,
on(setName, (state, { name }) => ({ ...state, name: 123 })),
);
`).toFail(/not assignable to type/);
});
});
});
}, 8_000);
5 changes: 5 additions & 0 deletions modules/store/src/models.ts
Original file line number Diff line number Diff line change
Expand Up @@ -175,4 +175,9 @@ export interface SelectSignalOptions<T> {
equal?: ValueEqualityFn<T>;
}

export const excessPropertiesAreNotAllowedMsg =
'callback return type must exactly match the state type. Remove excess properties.';
export type ExcessPropertiesAreNotAllowed =
typeof excessPropertiesAreNotAllowedMsg;

export type Prettify<T> = { [K in keyof T]: T[K] } & {};
Loading
Loading