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

Move combiner function before selector functions for type safety #366

Closed
nickmccurdy opened this issue Aug 17, 2018 · 4 comments
Closed

Comments

@nickmccurdy
Copy link
Contributor

nickmccurdy commented Aug 17, 2018

In JavaScript, spread parameters are only valid as the last parameter of a function to prevent ambiguity. Type checkers like TypeScript and Flow rely on this assumption as well for checking function argument types.

Unfortunately, even though TypeScript (as of version 3) and Flow support spread parameters, they cannot be used with Reselect's incorrect spread argument order. The combiner function is last, when it should be first so the selector functions can be a final spread parameter. This is necessary to take full advantage of all possible usages of Reselect besides the argument combinations hard coded in the TypeScript and Flow definitions, and also solves semantic confusion where Reselect has a different argument order for variable argument orders than most JavaScript APIs.

Alternatively, we could simply remove the API signatures that don't use the Array of selector functions. This would not break any code that is only using Arrays of selector functions.

@dcurletti
Copy link

dcurletti commented Sep 19, 2018

+1 the benefits of this would also include a better typing signature.

const fooSelector = (state: RootState) => state.foo;

const barSelector = createSelector(fooSelector, foo => {
  fooBar: foo.bar;
});

Rather than barSelector being inferred, I want to be able to declare its return value without having to pass in: RootState, Foo beforehand.

@ellbee ellbee mentioned this issue Sep 21, 2018
@AlexeiDarmin
Copy link

I reached the same conclusion to move the combiner as the first function but didn't say anything since I figured it would be too disruptive to the library. I'm all for this.

This also could allow for some better features moving forward supposing the spread operators gets better and better support by TypeScript

@nickmccurdy
Copy link
Contributor Author

TS now supports mapping tuples, including arguments. This might be enough of a workaround to get strong typing on the current API, but it would still be much easier and simpler to fix the location of the combiner function or remove the other signatures.

@markerikson
Copy link
Contributor

Based on the changes in #486 and #513 , I think this is now obsolete. We can keep the combiner last and still get perfect inferred types, no API changes needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants