-
Notifications
You must be signed in to change notification settings - Fork 670
Issue/136 Support array argument for createSelector. #143
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
Conversation
Current coverage is 100%@@ master #143 diff @@
====================================
Files 1 1
Lines 13 13
Methods 0 0
Messages 0 0
Branches 0 0
====================================
Hits 13 13
Misses 0 0
Partials 0 0
|
I guess the size of the definition file doesn't really matter, kind of funny though! What needs to be done to get this merged? Are you looking for someone else to review it first? |
@threehams By the way, just gave you publish rights on NPM if you need to make a release. |
Thanks. It'd be good to have another pair of eyes on this - it's working well in a smaller project I'm using, but there may be a better way to handle this. @geon - think you could take a look? |
This would help remove some duplication, but it's only a proposal for now: |
) => TOutput | ||
): Selector<TInput, TProps, TOutput>; | ||
|
||
function createSelector< |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This definition is redundant. There is another, identical one at line 62.
@threehams: Yeah, it looks good to me. Haven't tried it. I noticed a redundant definition, though. |
Hey @aikoven and @ulfryk, I hope you don't mind me pinging you. I see you are the keepers of the Redux Typescript type definitions, and as Reselect is part of the Redux organisation I wondered if you could spare a little time to look at this PR too, it would be much appreciated. No problem if you don't have time, I realise everyone is really busy, but I figured it was worth a try! |
Closing to reopen against 3.0.0: #148 |
@ellbee Looks good to me. |
Thanks @aikoven! |
I used the latest definition file, it still complains. |
@beckend Make sure to replace the right definitions file. There is one in Is this fix going to be backported to version 2.5.x or will it only be available in 3.0.0? |
Sadly, I can't find a way to support either format without duplicating the same approach taken for the multiple-argument signature. The definition file is now 10X bigger than the library itself. :(
Fix some eslint complaints.
Use trailing commas consistently.
It's possible there's a way to do this with
...args: Selector<>[]
, but I can't figure out how to preserve the selector / combiner return/argument pairing with that approach. Changes to Typescript 2.1 (supporting object spread) might open up some possibilities here.