gajus / redux-immutable

redux-immutable is used to create an equivalent function of Redux combineReducers that works with Immutable.js state.
Other
1.88k stars 82 forks source link

Wrong return type of combineReducers #74

Open enheit opened 5 years ago

enheit commented 5 years ago

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 commented 5 years ago

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

demo

dyllandry commented 5 years ago

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 commented 5 years ago

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.

dyllandry commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 2 years ago

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