Skip to content
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

Issue/136 Support array argument for createSelector. #143

Closed
wants to merge 2 commits into from

Conversation

threehams
Copy link
Collaborator

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.

@coveralls
Copy link

coveralls commented Jun 29, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling 9bda1cc on threehams:master into 2f1a967 on reactjs:master.

@codecov-io
Copy link

codecov-io commented Jun 29, 2016

Current coverage is 100%

Merging #143 into master will not change coverage

@@           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          

Powered by Codecov. Last updated by 2f1a967...9bda1cc

@ellbee
Copy link
Collaborator

ellbee commented Jun 29, 2016

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?

@ellbee
Copy link
Collaborator

ellbee commented Jun 29, 2016

@threehams By the way, just gave you publish rights on NPM if you need to make a release.

@threehams
Copy link
Collaborator Author

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?

@threehams
Copy link
Collaborator Author

This would help remove some duplication, but it's only a proposal for now:
microsoft/TypeScript#5296

) => TOutput
): Selector<TInput, TProps, TOutput>;

function createSelector<
Copy link
Contributor

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.

@geon
Copy link
Contributor

geon commented Jul 1, 2016

@threehams: Yeah, it looks good to me. Haven't tried it. I noticed a redundant definition, though.

@ellbee
Copy link
Collaborator

ellbee commented Jul 3, 2016

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!

@threehams
Copy link
Collaborator Author

Closing to reopen against 3.0.0: #148

@aikoven
Copy link
Contributor

aikoven commented Jul 5, 2016

@ellbee Looks good to me.

@ellbee
Copy link
Collaborator

ellbee commented Jul 5, 2016

Thanks @aikoven!

@threehams threehams closed this Jul 6, 2016
@beckend
Copy link

beckend commented Sep 1, 2016

I used the latest definition file, it still complains.

@tobias-kuendig
Copy link

@beckend Make sure to replace the right definitions file. There is one in src and lib.

Is this fix going to be backported to version 2.5.x or will it only be available in 3.0.0?

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.

8 participants