Skip to content

Changing subscribe's return type in Reducer #173

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

dawidzq
Copy link
Contributor

@dawidzq dawidzq commented Oct 19, 2017

Changed the return type of subscribe(listener: Function) since redux
defines it as "Unsubscribe". Although using Function works, it may result
in TypeScript errors in certain use cases.

http://redux.js.org/docs/api/Store.html#subscribe

Changed the return type of subscribe(listener: Function) since redux
defines it as "Unsubscribe". Although using Function works, it may result
in TypeScript errors in certain use cases.

http://redux.js.org/docs/api/Store.html#subscribe
@dawidzq
Copy link
Contributor Author

dawidzq commented Oct 20, 2017

Just to elaborate,

I am proposing this PR because I am getting typescript issues when trying to use another package, specifically redux-persist.

  Types of property 'subscribe' are incompatible.
    Type '(listener: Function) => Function' is not assignable to type '(listener: () => void) => Unsubscribe'.
      Type 'Function' is not assignable to type 'Unsubscribe'.
        Type 'Function' provides no match for the signature '(): void'.

I originally thought it was an issue with the redux-persist code base, but after doing some digging, I noticed that redux itself has an Unsubscribe data type that is declared as:

export interface Unsubscribe {
  (): void;
}

This is obviously a Function-like definition and seems to fall in line with the behavior of the subscribe function (being that it returns an unsubscribe function when called). I went into the ng-redux code and made the same change inside my project that I am proposing in this PR, and it didn't seem to break anything while simultaneously resolving the above Typescript issue. I can't imagine importing Unsubscribe from redux being a problem either, since redux is a dependency for ng-redux.

I'm fairly new to the industry, so I would greatly appreciate any feedback if I'm not understanding something, but I'm fairly confident that this is a harmless fix to something that could be affecting other packages.

@AntJanus AntJanus merged commit 7706abd into angular-redux:master Oct 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants