Skip to content

types: connect should allow null as the first argument #200

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

Closed
no-stack-dub-sack opened this issue May 12, 2018 · 3 comments · Fixed by #201
Closed

types: connect should allow null as the first argument #200

no-stack-dub-sack opened this issue May 12, 2018 · 3 comments · Fixed by #201

Comments

@no-stack-dub-sack
Copy link
Contributor

no-stack-dub-sack commented May 12, 2018

connect appears to allow a a null argument as the first argument, in the event that you only need to fire an action from a particular component, and do not need to map any state slices to your component's state. e.g.

constructor(public $ngRedux: ngRedux.INgRedux) {
  this.unsubscribe = $ngRedux.connect(null, componentActions)(this);
}

However, the types for connect are not written with an overload to support this. This forces you to write a mapStateToThis method that is ultimately not needed. It looks like there are some other changes holding up the 4.1.0 release, so this could be an easy fix to sneak in before that release.

Will you accept a PR to fix this?

@no-stack-dub-sack
Copy link
Contributor Author

@AntJanus do you happen to know who might be able to answer this for me? I'd love to make a contribution before the next release if I can (assuming this is something the team is interested in seeing).

@AntJanus
Copy link
Collaborator

@no-stack-dub-sack I'd accept a PR for it. It looks like the typing is a frequent issue and I don't have much knowledge about it.

@no-stack-dub-sack
Copy link
Contributor Author

@AntJanus cool , thanks - I'll work something up in the next few days, and I'll take a look through the rest of the file to see if anything else jumps out at me while I'm at it.

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 a pull request may close this issue.

2 participants