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

Wrong return type of combineReducers #74

Open
enheit opened this issue May 4, 2019 · 8 comments

Comments

@enheit
Copy link

commented May 4, 2019

Hello,

I am using redux-immutable with TypeScript and found this issue.

The combineReducer (provided by redux-immutable) returns a plain object instead of Immutable.Collection.

Such typing force me to do state.sidebar instead of state.get('sidebar') in mapStateToProps.

const mapStateToProps = (state: StateType<typeof rootReducer>) => ({
  sidebar: state.get('sidebar'), // <- this is correct example of usage
})

Screen Shot 2019-05-04 at 4 51 19 AM

Screen Shot 2019-05-04 at 2 07 41 PM

@lcoder

This comment has been minimized.

Copy link

commented Jul 30, 2019

Would you help me fix typescript types error with combineReducers ? Thank you!

demo

@dyllandry

This comment has been minimized.

Copy link

commented Aug 6, 2019

If redux-immutable's combineReducers() returns an incorrect type, then we cannot infer our application's state type from combineReducers().

This may be a significant problem.

Doing that, inferring app state from combineReducers(), is exactly the recommended pattern on the Redux website.

// src/store/index.ts

import { systemReducer } from './system/reducers'
import { chatReducer } from './chat/reducers'

const rootReducer = combineReducers({
  system: systemReducer,
  chat: chatReducer
})

// Here the app's entire state type is inferred from the
// return type of the reducer combineReducers returns
export type AppState = ReturnType<typeof rootReducer>

Having type safety for your entire application from the root level in one type variable can be very useful. Though, when redux-immutable's combineReducers() method reports an alternative type for our AppState, we cannot do that with type safety.

However, not all is lost. Our reducers enforce type safety on smaller slices of our application state. This issue only effects type safety at a root level. For example, using store.getState() to return the whole application state with type safety is not possible.

I hope this comment helps.

@dyllandry

This comment has been minimized.

Copy link

commented Aug 7, 2019

I've done a little digging.
node_modules/@types/redux-immutable/index.d.ts

// Type definitions for redux-immutable v4.0.0
// Project: https://github.com/gajus/redux-immutable
// Definitions by: Pedro Pereira <https://github.com/oizie>
//                 Sebastian Sebald <https://github.com/sebald>
//                 Gavin Gregory <https://github.com/gavingregory>
//                 Kanitkorn Sujautra <https://github.com/lukyth>
// Definitions: https://github.com/DefinitelyTyped/DefinitelyTyped
// TypeScript Version: 2.3

import { ReducersMapObject, Reducer, Action } from 'redux';
import { Collection } from 'immutable';

export declare function combineReducers<S, A extends Action, T>(reducers: ReducersMapObject<S, A>, getDefaultState?: () => Collection.Keyed<T, S>): Reducer<S, A>;
export declare function combineReducers<S, A extends Action>(reducers: ReducersMapObject<S, A>, getDefaultState?: () => Collection.Indexed<S>): Reducer<S, A>;
export declare function combineReducers<S>(reducers: ReducersMapObject<S, any>, getDefaultState?: () => Collection.Indexed<S>): Reducer<S>;

I think the above type declaration file for redux-immutable provides some information. Though, my abilities in diagnosing this are inadequate for the issue to stand out to me. I would be grateful for anyone who can help out.

@gajus gajus added the question label Aug 7, 2019

@dyllandry

This comment has been minimized.

Copy link

commented Aug 7, 2019

Like I said, the return type of the reducer combineReducers() returns is what we infer our app root state from. The return type of the reducer combineReducers() returns is of generic type <S>.

I think the reason why I cannot set generic parameter <S> to be my desired return type of Immutable.Map is because the first parameter of combineReducers() must be a Redux.ReducersMapObject type which uses generic type <S> to describe it's own type. However, that description of Redux.ReducersMapObject is as follows.

export type ReducersMapObject<S = any, A extends Action = Action> = {
  [K in keyof S]: Reducer<S[K], A>
}

[K in keyof S]: Reducer<S[K], A> cannot be an Immutable.Map. That type is specific to plain objects that use keys and values.

Might be totally wrong here. I am just going to try and write my own combineReducers that works with Immutable.

@dyllandry

This comment has been minimized.

Copy link

commented Aug 7, 2019

I wrote a root reducer function by hand to get it to return the root application state as an Immutable.Map. Any time I add a new reducer to my root state I'll need to type it here as well.

export const rootReducer = (state: Map<string, any> = Map(), action: any) => {
  return Map({
    todos: todosReducer(state.get('todos'), action)
  })
}

const store = createStore(rootReducer)
@lcoder

This comment has been minimized.

Copy link

commented Aug 8, 2019

I find a way . Just wrap the ReturnType<typeof rootReducer> with Immutable.Record

// src/store/index.ts
import { Record } from 'immutable'
import { combineReducers } from 'redux-immutable'
import { systemReducer } from './system/reducers'
import { chatReducer } from './chat/reducers'

const rootReducer = combineReducers({
  system: systemReducer,
  chat: chatReducer
})

export type AppState = Record<ReturnType<typeof rootReducer>>
@enheit

This comment has been minimized.

Copy link
Author

commented Aug 19, 2019

I find a way . Just wrap the ReturnType<typeof rootReducer> with Immutable.Record

// src/store/index.ts
import { Record } from 'immutable'
import { combineReducers } from 'redux-immutable'
import { systemReducer } from './system/reducers'
import { chatReducer } from './chat/reducers'

const rootReducer = combineReducers({
  system: systemReducer,
  chat: chatReducer
})

export type AppState = Record<ReturnType<typeof rootReducer>>

I think it works only one level deep.

If you try to get some properties one level deeper it will gives you wrong types (at least Visual Studio Code doesn't provide autosuggestion).

Correct me if I wrong.

@lcoder

This comment has been minimized.

Copy link

commented Aug 20, 2019

In my project,I think it works well.

autosuggestion:
image

correct types with deep levels:
image

But,It has a limit。autosuggestion can't works fine with getIn method。

autosuggestion doesn't work:
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.