Typescript definition does not allow for enhancer composition

[santiagoaguiar]

What is the current behavior?

export type StoreEnhancerStoreCreator<S> = (reducer: Reducer<S>, preloadedState?: S, enhancer?: StoreEnhancer<S>) => Store<S>;

Does not allow an enhancer argument, therefore the

This doesn't let you create a enhancer like https://github.com/reactjs/redux/blob/master/src/applyMiddleware.js with typescript, since you can't chain the enhancers.

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.

import * as Redux from "redux";
interface State {}
export const enhancer: Redux.StoreEnhancer<State> = createStore => (reducer, preloadedState, enhancer?) => {
    return createStore(reducer, preloadedState, enhancer) ;
}

Does not compile.

What is the expected behavior?
Should compile.

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

Given #1648 I may be missing something, but I think changing it to

export type StoreEnhancerStoreCreator<S> = (reducer: Reducer<S>, preloadedState?: S, enhancer?: StoreEnhancer<S>) => Store<S>;`

would be enough until that issue is resolved.

[aikoven]

I guess you're right, however as you can see in createStore code, enhancer isn't passed to the enhanced store creator:

function createStore(reducer, preloadedState, enhancer) {
  // ...
  return enhancer(createStore)(reducer, preloadedState)
  // ...
}

Thus, in applyMiddleware the value of enhancer is almost always undefined, so you can't rely on it inside the enhancer. You can use compose instead.

[begincalendar]

@santiagoaguiar I'm not sure this issue is dependent on #1648, because as Dan has labelled it, that issue just appears to be an enhancement, not a bug (like this issue).

Having said that, I'm confused by this...

To me it appears the TypeScript definition for StoreEnhancer is correct and that the problem is actually that the enhancer API (and implementation) do not allow for composition.

Yet that doesn't seem right, because I have seen no other mention of this train of thought.

---

This is my train of thought:

If we mentally alias StoreEnhancer as X and StoreEnhancerStoreCreator as Y, then the definitions simplify to:

X: (next: Y) => Y;

So assuming X and Y are unrelated, how are you supposed to compose multiple X's?

composedEnhancer = enhancer1()(
  enhancer2()
)

...(which is what compose effectively does for two enhancers), does not compile, because X takes in a Y, not another X.

[santiagoaguiar]

You would have:

StoreCreator {
    (..., StoreEnhancer): Store
}

StoreEnhancer = (StoreCreator) => StoreCreator;

so to create a composed enhancer you would do:

createStore: StoreCreator;
x,y,z: StoreEnhancer
x(y(z(createStore)))

Or:

compose(x,y,z)(createStore)

And the implementation of a store enhancer would basically do:

return (createStore) => (reducer, state, enhancer) => /* do something */ createStore(reducer, state, enhancer);

I understand that given the current implementation of create store, enhancer would be undefined, but the typings are a bit confusing, and the implementation of applyMiddleware.js, which for me was 'the' reference on how to build the enhancer, gives the bad impression (why receive the enhancer and pass the enhancer if it will be undefined? applyMiddleware would fail to compile given the current typings).

Maybe I wouldn't have opened this bug if applyMiddleware didn't receive the enhancer, but when I looked at the implementation I assumed the typings were wrong and that by not doing what applyMiddleware was doing, I could break something down the road ;).

I'm OK with closing the bug if typings are OK and applyMiddleware is the offender :).

[aikoven]

@santiagoaguiar You may want to check out #2201

[santiagoaguiar]

Thanks, seems this is a tiresome issue :), but these are my takeaways:

1. If enhancer is not needed, it should go (seems it's not the case)
2. If enhancer might be needed, it should be present on the typescript definition.
3. If enhancer is only needed on applyMiddleware for some reason I still don't get, then maybe add a comment saying needed to support old enhancer usage style, not needed on new enhancers or whatever :), using ...args doesn't improve the situation IMO, applyMiddleware is the go to file to get an idea of how an enhancer should work.

[timdorr]

Are we good with #2563 in place? (on the next branch)