Dispatch interface should not be generic

[connorwyatt]

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

A small typings refactor.

What is the current behavior?

The Dispatch interface is currently generic, when it doesn't need to be, the generic type is never used. I don't know if there is a particular reason why it has been done the way but it isn't documented either.

Dispatch interface

As I mentioned, I'm not sure if there is a reason why it has been done this way but I feel that it should either be changed or documented.

What is the expected behavior?

There should be no generic.

[TrySound]

It should be generic like here. The problem is realization.

[connorwyatt]

What do you mean by that? I see now that the implementation should be:

export interface Dispatch<A> {
    <A extends Action>(action: A): A;
}

Are you happy for me to create a PR?

[connorwyatt]

More like this actually:

export interface Dispatch<A extends Action> {
    (action: A): A;
}

[connorwyatt]

I have just found out that this causes other issues due to how the typings are written. For example here: dispatch method on Store, the store uses the state as the generic type of the dispatch function, however this seems to be incorrect to me. Am I missing something?

There are more examples of the state being used elsewhere in the types as well.

[timdorr]

This doesn't appear to be changed on the next branch, but if you want to do a PR, please do it against that branch.

I have only a passing knowledge of TS, and what you say seems correct, but I can't verify it myself.

[frankwallis]

The type parameter is used when the dispatch gets overloaded - for example by redux-thunk:

https://github.com/gaearon/redux-thunk/blob/master/index.d.ts#L9

[connorwyatt]

@frankwallis Thanks for letting me know, I didn't realise that was a use case. @timdorr because of this I'll have to have a look at how it would integrate with other types (like redux-thunk) and then I will submit a PR.

My first thoughts on it are that I would expect to have to make Dispatch generic, it should (imho) be made generic by the overload as it intends to use the type. If anyone knows more on this let me know.

I'll get back to you once I've investigated.

[connorwyatt]

As a side note kudos on including the types with your packages, it's a much nice developer experience to not have to install a separate @types package that may be out of sync with the main package (version wise). 👍

[connorwyatt]

So after investigating, it appears that in order to be overloaded, the base interface has to have the same signature. If I remove the generic, it can't be declared with a generic later on.

From what I can see it looks like it can't be changed as it would break the interoperability with other libraries, but I would inclined to include a comment as to why it is the way it is (as it requires you to have knowledge of a third party library's intentions).

What are your thoughts on this @frankwallis and @timdorr?

[aikoven]

Adding a comment near the definition is the best we can do now without introducing a breaking change.

With generic defaults introduced in TS 2.3 we can update it as follows:

interface Dispatch<S = any> {
  // ...
}

This way users won't have to set a parameter explicitly every time they use Dispatch type.

However, this would still be a breaking change. The next branch is a good place for stuff like this.

[connorwyatt]

@aikoven Thanks for the link, I wasn't aware of that feature 👍.

What is your policy regarding TS version compatibility? Do you have a minimum version you are tying to support?

[aikoven]

@connorwyatt Unfortunately, there's currently no robust way of keeping typings together with the source package while maintaining different versioning for the package and typings. And the @types and the DT is a hell for maintainers.

This means that we can't introduce any breaking change in typings without bumping major version of Redux itself. There are already some changes in the typings in the next branch, however, no one knows when Redux 4 will be released.

You might also be interested in microsoft/types-publisher#4, which is a possible solution for being able to maintain typings in a separate (non-DT) repo while still being able to install them via @types.

[connorwyatt]

@aikoven I understand, I am currently just typing the dispatch generic to any which is fine as a temporary workaround so I'm not too worried about it.

I can add the comment to the line just so that there is no confusion for other devs if they look into the typings as well to work out what the generic should be.

[timdorr]

If you want to make this against next, which is already at TS 2.1, then that would be fine.

[winseros]

Folks, and what would you think of marking the existing generic interface as deprecated, and placing a new one with the correct signature nearby? That will not break the backwards compatibility for the existing libraries.

[hodavidhara]

Any movement on this? I know it's only a minor annoyance, but it's really ugly to have to constantly provide a type parameter that has no effect at all in projects not using middleware like redux-thunk.

[bdwain]

Typescript's Do's and Don'ts page says right near the top to never have unused type parameters

Generics
Don’t ever have a generic type which doesn’t use its type parameter. See more details in TypeScript FAQ page.

Not sure if this is a known exception to that rule, but the linked FAQ explains some problems that can occur from this.

[timdorr]

Fixed by #2563

[nathan-chappell]

Sorry for commenting on a closed issue, but I got led here trying to understand the current implementation:

export interface Dispatch<A extends Action = AnyAction> {
  <T extends A>(action: T): T
}

As far as I can tell this only works due to Function Parameter Bivariance. I hope this may help someone if they stumble upon this.