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 · 10 comments
Open

Wrong return type of combineReducers #74

enheit opened this issue May 4, 2019 · 10 comments
Labels

Comments

@enheit
Copy link

enheit 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
Copy link

lcoder commented Jul 30, 2019

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

demo

@dyllandry
Copy link

dyllandry 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
Copy link

dyllandry 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
Copy link

dyllandry 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
Copy link

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
Copy link

lcoder 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
Copy link
Author

enheit 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
Copy link

lcoder 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

@GengPeng951
Copy link

GengPeng951 commented Sep 9, 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>>

It dose not work
image

@jochumdev
Copy link

jochumdev commented Jan 16, 2022

combineReducers.ts:

import { AnyAction, ReducersMapObject } from "redux";
import Immutable from "immutable";
import { getUnexpectedInvocationParameterMessage } from "./utilities";

export default <S extends Immutable.Map<string, any>>(
  reducers: ReducersMapObject<any, AnyAction>,
  getDefaultState: () => S
): ((inputState: S | undefined, action: AnyAction) => S) => {
  const reducerKeys = Object.keys(reducers);

  // eslint-disable-next-line space-infix-ops
  return (inputState: S | undefined, action: AnyAction): S => {
    if (typeof inputState === "undefined") {
      inputState = getDefaultState();
    }

    // eslint-disable-next-line no-process-env
    if (process.env.NODE_ENV !== "production") {
      const warningMessage = getUnexpectedInvocationParameterMessage<S>(
        inputState,
        reducers,
        action
      );

      if (warningMessage) {
        // eslint-disable-next-line no-console
        console.error(warningMessage);
      }
    }

    return inputState.withMutations(temporaryState => {
      reducerKeys.forEach(reducerName => {
        const reducer = reducers[reducerName];
        const currentDomainState = temporaryState.get(reducerName);
        const nextDomainState = reducer(currentDomainState, action);

        if (nextDomainState === undefined) {
          throw new Error(
            'Reducer "' +
              reducerName +
              '" returned undefined when handling "' +
              action.type +
              '" action. To ignore an action, you must explicitly return the previous state.'
          );
        }

        temporaryState.set(reducerName, nextDomainState);
      });
    });
  };
};

Full code: https://github.com/pcdummy/redux-immutable-ts

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants