TypeScript defintion of StoreEnhancer does not allow state extension with replaceReducer #3482
Comments
mhelmer
changed the title
|
This actually prevents moving the |
|
I'm not sure about that fix since it essentially removes the type safety from I'm thinking it could be solved by adding an extra type generic to the export interface Store<S = any, A extends Action = AnyAction, ReplacedState = S> {
/*... */
replaceReducer(nextReducer: Reducer<ReplacedState, A>): void
/*... */
} The 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> & ExtThis maintains type safety and makes it possible to call I could open a PR if you think it's a reasonable solution. |
|
@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! |
|
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. |
|
Ok, I'll see if I can break it, thanks for the notice |
|
@markerikson and @timdorr is there any history behind the typings for 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:
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 |
|
I have always stayed away from any typings discussions, so I have no idea what's changed when. FWIW, while passing 0 middlewares to 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. |
|
Looks like this issue is resolved in the TS rewrite, so I'll close this out. Stay tuned for release... |
|
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 Post #3507 we can call Also, my concerns for the typings were about the reducer input, which can now be any reducer. |
|
I see the issue. The problem I fixed was that |
|
@cellog: I tried against the branch, but I could not get it to work. I'm not sure if I'm missing something here.. import {
StoreEnhancer,
Action,
AnyAction,
Reducer,
createStore,
PreloadedState
} 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?: PreloadedState<S>
) => {
const wrappedReducer: Reducer<S & ExtraState, A> = null as any;
const wrappedPreloadedState: PreloadedState<S & ExtraState> = null as any;
const store = createStore(wrappedReducer, wrappedPreloadedState);
return {
...store,
replaceReducer: (nextReducer: Reducer<S, A>) => {
const nextWrappedReducer: Reducer<S & ExtraState, A> = null as any;
return store.replaceReducer(nextWrappedReducer);
}
};
};
const store = createStore(reducer, enhancer);
store.replaceReducer(reducer);
}I get the following error: |
|
Thanks for trying it out, I'll see if I can figure out why it fails |
|
OK, so here's the deal. The reason this fails is that the generic for state used in the definition of This is fixable in your example by adding return {
...store,
replaceReducer: (nextReducer: Reducer<S, A>) => {
const nextWrappedReducer: Reducer<S & ExtraState, A> = null as any
return store.replaceReducer(nextWrappedReducer)
}
} as Store<S & ExtraState, A, ExtraState, {}>I am uncertain if there is a way in typescript to get inference from a generic and not allow passing in arbitrary types. If there is, we would need to use that here. Otherwise, this will need to be updated in documentation as well. What do you think? |
|
@mhelmer I spent several hours on this, and asked advice of a typescript expert, and basically this is an OK way to solve this particular issue. I don't think there is a simpler way unless we completely rewrite the types for |
|
Apologies for my slow reply. If |
|
I'm planning on doing a major for this TS rewrite, so a Store rewrite is totally on the table, FWIW. |
|
I think there is hope yet. This code: interface Store {
a<S>(t: S): S[]
}
interface EnhancedStore<Extension> extends Store {
a<S>(t: S & Extension): (S & Extension)[]
}
const f: Store = {
a: (t) => {
return [t]
}
}
function enhance<Extension>(s: Store, t: Extension): EnhancedStore<Extension> {
return {
a: (a) => {
return [{
...a,
...t
}]
}
}
}works without error. I'm going to gradually make it more complex until it hits the sweet spot |
|
Just for some context on my use case here with a concrete example. I'm using The setup wraps the root reducer and adds some extra state ( function configureStore(preloadedState) {
// ...
if (process.env.NODE_ENV !== 'production' && module.hot) {
module.hot.accept('src/reducers', () => { store.replaceReducer(rootReducer) });
}
}So, my enhancer looks like this: function createPersistEnhancer(persistConfig: PersistConfig) {
return (createStore) => (reducer, preloadedState, enhancer) => {
const persistedReducer = persistReducer(persistConfig, reducer);
const store = createStore(persistedReducer, preloadedState, enhancer);
const persistor = persistStore(store);
return {
...store,
replaceReducer: (nextReducer) => { store.replaceReducer(persistReducer(persistConfig, nextReducer)) },
persistor,
};
};
} |
|
Further update: I tried removing some of the weird typing hacks in the tests like: const wrappedReducer: Reducer<S & ExtraState, A> = null as any
const wrappedPreloadedState: PreloadedState<S & ExtraState> = null as anyand replacing them with real code: const wrappedReducer = (state: S & ExtraState | undefined, action: any) => {
return {
...(state ? state : {}),
extraField: 'extra'
}
}
const wrappedPreloadedState = {
...preloadedState,
extraField: 'extra'
}to get: function mhelmersonExample() {
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?: PreloadedState<S>
) => {
const wrappedReducer = (state: S & ExtraState | undefined, action: any) => {
return {
...(state ? state : {}),
extraField: 'extra'
}
}
const wrappedPreloadedState = {
...preloadedState,
extraField: 'extra'
}
const store = createStore<S & ExtraState>(wrappedReducer, wrappedPreloadedState)
return {
...store,
replaceReducer: <NewState extends S, NewActions extends A>(
nextReducer: Reducer<NewState, NewActions>
) => {
const nextWrappedReducer = (state: S | undefined, action: any) => {
return {
...(state ? state : {}),
extraField: 'extra'
}
}
return store.replaceReducer(nextWrappedReducer)
}
}
}
const store = createStore(reducer, enhancer)
store.replaceReducer(reducer)
}
}Now, and this is very interesting, the type failure is at Note that the |
|
OK!! I am satisfied I have found a solution that actually works. First of all, the fix. 8ca8290#diff-b52768974e6bc0faccb7d4b75b162c99L345 It turns out that by providing default values, type inference was short-circuiting, and causing the error. Next, the tests were all using contrived non-realistic examples, so I fixed the tests, first here: 8ca8290#diff-09c07e35fbab9793e5f037dc3c72c878L128 and then in a follow-up commit for the rest of the tests: As you can see, the final example of your code, now working, is: function finalHelmersonExample() {
function persistReducer<S>(config: any, reducer: S): S {
return reducer
}
function persistStore<S>(store: S) {
return store
}
function createPersistEnhancer(persistConfig: any): StoreEnhancer {
return createStore => <S, A extends Action = AnyAction>(
reducer: Reducer<S, A>,
preloadedState?: any
) => {
const persistedReducer = persistReducer(persistConfig, reducer)
const store = createStore(persistedReducer, preloadedState)
const persistor = persistStore(store)
return {
...store,
replaceReducer: nextReducer => {
return store.replaceReducer(
persistReducer(persistConfig, nextReducer)
)
},
persistor
}
}
}
}Hopefully this is the right solution! Let me know if it works for you as well as it does here @mhelmer |
|
Nice. I will try it out this evening. Just one thing about that final example. The function persistReducer<S, A>(reducer: Reducer<S, A>): Reducer<S & ExtraState, A> {
// ...
} |
|
just pushed a commit that adds this detail, and a few more granular tests to ensure typing is propagated to the store. |
|
Alright, so it seems to work now. Need to do unsafe cast of |
|
I have a similar issue which I believe is related. I'm trying to create an enhancer which adds a reducer, but I cannot for the life of me figure out how to type it. The types below (CodeSandbox) are clearly incorrect, but are the ones that yield least errors. Soon as I import {
combineReducers,
StoreEnhancer,
Dispatch,
PreloadedState,
StoreEnhancerStoreCreator,
AnyAction,
Reducer
} from "redux";
const extraReducer = combineReducers({ dummy: () => null });
// type StateExt = ReturnType<typeof extraReducer>;
export default function tools(): StoreEnhancer {
return (createStore: StoreEnhancerStoreCreator) => <S, A extends AnyAction>(
reducer: Reducer<S, A>,
preloadedState?: PreloadedState<S>
) => {
const rootReducer: Reducer<S, A> = (state, action) => {
const intermediateState = reducer(state, action);
const finalState = extraReducer(intermediateState, action);
return finalState;
};
const store = createStore(rootReducer, preloadedState);
const dispatch: Dispatch<A> = (action) => {
console.log(action);
return store.dispatch(action);
};
return { ...store, dispatch };
};
}Context: We wish to provide features to users that they can mix-and-match; each feature is a combination of a reducer and a middleware (and in one case also add to the store API), so we wish to ship these as enhancers. |
|
No current answers here, but this is something that would be nice to resolve as part of a Redux 5.0. @Izhaki fwiw, have you considered shipping that feature in some other form? For example, something like https://github.com/microsoft/redux-dynamic-modules or https://github.com/fostyfost/redux-eggs could let you define the correlated reducer+middleware as more of its own combined/encapsulated piece. |
|
It's been a while since I've looked at this, but I believe #3772 resolves this issue. |
|
There's an example in the tests for that PR that shows how the state could be extended. |
|
Thanks guys! redux-eggs looks awesome, but does not seem to support store API extensions (and in our case it's an additional dependency of a library to a library). I've added my comment not because I can't get it to function, but because I can't seem to be able to type it. I'll be looking forward for the fix to be merged and then verify it with my setup. |
Do you want to request a feature or report a bug?
The
StoreEnhancerTypeScript interface does not seem to have the appropriate type for thereplaceReducerproperty of the enhanced store. I would consider it a bug in the type definition.What is the current behavior?
When implementing the
StoreEnhancerinterface withExtraState, the type signature ofreplaceReduceron the "enhanced" store is coupled to the type of the wrapped reducer.Given a
StoreEnhancerof typethe returned store is of type
with the
replaceReducerproperty as suchReturning a store with a
replaceReducerthat accepts the original reducer gives the following type-error: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:
See
src/index.tsin 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.tsin 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
replaceReduceron the returned store can be called with the originalrootReducer.It would be the responsibility of the
enhancerto appropriately replace the wrapped reducer. I.e return a store such as: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
The text was updated successfully, but these errors were encountered: