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

added typings for dynamic array of uniform selectors #315

Merged
merged 1 commit into from
Jun 22, 2018

Conversation

sergey-su
Copy link
Contributor

Currently array version of createSelector typing has fixed number of selectors known at compile-time. However createSelector's implementation supports any number of selectors determined at run-time. Technically input selectors can have different return types. That's impossible to describe in TypeScript. But it's possible to describe the case when all selectors have same return type. For example when input selectors are generated for each element of a container that has nr of elements fixed during selector's lifetime:

createSelector(containerWithFixedNrOfElements.map(obj => () => obj.value), (...values) => expensiveCalculation(values))

@coveralls
Copy link

coveralls commented Jan 6, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling ba61be7 on sergey-su:dynamic-selectors-array into 660de49 on reactjs:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling ba61be7 on sergey-su:dynamic-selectors-array into 660de49 on reactjs:master.

@coveralls
Copy link

coveralls commented Jan 6, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling 36f256b on sergey-su:dynamic-selectors-array into 978bd89 on reactjs:master.

@aikoven
Copy link
Contributor

aikoven commented Jan 7, 2018

👍

@sergey-su
Copy link
Contributor Author

Can anyone re-trigger the travis build? The first one failed with an issue not related to this change.

What else this PR is missing to proceed?

@codecov-io
Copy link

codecov-io commented Jan 17, 2018

Codecov Report

Merging #315 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #315   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           1      1           
  Lines          15     15           
=====================================
  Hits           15     15

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 978bd89...36f256b. Read the comment docs.

@ellbee
Copy link
Collaborator

ellbee commented Apr 19, 2018

Hey @sergey-su, sorry to be so late to this. I just merged another long-outstanding typings pr and seem to have introduced some merge conflicts with this pr. If you resolve them and we can find someone to review (I don't use typescript), then I'll get this merged too.

@sergey-su
Copy link
Contributor Author

@ellbee , PR branch rebased

@jsprds
Copy link

jsprds commented Jun 22, 2018

@ellbee @DmShpak is it possible to merge this PR? Thanks!

@ellbee
Copy link
Collaborator

ellbee commented Jun 22, 2018

I was hoping for someone who uses Typescript to approve it, but I see @aikoven approved it before the rebase so I'll merge.

@ellbee ellbee merged commit 25ad346 into reduxjs:master Jun 22, 2018
@ellbee ellbee mentioned this pull request Jun 22, 2018
@jsprds
Copy link

jsprds commented Jun 22, 2018

@ellbee Thanks! When are you planning a release that includes this fix?

@ellbee
Copy link
Collaborator

ellbee commented Jun 22, 2018

#351

@ellbee
Copy link
Collaborator

ellbee commented Jun 22, 2018

I'll likely release a beta sometime over the weekend. There is another TypeScript related issue that I'd like to see if we can clear up (#340) as part of v4 as I don't feel comfortable making TypeScript changes in minor versions. I don't use TypeScript personally so any help would be much appreciated!

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.

7 participants