Revert adding the store as a return type to replaceReducer

[Methuselah96]

---

name: "Revert adding return to replaceReducer"
about: Revert adding return to replaceReducer

PR Type

Does this PR add a new feature, or fix a bug?

Fix a bug.

Why should this PR be included?

This PR resolves #3767 and resolves #3482.

tl;dr You can't change the shape of the store and have static typing at the same time.

This PR reverts most of #3507 and #3524. If someone wants to change the state or actions that a store should operate on, then they'll have to cast the store. This cast should hopefully make it obvious to them that they're doing an operation that would make the types of the original store incorrect.

Note that this resolves the original issue in #3482. You can now replace the reducer with your original reducer and it compiles correctly.

Checklist

• Have you added an explanation of what your changes do and why you'd like us to include them?
• Is there an existing issue for this PR?
  • replaceReducer type doesn't account for the mutation to the original store #3767 and TypeScript defintion of StoreEnhancer does not allow state extension with replaceReducer #3482
• Have the files been linted and formatted?
• Have the docs been updated to match the changes in the PR?
• Have the tests been updated to match the changes in the PR?
• Have you run the tests locally to confirm they pass?

Bug Fixes

What is the current behavior, and the steps to reproduce the issue?

When you replace a reducer, it returns a new store with new types for state and action. You can still access the old store which now has the wrong types for store and action.

What is the expected behavior?

The user should be forced to cast the store if they're replacing the reducer with a new reducer that has a different type for state or actions.

How does this PR fix the problem?

It removes the return from replaceReducer.

[netlify]

Deploy preview for redux-docs ready!

Built with commit b8fc8a9

https://deploy-preview-3772--redux-docs.netlify.app

[Methuselah96]

The change in types actually caught a bug in one the tests where we weren't re-wrapping the reducer previously.

[timdorr]

I dunno if @cellog is around, but if he is, did he want to take a look at this?

[cellog]

I'll try to take a look.

[Methuselah96]

@cellog I took out some unnecessary changes that I'll put in a separate PR in order to keep this PR as focused as possible. My apologies if you've already started reviewing.

[cellog]

I don't have time right now to do the fix, but #3767 assumes it is not possible to change the type of the store when it actually is possible. All we have to do is change replaceReducer to be a typeguard changing the definition of this as in methodname(): this is NewType and it will fix the typing errors you were seeing. Subtle but totally legit typescript trick. Apologies is that is unclear. When I have time, I'll whip up an example PR to compare to this. It will, however, break the API of the return type of replaceReducer (again), as typeguards must return a boolean value. However, in this case, the benefit of breaking a just-now-introduced API hack just for typescript to another one that will actually change the type of the store is worth it.

[Methuselah96]

@cellog That sounds really cool, however I'm having trouble getting an example working. Can you help? I'm not seeing much online about it. It seems like within the type guard, the type is now Container<T> & Container<NT> and getValue() still returns T. Even if this does work, the store is still the wrong type outside of the type guard, so I'm not sure how helpful it is and I'm not sure that it resolves the original issue. Here's what I have so far:

Code

interface Containter<T> {
    getValue(): T;
    changeType<NT>(newValue: NT): this is Containter<NT>;
}

function createConatiner<T>(value: T): Containter<T> {
    let savedValue = value;
    return {
        getValue() {
            return savedValue;
        },
        changeType<NT>(newValue: NT) {
            savedValue = newValue as unknown as T;
            return true;
        }
    }
}

const originalContainer = createConatiner(5);

if (originalContainer.changeType('testing')) {
    const value = originalContainer.getValue();
    // This should be a `string` now right?
    const valueShouldBeString: string = value;
}

const value = originalContainer.getValue();
// I guess this is still a number?
const valueStillANumber: number = value;

Output

"use strict";
function createConatiner(value) {
    let savedValue = value;
    return {
        getValue() {
            return savedValue;
        },
        changeType(newValue) {
            savedValue = newValue;
            return true;
        }
    };
}
const originalContainer = createConatiner(5);
if (originalContainer.changeType('testing')) {
    const value = originalContainer.getValue();
    // This should be a `string` now right?
    const valueShouldBeString = value;
}
const value = originalContainer.getValue();
// I guess this is still a number?
const valueStillANumber = value;

Compiler Options

{
  "compilerOptions": {
    "noImplicitAny": true,
    "strictNullChecks": true,
    "strictFunctionTypes": true,
    "strictPropertyInitialization": true,
    "strictBindCallApply": true,
    "noImplicitThis": true,
    "noImplicitReturns": true,
    "useDefineForClassFields": false,
    "alwaysStrict": true,
    "allowUnreachableCode": false,
    "allowUnusedLabels": false,
    "downlevelIteration": false,
    "noEmitHelpers": false,
    "noLib": false,
    "noStrictGenericChecks": false,
    "noUnusedLocals": false,
    "noUnusedParameters": false,
    "esModuleInterop": true,
    "preserveConstEnums": false,
    "removeComments": false,
    "skipLibCheck": false,
    "checkJs": false,
    "allowJs": false,
    "declaration": true,
    "experimentalDecorators": false,
    "emitDecoratorMetadata": false,
    "target": "ES2017",
    "module": "ESNext"
  }
}

Playground Link: Provided

[cellog]

I have been spending some time thinking through this, and experimenting. Here is what I have found.

1. this is <blah> doesn't solve our problem, and I can't figure out why. The documentation of this-based type guards is no longer in the official manual for typescript, but there is no announcement anywhere. Also, it still works for XState so
2. I don't think this PR takes the correct approach to solving the problem, but I do think it is a real problem. I hope you'll bear with the long explanation below.

Regarding #2, let's examine what the purpose of replaceReducer is in Redux. There are 2 use cases I have seen:

1. hot reloading
2. dynamically augmenting the store state

There is another standard method that is more common for augmenting store state in Redux: store enhancers. A store enhancer may extend the state shape or the store shape. The existing code makes strict typing of store enhancers through inference possible, this PR strips out that possibility.

Let's imagine the following scenario:

1. user is using a store enhancer like ion-router (https://cellog.github.io/ion-router/) that adds a field to the store shape and augments the store shape. In this case, it adds router to the store, and routing to the store state. Our store looks like:

{
  router: {...},
  state: {
    routing: {...},
    // user-defined state
  }
  // redux functions
}

When the user calls replaceReducer to change the reducers to get a new store shape (for instance, to add a lazy-loaded sub-tree of the application that adds new state, say a form widget), the new store shape might be:

{
  router: {...},
  state: {
    routing: {...},
    // previous user-defined state
    // plus the new form widget
    form: {...},
  }
  // redux functions
}

This is all perfectly valid javascript. Note that the store enhancer additions to the store shape and to the state shape persist through a replaceReducer call.

How do we represent this in typescript? If we don't pass in extra fields to the store interface, this is no longer possible to represent, as replaceReducer has no knowledge of the difference between the user-supplied store shape and the store enhancer-provided store shape, and so when it replaces the shape, this is also nuked. Users will have to know all of the store enhancer's shape augmentation and store augmentation to do a correct unsafe type cast.

It also presents a dilemma, which you have stumbled across: how do we tell typescript that the original store variable that createStore returned has changed its type?

The current code will preserve the typing and also correctly infer the propagation of the store enhancer chain through createStore.

It is true that we will have to use an unsafe cast to continue to use the original store declaration, this change will not propagate automagically throughout the app.

But let's look at how you might make this work.

Let's imagine that you are using the variable store and exporting it all over the place, and want to be able to safely type this variable.

You call store.replaceReducer with a new reducer.

Now, you will need to scatter (store as unknown as Store<NewState, NewA>) all over the app. This gets tedious, so you might find yourself doing:

store.replaceReducer(newReducer);
const newStore = (store as unknown as Store<NewState, NewA>);

and then using newStore instead. This is slightly longer than:

const newStore = store.replaceReducer(newReducer);

and requires using a very unsafe double type cast. In the end, this is unlikely to be a good solution, because how do we know for certain which version of the store shape is in use?

The answer I see is a different approach: using type guards at the point the store is used.

If you have code that relies upon strict typing to differentiate between two different possible store shapes, you should write 2 type guards:

function isStore1<S extends Store>(store: S): store is S & StoreShape1 {
  return store.getState().shape1 !== undefined
}

function isStore2<S extends Store>(store: S): store is S & StoreShape2 {
  return store.getState().shape2 !== undefined
}

then, you can use this in the code:

if (isStore1(store)) {
  // operate on store with shape 1
} else if (isStore2(store)) {
  // operate on store with shape 2
}

In other words, I don't think redux is the correct surface to fix the problem you are encountering. It is definitely a problem, but there is no generic way to solve your specific issue. However, the code that is in redux that this PR strips does solve the problem of store enhancers that augment the store or state shape in a generic, chainable fashion so that you can use createStore with any store enhancer and still use replaceReducer with strict type safety.

For these reasons, I don't think this PR is the right solution. I think the right solution is perhaps to add something similar to the explanation above to the documentation.

Thanks for being patient, I have zero time to do proper investigations during the week right now with working from home and full-time parenting. Also, thanks for raising this issue and putting so much effort in, I am definitely interested in having further dialog with you, @timdorr and @markerikson about these points.

I think it's fair to say all of this is pretty edge case-y for most folks who use redux (we don't use any of this fanciness in my workplace, for instance. The most avant garde stuff we use is thunks, and even that is about to be stripped out in favor of using redux only as the caching layer), so it may be that we 2 are the only people who care about this. Either way, let's see if we can come to a resolution that works for your needs.

[Methuselah96]

@cellog Thanks for spending the time to think about this some more, I really appreciate it. I'm working on TypeScript-ing redux-devtools right now, hence the onslaught of issues/PRs regarding the types for enhancers/stores. We might very well be the only two people who care about this.

The example you provided is very helpful and helps ground the discussion. I agree that having type-safety is the whole point of using TypeScript, and so if we lower ability for the types to catch mistakes, then its not serving its purpose (that's why we have dispatch() accept a user-specified generic action, even though it should be able to accept any action in reality).

I'm gonna dig into it a little bit more and see what I can come up with. Again, really appreciate the response.

[Methuselah96]

@cellog Apologies in advance for the length of this response.

It seems like there are two concerns: type-safety for the person writing the enhancer and type-safety for the person using the enhancer.

Type-safety for the person using the enhancer

As you noted, it seems like the best way to ensure type-safety would be to have type-guards. However, I'm not sure how having replaceReducer return the new Store type or not return the new Store type would affect this. Here are the two scenarios in my mind:

With return from replaceReducer:

import { createStore, Reducer, Store } from "redux";

// ----------------------
// storeManager.ts

interface State1 {
  state1: number;
}
interface State2 {
  state2: string;
}

declare const reducer1: Reducer<State1>;
declare const reducer2: Reducer<State2>;
export let store: Store<State1 | State2> = createStore(reducer1);

export function dynamicallySwitchToReducer2() {
  const newStore = store.replaceReducer(reducer2);
  // What do I do with this `newStore` variable?
  // I guess assign it back to `store`?
  // This seems unnecessary and confusing.
  store = newStore;
}

function isStore1(store: Store<State1 | State2>): store is Store<State1> {
  return (store.getState() as State1).state1 !== undefined;
}
function isStore2(store: Store<State1 | State2>): store is Store<State2> {
  return (store.getState() as State2).state2 !== undefined;
}

// -----------------------
// someUserCode.ts

if (isStore1(store)) {
  const state1Property = store.getState().state1;
} else if (isStore2(store)) {
  const state2Property = store.getState().state2;
}

Without return from replaceReducer:

import { createStore, Reducer, Store } from "redux";

// ----------------------
// storeManager.ts

interface State1 {
  state1: number;
}
interface State2 {
  state2: string;
}

declare const reducer1: Reducer<State1>;
declare const reducer2: Reducer<State2>;
export const store: Store<State1 | State2> = createStore(reducer1);

export function dynamicallySwitchToReducer2() {
  store.replaceReducer((reducer2 as unknown) as Reducer<State1 | State2>);
}

function isStore1(store: Store<State1 | State2>): store is Store<State1> {
  return (store.getState() as State1).state1 !== undefined;
}
function isStore2(store: Store<State1 | State2>): store is Store<State2> {
  return (store.getState() as State2).state2 !== undefined;
}

// -----------------------
// someUserCode.ts

if (isStore1(store)) {
  const state1Property = store.getState().state1;
} else if (isStore2(store)) {
  const state2Property = store.getState().state2;
}

To me the the lack of a return type makes it more clear that replaceReducer is going to mutate the original store. When I see that a method has a return type that, it leads me to believe that the method is immutable and is returning a different store than what I had previously.

I could be misrepresenting how you think the reducer would be replaced dynamically, so my apologies if that's the case. Sill, I believe the return type forces the user to realize that the type of their original store is going to change instead of assuming that they can still use their old store the same way they were previously. I'm having trouble seeing a scenario where just replacing the reducer locally could be safe.

Type-safety for the person writing the enhancer

I'm going to go through the different mistakes that a store enhancer author could make regarding replaceReducer.

Returning the wrong store from replaceReducer

This bug was present in all of the enhancers in test/typescript/enhancers.ts that did not augment the state. The potential for this bug is eliminated in this PR because we no longer return the new store. If replaceReducer is expected to return the enhanced store, then every enhancer now has to include a boilerplate replaceReducer function in order to return the correct store as seen below in the comments. Note that #3776 resolves types not catching this case, but it's still annoying from an enhancer's author's position to have to include this in enhancers that don't augment the state.

import { createStore, Reducer, StoreEnhancer } from "redux";

const enhancer: StoreEnhancer<{
  enhancedProperty: number;
}> = (createStore) => (reducer, preloadedState) => {
  const store = createStore(reducer, preloadedState);
  return {
    ...store,
    enhancedProperty: 5,
    // Forgot to override replaceReducer to return the new store with the enhanced property.
    // This store will return the un-augmented store from the original `store` variable.
  };

  // Should be:
  // const store = createStore(reducer, preloadedState);

  // function replaceReducer<NewState, NewActions extends Action>(
  //   nextReducer: Reducer<NewState, NewActions>
  // ) {
  //   store.replaceReducer(nextReducer)
  //   return newStore
  // }

  // const newStore = {
  //   ...store,
  //   replaceReducer,
  //   enhancedProperty: 5,
  // };
  // return newStore;
};

declare const reducer: Reducer<{ someState: string }>;
const store = createStore(reducer, enhancer);

const enhancedProperty = store.enhancedProperty;

declare const newReducer: Reducer<{ someDifferentState: string }>;
const newStore = store.replaceReducer(newReducer);

// TypeScript thinks I can access this property.
// However because my enhanced store did not override `replaceReducer`
// it will actually return the un-enhanced store without `enhancedProperty`.
const enhancedProperty2 = newStore.enhancedProperty;

Forgetting to re-wrap the new reducer

Even without a return type, the types should still catch the case where a the enhancer author forgets to re-wrap the reducer. Here's an example:

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

interface ExtraState {
  extraField: "extra";
}

const enhancer: StoreEnhancer<{}, ExtraState> = (createStore) => <
  S,
  A extends Action = AnyAction
>(
  reducer: Reducer<S, A>,
  preloadedState?: any
) => {
  const wrapReducer = (reducer: Reducer<S, A>): Reducer<S & ExtraState, A> => (
    state,
    action
  ) => {
    const newState = reducer(state, action);
    return {
      ...newState,
      extraField: "extra",
    };
  };
  const wrappedPreloadedState = preloadedState
    ? {
        ...preloadedState,
        extraField: "extra",
      }
    : undefined;

  const store = createStore(wrapReducer(reducer), wrappedPreloadedState);
  return {
    ...store,
    replaceReducer(nextReducer: Reducer<S, A>) {
      // I forgot to re-wrap the reducer and got this error:
      // TS2345: Argument of type 'Reducer<S, A>' is not assignable to parameter of type 'Reducer<S & ExtraState, A>'.
      //   Type 'S' is not assignable to type 'S & ExtraState'.
      //     Type 'S' is not assignable to type 'ExtraState'.
      store.replaceReducer(nextReducer);
    },
  };
};

Or if I didn't include a return type for wrapReducer, I would get:

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

interface ExtraState {
  extraField: 'extra';
}

const enhancer: StoreEnhancer<{}, ExtraState> = (createStore) => <
  S,
  A extends Action = AnyAction
>(
  reducer: Reducer<S, A>,
  preloadedState?: any
) => {
  const wrapReducer = (reducer: Reducer<S, A>) => (state: S, action: A) => {
    const newState = reducer(state, action);
    return {
      ...newState,
      extraField: "extra",
    };
  };
  const wrappedPreloadedState = preloadedState
    ? {
        ...preloadedState,
        extraField: "extra",
      }
    : undefined;

  // I forgot to re-wrap the reducer and got this error:
  // TS2345: Argument of type '(state: S, action: A) => S & { extraField: string; }' is not assignable to parameter of type 'Reducer<S & { extraField: string; }, A>'.
  //   Types of parameters 'state' and 'state' are incompatible.
  //     Type '(S & { extraField: string; }) | undefined' is not assignable to type 'S'.
  //       Type 'undefined' is not assignable to type 'S'.
  const store = createStore(wrapReducer(reducer), wrappedPreloadedState);
  return {
    ...store,
    replaceReducer(nextReducer: Reducer<S, A>) {
      store.replaceReducer(nextReducer);
    },
  };
};

I think those are the main bugs that an enhancer author can make.

In summary, I don't think we lose any type-safety with this change. In fact, I think we increase the likelihood of the enhancer author and user getting the types correct. Sorry for the long code samples. Let me know what you think.

On a side note, do you have a real-life example of a store enhancer that augments the state within the enhancer? I've been looking for a real-life example, but have been unable to find one so far. The types would be a lot simpler if StateExt was not necessary. I noticed that ion-router has the user pass the reducer into combineReducers themselves, which seems like the preferable option. See #3773 for more about that.

[Methuselah96]

P.S. Regardless of how we type it, I agree some documentation would be helpful and I would be willing to write up a section on how to use it on the Usage With TypeScript page.

I think there are more situations with the current typing where it's easier to go wrong and for the code to still compile. I think getting rid pf the return type would make it more clear what is going on, would make the user think twice before doing anything rash with replaceReducer, and would hopefully make them look at the documentation to make sure they're using it properly.

For code splitting (which is the other usage for replaceReducer besides hot reloading mentioned in the docs), I found a decent article on how to TypeScript code splitting in Redux, and it doesn't seem to require the use of the return type of replaceReducer and I don't think you would even end up needing to cast the reducers coming in, but I haven't looked at it too closely.

[cellog]

I am playing a concert tonight, so can't dive in depth into your explanation, but my initial reaction is that it sounds solid to me. More tomorrow.

[Methuselah96]

Good luck on your concert, hope it goes well! Sounds like you got enough to keep you busy with working from home and being a full-time parent. Thanks for spending the time to help out here as well.

[timdorr]

Just to chime in here: I'm cool with changing the signature of the API, given that this will release in a major. However, now that I think about it in this context, I don't think a return from replaceReducer makes sense.

Redux treats the store as an instance object, not a factory. That is, the other functions on a Store read/write data to the instance itself. While it's not using this, it is using closures to the same effect. Hence, a replaceReducer function that returns another instance doesn't really feel in line with the rest of the API surface.

Put another way, it feels not too far off from just being a wrapper around const nextStore = createStore(nextReducer, store.getState()). In fact, that would work for any non-enhanced store.

Also, there is a subtle issue with subscribers, as replaceReducers dispatches an action right before returning, which would have them firing before the store state type has been updated (assuming you're doing something like globalThis.store = store.replaceReducer(nextReducer)). That would make it hard to communicate the type change to subscribers at the right time.

I'm not sure of the best way to resolve those issues, but I figure I should voice them now before we get too far.

[Methuselah96]

It's worth noting that adding the return type hasn't been released yet. So this is actually reverting a potentially breaking change that would've been released in v5, not making a new breaking change.

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 8edc1ff:

Sandbox	Source
Vanilla	Configuration
Vanilla Typescript	Configuration
redux-store-enhancer	Issue #3482

[Methuselah96]

Ideally the Store type would only need to know about S and A. Unfortunately this is necessary since replaceReducer should be able to take the reducer without StateExt being applied (the same reducer that was passed into createStore). My preference would be to remove StateExt altogether since I don't think there's a good practical use-case for it, but short of that, this seems like the necessary solution.

[Methuselah96]

I removed this test since this was explicitly testing for the behavior that is removed in this PR.

[Methuselah96]

Removed this test as well since it was written to test the idea that replaceReducer would return a store with new types, which is no longer the case.

[markerikson]

Okay, going to assume that this is good to go now.

Thanks for putting up with my delay here! :)