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

StoreModule.forRoot and forFeature initialState typing not OK when providing metaReducers #3007

Closed
spierala opened this issue Apr 30, 2021 · 4 comments · Fixed by #3102
Closed

Comments

@spierala
Copy link

spierala commented Apr 30, 2021

Minimal reproduction of the bug/regression with instructions:

Use the following StackBlitz example to create a reproduction: https://stackblitz.com/edit/angular-ngrx-store-module-for-root?file=src/app/app.module.ts

The example code does not show (the expected) type errors when meta reducers are provided.
When the metaReducers are removed, then the type errors become visible.

Expected behavior:

ForRoot initialState type should match the provided reducers map keys.
ForFeature initial state type should match the provided reducers state type.
Type errors should show up accordingly, also if metaReducers are provided.

Versions of NgRx, Angular, Node, affected browser(s) and operating system(s):

NgRx 11.1.1, Ng11

@timdeschryver
Copy link
Member

@spierala have you tried to add the State type to the metareducer?

interface State {
  counter1: CounterState;
  counter2: CounterState;
}

function someMetaReducer(reducer): ActionReducer<State>  {
  return (state, action) => {
    return reducer(state, action);
  };
}

In order to infer the types correctly, we have to add a type to something.
Because the same generic is used for the root state, initial state, and metareducers we need to define a "truth" otherwise TypeScript won't know which type to use.

We could also remove the state generic from the metareducers, as the metareducer receives the global state... 🤔

@spierala
Copy link
Author

spierala commented May 4, 2021

@timdeschryver Adding a return type to the metaReducers helps indeed.
See updated example here: https://stackblitz.com/edit/angular-ngrx-store-module-for-root-typed-meta?file=src%2Fapp%2Fapp.module.ts

Also StoreModule.forRoot<State> would help.

Personally I think that the matching between initialState and the reducer structure is more important than the metaReducer type. On the other hand I think that the root reducer will drop "wrong" initialState properties anyway when it processes an action.

@timdeschryver
Copy link
Member

Personally I think that the matching between initialState and the reducer structure is more important than the metaReducer type

Yea, I feel the same way - that's why I was thinking not to use the State interface here.

On the other hand I think that the root reducer will drop "wrong" initialState properties anyway when it processes an action.

This isn't true, TypeScript doesn't drop properties (or doesn't throw like in some other languages), the "wrong" properties will still be there in the state.

@spierala
Copy link
Author

spierala commented May 5, 2021

This isn't true, TypeScript doesn't drop properties (or doesn't throw like in some other languages), the "wrong" properties will still be there in the state.

I mean the combined root reducer creates a new state object based on the reducer keys and therefore would drop inititalState props which do not have a corresponding reducer key. See combineReducers:
https://github.com/ngrx/platform/blob/master/modules/store/src/utils.ts#L63-L77

But "wrong" initialState props will remain when provided with StoreModule.forFeature

brandonroberts pushed a commit that referenced this issue Nov 2, 2021
Closes #3007 

BREAKING CHANGES:

`initialState` needs to match the interface of the store/feature.

BEFORE:

Missing properties were valid

```ts
StoreModule.forRoot(reducers, {
  initialState: { notExisting: 3 },
  metaReducers: [metaReducer]
});
```

AFTER:

A type error is produced for initialState that does not match the store/feature

```ts
StoreModule.forRoot(reducers, {
  initialState: { notExisting: 3 },
  metaReducers: [metaReducer]
});
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants