TypeScript defintion of StoreEnhancer does not allow state extension with replaceReducer

[mhelmer]

Do you want to request a feature or report a bug?

The StoreEnhancer TypeScript interface does not seem to have the appropriate type for the replaceReducer property of the enhanced store. I would consider it a bug in the type definition.

What is the current behavior?

When implementing the StoreEnhancer interface with ExtraState, the type signature of replaceReducer on the "enhanced" store is coupled to the type of the wrapped reducer.

Given a StoreEnhancer of type

StoreEnhancer<Ext, ExtraState>

the returned store is of type

Store<S & StateExt, A> & Ext

with the replaceReducer property as such

(nextReducer: Reducer<S & ExtraState, A>) => void

Returning a store with a replaceReducer that accepts the original reducer gives the following type-error:

Type '<S, A extends Action<any> = AnyAction>(reducer: Reducer<S, A>, preloadedState?: DeepPartial<S> | undefined) => { replaceReducer: (nextReducer: Reducer<S, A>) => void; dispatch: Dispatch<A>; getState(): S & ExtraState; subscribe(listener: () => void): Unsubscribe; [Symbol.observable](): Observable<...>; }' is not assignable to type 'StoreEnhancerStoreCreator<{}, ExtraState>'.
      Type '{ replaceReducer: (nextReducer: Reducer<S, A>) => void; dispatch: Dispatch<A>; getState(): S & ExtraState; subscribe(listener: () => void): Unsubscribe; [Symbol.observable](): Observable<S & ExtraState>; }' is not assignable to type 'Store<S & ExtraState, A>'.
        Types of property 'replaceReducer' are incompatible.
          Type '(nextReducer: Reducer<S, A>) => void' is not assignable to type '(nextReducer: Reducer<S & ExtraState, A>) => void'.
            Types of parameters 'nextReducer' and 'nextReducer' are incompatible.
              Types of parameters 'state' and 'state' are incompatible.
                Type 'S | undefined' is not assignable to type '(S & ExtraState) | undefined'.
                  Type 'S' is not assignable to type '(S & ExtraState) | undefined'.
                    Type 'S' is not assignable to type 'S & ExtraState'.
                      Type 'S' is not assignable to type 'ExtraState'.

If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem via https://jsfiddle.net or similar.

The type check fails for the following code:

import {
  StoreEnhancer,
  Action,
  AnyAction,
  Reducer,
  createStore,
  DeepPartial
} from "redux";

interface State {
  someField: "string";
}

interface ExtraState {
  extraField: "extra";
}

const reducer: Reducer<State> = null as any;

function stateExtensionExpectedToWork() {
  interface ExtraState {
    extraField: "extra";
  }

  const enhancer: StoreEnhancer<{}, ExtraState> = createStore => <
    S,
    A extends Action = AnyAction
  >(
    reducer: Reducer<S, A>,
    preloadedState?: DeepPartial<S>
  ) => {
    const wrappedReducer: Reducer<S & ExtraState, A> = null as any;
    const wrappedPreloadedState: S & ExtraState = null as any;
    const store = createStore(wrappedReducer, wrappedPreloadedState);
    return {
      ...store,
      replaceReducer: (nextReducer: Reducer<S, A>): void => {
        const nextWrappedReducer: Reducer<S & ExtraState, A> = null as any;
        store.replaceReducer(nextWrappedReducer);
      }
    };
  };

  const store = createStore(reducer, enhancer);
  store.replaceReducer(reducer);
}

See src/index.ts in the linked codesandbox example that implements the same function that would be expected to type-check, followed by another function of how it actually has to be done.

https://codesandbox.io/s/redux-store-enhancer-types-s6d3v

The example is based on the typescript test for the enhancer at test/typescript/enhancers.ts in this repo. The code doesn't execute (due to unsafe casts of null as any), but it is the type check that is of interest here.

What is the expected behavior?

When a store is created with a store enhancer that wraps a reducer and adds state, I would expect that replaceReducer on the returned store can be called with the original rootReducer.

const store = createStore(rootReducer, preloadedState, enhancer)
// ...
store.replaceReducer(rootReducer)

It would be the responsibility of the enhancer to appropriately replace the wrapped reducer. I.e return a store such as:

{
  ...store,
  replaceReducer: (nextReducer: Reducer<S, A>) => {
    store.replaceReducer(wrapReducer(nextReducer))
  },
}

Which versions of Redux, and which browser and OS are affected by this issue? Did this work in previous versions of Redux?

Redux version: 4.0.4,
OS: Ubuntu 19.04
Browser: N/A
Did this work in previous versions of Redux?: Not that I know of

[cellog]

This actually prevents moving the replaceReducer test to typescript (#3500)

[mhelmer]

I'm not sure about that fix since it essentially removes the type safety from replaceReducer, if I'm not mistaken.

I'm thinking it could be solved by adding an extra type generic to the Store type

export interface Store<S = any, A extends Action = AnyAction, ReplacedState = S> {
  /*... */
  replaceReducer(nextReducer: Reducer<ReplacedState, A>): void
  /*... */
} 

The StoreEnhancer type would be updated to

export type StoreEnhancer<Ext = {}, StateExt = {}> = (
  next: StoreEnhancerStoreCreator
) => StoreEnhancerStoreCreator<Ext, StateExt>
export type StoreEnhancerStoreCreator<Ext = {}, StateExt = {}> = <
  S = any,
  A extends Action = AnyAction
>(
  reducer: Reducer<S, A>,
  preloadedState?: PreloadedState<S>
) => Store<S & StateExt, A, S> & Ext

This maintains type safety and makes it possible to call replaceReducer with the original reducer from the outside. It would be breaking change though, since the returned store has a different signature and requires that you implement replaceReducer if you add extra state in an enhancer.

I could open a PR if you think it's a reasonable solution.

[cellog]

@mhelmer are you referring to the original issue or to #3507? #3507 included a different solution, which modifies replaceReducer to return the store, but with a different type signature, so type safety is not only preserved, it is enforced. But if you were referring to #3507, I am not sure I understood how it removes type safety, would like to hear more detail if you're willing!

[mhelmer]

Yes, #3507. It adds a type generic to replaceReducer. With that I believe you can call it with any reducer and the generics are inferred. I'm not sure, perhaps that's desireable.

[cellog]

Ok, I'll see if I can break it, thanks for the notice

[cellog]

@markerikson and @timdorr is there any history behind the typings for applyMiddleware that would be good to know about?

The reason I ask is because it's exceedingly difficult to apply the existing types, and they allow at least 1 invalid case (calling applyMiddleware with no middlewares).

In my experimentation, we can actually extract the dispatch and state extensions as well as action extensions from the middlewares and combine them pretty easily, so that no types need be passed, they will all be inferred.

However, this will break BC, so would need to be released in a breaking version.

So, before proceeding further, I'd love to see what people think is a good path. Here's what I propose:

1. We use a bunch of any in applyMiddleware.ts to make the existing function overload types work
2. In a separate PR, I put up the new types for review and discuss how to handle this.

It will require us to reach out to external middlewares maintainers to update their types as well. As part of the PR for (2), I will try updating the types for thunk and saga and see what we need

[markerikson]

I have always stayed away from any typings discussions, so I have no idea what's changed when.

FWIW, while passing 0 middlewares to applyMiddleware makes no sense semantically, it is legal syntactically. applyMiddleware passes them to compose, and compose handles the 0-arg case by returning arg => arg. So, while it's not meaningful, you can do it as far as I can see.

I'd prefer to not have any real breaking types changes at the moment if at all possible. On the other hand, if we're going to make changes, now would be the time to do it.

[cellog]

@mhelmer #3521 should assuage your concerns - the returned reducer strongly types its state. The inference is intentional - it causes the types of the reducer to attach themselves to the new store, as it should.

[timdorr]

Looks like this issue is resolved in the TS rewrite, so I'll close this out. Stay tuned for release...

[mhelmer]

@timdorr:

Actually, the original issue is not resolved at all.

The problem as described is for store enhancers that add extra state. In such cases it would make more sense if replaceReducer is called with the original reducer as input.

Post #3507 we can call replaceReducer with any reducer and it's supposed to return a store with the same type of reducer. This is not the case when an enhancer is adding extra state. Should also be noted that it's a breaking change. I don't think this can be solved without a breaking change to the types though.

Also, my concerns for the typings were about the reducer input, which can now be any reducer.