Enhancers extending state will not work for all reducers

[Methuselah96]

Prior Issues

#1648

What is the current behavior?

The type for enhancers imply that enhancers should be allowed to extend the state. This is problematic for simple states that are just a number or a string because there is no way to extend them while still maintaining the original string or number. If you spread a string or a number, you'll get an empty object back, so if an enhancer tries to extend state, they'll likely end up erasing the user's state.

Steps to Reproduce

function counter(state = 0, action: AnyAction) {
  switch (action.type) {
    case "INCREMENT":
      return state + 1;
    case "DECREMENT":
      return state - 1;
    default:
      return state;
  }
}

interface MyStateExt {
  bar: string;
}

function makeEnhancer(): StoreEnhancer<{}, MyStateExt> {
  return (createStore: StoreEnhancerStoreCreator) => <S, A extends Action>(
    reducer: Reducer<S, A>,
    preloadedState?: PreloadedState<S>
  ): Store<S & MyStateExt, A> => {
    const store = createStore(reducer, preloadedState);

    return {
      ...store,
      getState: () => ({ ...store.getState(), bar: "test2" }),
      replaceReducer: (reducer: Reducer<S & MyStateExt, A>) => {
        throw new Error("Not implemented.");
      }
    };
  };
}

const store = createStore(counter, makeEnhancer());
// User expected store.getState() to return their counter number,
// but instead it returns { bar: "test2" } from the enhancer
const state = store.getState();

https://codesandbox.io/s/unruffled-faraday-u502r?fontsize=14&hidenavigation=1&theme=dark

What is the expected behavior?

The types should not allow you to create an enhancer that tries to extend the state because it's not possible to do so if the state is a string or number.

Environment Details

N/A

[Methuselah96]

@timdorr Any insights into #1648 or why we would want to let enhancers extend state?

[timdorr]

As a store enhancer, they have quite a bit of control over the behavior and shape of the resulting store. They may want to add things to state related to the things they are enhancing. I could see a WebSocket enhancer that exposes the global connection instance via getState(). The main reason for doing that would be to allow access during connect() or useSelector(). So, I think it should stay.

[Methuselah96]

But how would the WebSocket enhancer add to the state if the state is just a number or a string?

[Methuselah96]

It seems to me like the preferable way for a library to extend state would be to expose a reducer and an enhancer and have the user provide the reducer in combineReducers themselves. I've seen quite a few libraries export a reducer that is expected to get mixed in (e.g., https://github.com/supasate/connected-react-router#step-1).

Also, if the enhancer mixes in the state themselves, it seems like it would be hard to make sure that the key for that state isn't already used. It would be easier to ensure that if the user has to mix in the reducer themselves.

Finally, are there are any real-life examples of enhancers that extend state? It would be helpful to see what that would look like to help inform the conversation.

[Methuselah96]

Also worth noting that this is really a conversation about types. If an enhancer really wants to extend state, they can write the JavaScript to do so. However, providing a StateExt type provides the illusion that it's safe to extend the state for any generic state. In the example I gave above, everything compiles, but the result is not safe. This is because TypeScript allows you to spread an unrestrained generic (still looking into this).

Removing the StateExt generic wouldn't make it impossible to extend the state from a JavaScript perspective, but from a TypeScript perspective it will prevent cases that we normally expect TypeScript to catch.

If a library wants to be able to extend the state using TypeScript, they can expose a createStore method themselves that returns a store with the extended state (with the correct type) and just do the cast internally to the library. The library takes on the liability of exposing a createStore method that doesn't work for all reducers, and it's just as simple from an end-user perspective.

[timdorr]

But how would the WebSocket enhancer add to the state if the state is just a number or a string?

They would need to handle it in the setup function, I would imagine. Simple state values aren't typically used with enhancers like that. I would say that's an extreme edge case.

[Methuselah96]

Still seems like it wouldn't be too hard for an enhancer to just have the user add the reducer themselves. It will also prevent the enhancer from having to deal with the edge cases where the state isn't an object or the key already exists in the reducer.

It could be implemented like this:

// Enhancer code
interface WebSocketConnection {}
declare function makeConnection(): WebSocketConnection;

interface WebSocketElements {
  webSocketReducer: Reducer<{ connection: WebSocketConnection }, AnyAction>;
  webSocketEnhancer: StoreEnhancer<{ extendStoreProperty: {} }>
}

function connectToWebSocket(): WebSocketElements {
  const connection = makeConnection();

  return {
    webSocketReducer: (state, action) => ({ connection }),
    webSocketEnhancer: (createStore) => (reducer, preloadedState) => ({
      ...createStore(reducer, preloadedState),
      extendStoreProperty: {
        // do some fancy stuff in here
      }
      }),
  }
}

// User code
const { webSocketReducer, webSocketEnhancer } = connectToWebSocket();

const reducer = combineReducers({
  webSocket: webSocketReducer,
  // rest of your reducers
});

const store = createStore(reducer, webSocketEnhancer);

In summary:

1. As far as I can tell, enhancers extending state is not very common (I don't know of any real-life examples).
2. There's already precedent for having users add parts of state to combineReducers themselves (e.g., https://github.com/supasate/connected-react-router#step-1).
3. It simplifies the types for enhancers in Redux.
4. It makes the types for enhances in Redux more type-safe.
5. It provides better safety for the enhancer and allows them to not have to deal with the cases where the state isn't an object or the key already exists in the reducer.

Since it doesn't seem very common, it simplifies the Redux types, and there's better type-safety all around, I still think it's a net positive to remove the StateExt generic.

[phryneas]

A number or a string can be assigned additional properties, which converts them to Number or String objects.

So in JavaScript, this is possible.

const numberWithProps = Object.assign(5, {foo: "bar"})

and results in a Number & { foo: "bar" } that behaves just like 5 with an additional property.

But really, this is more of a typing issue. It doesn't sound reasonable to me to remove a feature that might or might not be used by who-knows-how-many of the 13k redux-related packages currently on npm.
Instead, the type could be modified to simply only accept object-like types to extend?

[Methuselah96]

Nice, didn't realize you could do that.

As you said, this is a typing issue. In reality, an enhancer can do whatever it wants to a store. It could change the state of shape completely (not just extend the state), which is something our types don't support.

With any other type in Redux, I know of a package that uses that type. However, I have not seen a real-life usage of StateExt, which is why I call it into question. Even less likely is that this enhancer uses TypeScript. If TypeScript supported deprecating, then we could deprecate the type to see how many people are actually using it and then just un-deprecate it if it's being used, but I don't think TypeScript currently supports deprecating types.

It seems odd to support a feature that considerably complicates our types that "who-knows-how-many of the 13k redux-related packages currently on npm" are relying on if that number could reasonably be 0.

I'm not too worried about constraining the type to be just object-like, my main push for this change would be to simplify the types.

[Methuselah96]

My original concern of enhancers extending state not working for all reducers has been addressed. Thanks!