Repository navigation
feat(store): enforce exact return types in on() and updater() callbacks - #5078
david-shortman wants to merge 12 commits into
Conversation
✅ Deploy Preview for ngrx-io ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
The updater() method now enforces exact return types at the type level,
preventing excess properties in callback return values without requiring
explicit return type annotations.
This eliminates the need for the updater-explicit-return-type ESLint rule,
which is now deprecated.
BREAKING CHANGES:
Updater callbacks that return objects with properties not present in the
state type will now produce TypeScript compilation errors.
When ComponentStore is extended with a generic state type parameter
(e.g. class MyStore<T> extends ComponentStore<T>), callbacks that
spread state and override known properties may produce a false type
error. Return state directly or use a type assertion (as T) in those
cases.
BEFORE:
```ts
interface State { name: string; count: number }
const store = new ComponentStore<State>({ name: '', count: 0 });
// Previously compiled without error despite the excess property
store.updater((state, name: string) => ({
...state,
name,
extraProp: true, // no error
}));
```
AFTER:
```ts
// Now produces a TypeScript error:
// Type 'boolean' is not assignable to type
// '"updater callback return type must exactly match the state type.
// Remove excess properties."'
store.updater((state, name: string) => ({
...state,
name,
extraProp: true, // TS error
}));
```
Closes ngrx#4280
The on() function now enforces exact return types at the type level,
preventing excess properties in callback return values without requiring
explicit return type annotations.
This eliminates the need for the on-function-explicit-return-type ESLint
rule, which is now deprecated.
BREAKING CHANGES:
Reducer callbacks passed to on() that return objects with properties not
present in the state type will now produce TypeScript compilation errors.
When on() is used inside a generic reducer factory where the state type
is an unresolved generic parameter (e.g. createGenericReducer<TState>),
callbacks that spread state and override known properties may produce a
false type error. Return state directly or use a type assertion
(as TState) in those cases.
BEFORE:
```ts
interface State { name: string; count: number }
const initialState: State = { name: '', count: 0 };
const reducer = createReducer(
initialState,
on(setName, (state, { name }) => ({
...state,
name,
extraProp: true, // no error
})),
);
```
AFTER:
```ts
// Now produces a TypeScript error:
// Type '{ extraProp: boolean; ... }' is not assignable to type
// '"callback return type must exactly match the state type.
// Remove excess properties."'
const reducer = createReducer(
initialState,
on(setName, (state, { name }) => ({
...state,
name,
extraProp: true, // TS error
})),
);
```
Closes ngrx#4280
f5a3b4b to
23aa053
Compare
|
CI failures seem unrelated to this PR: effects:test example-app-e2e |
timdeschryver
left a comment
There was a problem hiding this comment.
Because this is a breaking change, the earliest that this can be released is v22.
It will also require a discussion first.
| name: path.parse(__filename).name, | ||
| meta: { | ||
| type: 'problem', | ||
| deprecated: true, |
There was a problem hiding this comment.
Is there a reason/advantage to deprecate the rules instead of removing them?
There was a problem hiding this comment.
I suppose they actually don't have value when the return types are no longer needed. So might as well just remove.
|
|
||
| </ngrx-code-example> | ||
|
|
||
| The `updater` method enforces that callbacks return an object matching the state type exactly. Returning an object with extra properties that don't exist on the state type will produce a TypeScript compilation error: |
There was a problem hiding this comment.
| The `updater` method enforces that callbacks return an object matching the state type exactly. Returning an object with extra properties that don't exist on the state type will produce a TypeScript compilation error: | |
| The `updater` method enforces that callbacks return an object matching the state type exactly. Returning an object with extra properties that don't exist on the state type produces a TypeScript compilation error: |
|
|
||
| ### Exact return type enforcement | ||
|
|
||
| The `on` function enforces that callbacks return an object matching the state type exactly. Returning an object with extra properties that don't exist on the state type will produce a TypeScript compilation error: |
There was a problem hiding this comment.
| The `on` function enforces that callbacks return an object matching the state type exactly. Returning an object with extra properties that don't exist on the state type will produce a TypeScript compilation error: | |
| The `on` function enforces that callbacks return an object matching the state type exactly. Returning an object with extra properties that don't exist on the state type produces a TypeScript compilation error: |
|
|
||
| <ngrx-docs-alert type="inform"> | ||
|
|
||
| **Note:** When `ComponentStore` is extended with a generic state type parameter (e.g., `class MyStore<T extends object> extends ComponentStore<T>`), TypeScript cannot fully resolve the excess property check because `keyof T` is deferred. In those cases, callbacks that spread state and override known properties may produce a false type error. Return `state` directly or use a type assertion (`as T`) as a workaround. |
There was a problem hiding this comment.
I think mentioning "because keyof T is deferred" is not very useful here for our users. Additionally, adding a test case for this scenario can be useful.
| **Note:** When `ComponentStore` is extended with a generic state type parameter (e.g., `class MyStore<T extends object> extends ComponentStore<T>`), TypeScript cannot fully resolve the excess property check because `keyof T` is deferred. In those cases, callbacks that spread state and override known properties may produce a false type error. Return `state` directly or use a type assertion (`as T`) as a workaround. | |
| **Note:** When `ComponentStore` is extended with a generic state type parameter (e.g., `class MyStore<T extends object> extends ComponentStore<T>`), TypeScript cannot fully resolve the excess property check. In those cases, callbacks that spread state and override known properties may produce a false type error. Return `state` directly or use a type assertion (`as T`) as a workaround. |
✅ Deploy Preview for ngrx-site-v21 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Remove updater-explicit-return-type and on-function-explicit-return-type ESLint rules since TypeScript now enforces exact return types at the type level, making these lint rules unnecessary. Address review feedback by using present tense in docs and simplifying generic type notes. Add v22 migration guide with breaking change callout for rule removal. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ba777e1 to
696698f
Compare
The updater exact-return-type tests added in this branch used ts-snippet's expectSnippet API, which was removed when ngrx#5145 migrated the component-store type specs to Vitest. Rewrite them with expectTypeOf / @ts-expect-error to match the migrated specs and fix the test-affected CI failure. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Under TypeScript 6.0 the branded exact-return-type error is reported on the updater callback argument (and the trailing invocation surfaces a second "Expected 0 arguments" error), so the inline @ts-expect-error placements did not match. Move the directive before the callback argument and drop the trailing invocation so exactly one expected error is suppressed per case. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@david-shortman can you please take a look at the conflicts/build please? |
Resolve modify/delete conflicts on the legacy eslintrc config files (all.json, all-type-checked.json, component-store.json, store.json) by accepting their removal from upstream (ngrx#5175 dropped eslint v8 / legacy eslintrc config). The deprecated rule removals are already reflected in the corresponding .ts config files. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ric state Add a type test covering the known limitation where an unresolved generic state type (class MyStore<T> extends ComponentStore<T>) defers the excess-property check, causing spread-with-override callbacks to report a false positive. Verifies both documented workarounds (an `as T` assertion and returning state/T directly) compile. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
|
||
| <ngrx-docs-alert type="inform"> | ||
|
|
||
| **Note:** When `ComponentStore` is extended with a generic state type parameter (e.g., `class MyStore<T extends object> extends ComponentStore<T>`), TypeScript cannot fully resolve the excess property check. In those cases, callbacks that spread state and override known properties may produce a false type error. Return `state` directly or use a type assertion (`as T`) as a workaround. |
There was a problem hiding this comment.
If this fix solves the updater return type but introduces the issue with user-defined stores that use generics for state (as it is mentioned here), I don't think we can proceed with it.
|
|
||
| <ngrx-docs-alert type="inform"> | ||
|
|
||
| **Note:** When `on` is used inside a generic reducer factory where the state type is an unresolved generic parameter (e.g., `function createGenericReducer<TState>()`), TypeScript cannot fully resolve the excess property check. In those cases, callbacks that spread state and override known properties may produce a false type error. Return `state` directly or use a type assertion (`as TState`) as a workaround. |
There was a problem hiding this comment.
|
We've decided to not include this in the v22 release. |
|
Sounds good, seems like I need more thought on this. |
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…e in exact return type check
✅ Deploy Preview for ngrx-io ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
I've added a fix for false positives and don't need a generic workaround anymore. The primary fix was to infer returned keys into The generic-limitation docs notes are removed, and the @ts-expect-error limitation test is now a passing test, with added cases for generic, union, nullable and Record state. |
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
TypeScript does not perform excess property checking on callback return values. This means that
on()andComponentStore.updater()callbacks can return objects with extra properties that don't exist on the state type, and TypeScript won't report an error:The
on-function-explicit-return-typeandupdater-explicit-return-typeESLint rules exist as workarounds, requiring developers to add explicit return type annotations to trigger TypeScript's excess property checking.Closes #4280
What is the new behavior?
on()andComponentStore.updater()report excess properties at compile time, and the error names the property:How it works: the callback's return type is captured as
R, and its keys are inferred intoK extends AllowedStateKeys<State>. A conditional type can't see a generic's constraint (e.g.T extends { loading: boolean }), but the constraint check on an inferred type argument can, because it uses assignability. Keys that fail the constraint are left over and reported as excess.The custom error messages follow the same pattern as NgRx action creators (e.g.,
'action creator cannot return an array'), giving actionable guidance instead of a crypticnevererror.No explicit return type annotations are needed. The following continue to work without changes:
(state) => ({ ...state, name: 'updated' })initialStateorstatedirectlyon()Record<string, T>with computed keys)User | null){ ...state, loading: true }withT extends { loading: boolean }), other keys are reported as excessThe
on-function-explicit-return-typeandupdater-explicit-return-typeESLint rules are removed, since the type check makes them unnecessary.Does this PR introduce a breaking change?
BREAKING CHANGES:
on()andComponentStore.updater()callbacks must return the exact state type. Returning properties that don't exist on the state type is now a compilation error that names the property.BEFORE:
AFTER:
Remove the excess property, or add it to the state type. Union, nullable, and index-signature state types are supported, as are generic state types: properties guaranteed by the generic constraint can be returned, other properties are reported as excess.
The
@ngrx/on-function-explicit-return-typeand@ngrx/updater-explicit-return-typeESLint rules are removed because the type check makes them unnecessary.BEFORE:
{ "rules": { "@ngrx/on-function-explicit-return-type": "error", "@ngrx/updater-explicit-return-type": "error" } }AFTER:
{ "rules": {} }Other information
Type tests (
ts-snippetforon(), VitestexpectTypeOf/@ts-expect-errorforupdater()) cover:state/initialState, optional properties, index signatures andRecordstate, discriminated unions, nullable state, and multiple action creators. Generic cases: constrained overrides, unconstrainedreturn state,ListState<T>,Base & T, andas Tassertions.createReducer-wrapped and standaloneon(), excess properties in one branch of a ternary, keys on no union member, excess keys on nullable state, keys not guaranteed by a generic constraint, missing properties, and wrong property types.