Update TypeScript definitions and tests

[aikoven]

Original issue: #125

cc. @threehams

[coveralls]

Coverage remained the same at 100.0% when pulling 0aed9b7 on aikoven:3.0.0 into f63e49c on reactjs:3.0.0.

[codecov-io]

Current coverage is 100% (diff: 100%)

Merging #192 into 3.0.0 will not change coverage

@@           3.0.0   #192   diff @@
===================================
  Files          1      1          
  Lines         12     12          
  Methods        0      0          
  Messages       0      0          
  Branches       0      0          
===================================
  Hits          12     12          
  Misses         0      0          
  Partials       0      0          

Powered by Codecov. Last update f63e49c...8c48a39

[frankwallis]

I think this should be props.bar not state.bar?

To avoid this issue it would be nice to turn on noImplicitAny for the tests and then specify type never for unused params?

(state: never, props: {bar: number}) => props.bar,

[frankwallis]

In general these seem to be working really well, I have added a few comments. The main thing I wanted to check was that this code still worked:

  const selector2 = createSelector(
      (_, props: {id: string}) => props.id,
      (state: {foos: {[id: string]: string}}) => state.foos,
      (id, foos) => foos[id]
    );

  selector2({foos: { '1': 'bar'}}, {id: '1'});
  // typings:expect-error
  selector2({foos: { '1': 'bar'}}, {notid: '1'});
  // typings:expect-error
  selector2({notfoos: { '1': 'bar'}}, {id: '1'});

Which it does, and it is covered by testParametricSelectors.

I found it interesting that specifying type any for the unused state parameted _ caused the resulting selector to widen its state parameter to any, which is not what I wanted. Using type never for the unused parameter does seem to have the desired effect though and allows you to turn on noImpilictAny

I will try them out against a whole application next week and report back

[frankwallis]

This is hitting the array with > 8 elements function declaration and foo1 is inferred to have type any I'm not sure that this is intended?

[aikoven]

Yep, apparently return types of input selectors are not inferred and this hack doesn't work:

export function createSelector<S, P, R1, ..., R8, SS extends [
  ParametricSelector<S, P, R1>,
  ...,
  ParametricSelector<S, P, R8>
] & {[index: number]: ParametricSelector<S, P, any>}, T>(
  selectors: SS,
  combiner: (res1: R1, ..., res8: R8, ...rest: any[]) => T,
): ParametricSelector<S, P, T>;

I guess we'll have to stick with

export function createSelector<S, P, T>(
  selectors: ParametricSelector<S, P, any>[],
  combiner: (...rest: any[]) => T,
): ParametricSelector<S, P, T>;

[frankwallis]

Probably should validate the shape of the result of calling selector2? Ditto for parametric below..

[aikoven]

@frankwallis Thanks. I've enabled noImplicitAny. There's a problem with >8 selectors in array and I'm not sure there's a way to fix it. We could do

export function createSelector<S, P, T>(
  selectors: ParametricSelector<S, P, any>[],
  combiner: (...rest: any[]) => T,
): ParametricSelector<S, P, T>;

but it would kill all the type safety for previous overloads.

[coveralls]

Coverage remained the same at 100.0% when pulling dd1003f on aikoven:3.0.0 into f63e49c on reactjs:3.0.0.

[frankwallis]

@aikoven - yes I don't think that can be done currently, there is an open typescript issue about it here. I think the issue also exists with the >8 parameters version as well as the array version (I have added a comment).

I suppose the question is whether it is better to leave them out for now or include them? This may be solvable in 2.1.3 using the new 'mapped types' functionality (microsoft/TypeScript#12114 (comment)) but I haven't tried it yet..

[frankwallis]

bar5 should be number here? But as discussed the type inference is not working with the rest arguments (this is the issue I refer to in my main comment)

[aikoven]

@frankwallis

This may be solvable in 2.1.3 using the new 'mapped types' functionality

I can't find a way to apply mapping to an array. I tried this:

export function createSelector<S, R extends any[], T>(
  selectors: {[K in keyof R]: Selector<S, R[K]>},
  combiner: (...values: R) => T,
): Selector<S, T>;

But there are two problems:

1. keyof R is number | "length" | "push" | ..., not 0 | 1 | 2 | ....
2. (...values: R) => T doesn't work either, produces error A rest parameter must be of an array type.

Maybe it'd be better if we completely left out the ...rest overloads for the sake of type safety. We can provide enough overloads to cover most cases. Besides, one call to createSelector can usually be split:

createSelector(
  selector1, selector2, selector3, selector4,
  (v1, v2, v3, v4) => {...}
)
// vs
createSelector(
  createSelector(selector1, selector2, (v1, v2) => ({v1, v2})),
  createSelector(selector3, selector4, (v3, v4) => ({v3, v4})),
  ({v1, v2}, {v3, v4}) => {...}
)

[frankwallis]

Yes I found the same thing, mapped types don't seem to work for arrays/tuples :(. I agree that the ...rest declarations should be left out for now, perhaps provide overrides up to 12 selectors and call it done?

[Jessidhia]

Hm... it doesn't seem to be possible to receive additional arguments (besides the singular props) on the selectors? Though this is already the case with the existing definitions.

[aikoven]

@Kovensky You're right, I'll fix that.

I'd also like to wait until TS 2.1 is released to correctly type createStructuredSelector.

[aikoven]

Updated PR.

Added support for extra args in selectors, removed ...rest versions of createSelector, updated to TS 2.1.

[coveralls]

Coverage remained the same at 100.0% when pulling 8c48a39 on aikoven:3.0.0 into f63e49c on reactjs:3.0.0.

[threehams]

Sorry for being quiet for a while (crazy project at work), but I've been watching, and @frankwallis has been doing a great job of reviewing. Sounds like you have a more comprehensive project than mine.

I've been spending the last few days updating a six-month-old project which uses Reselect pretty heavily so I can check out the typings in a real environment. While doing that, I've run into problems with typings across three libraries, so I may or may not have any input around the types of problems I ran into. I should be able to either comment (with very minor stuff, specifically around Typescript's inference issues) or merge by the end of the day.

[threehams]

Note that I do want to bring up the possibility of merging these into DefinitelyTyped instead of this repo.

Three months ago, there was a huge advantage to having these available from the package itself - getting typings required a third-party tool (typings or tsd) and maintaining a separate list of dependencies. TS 2.0 fixed that with @types, which was pretty fantastic.

Now, we're having the opposite problem, where we're needing to make a major version update within this library because of breaking changes in type definitions, even though the underlying library hasn't changed. In addition, these typings require everyone to use Typescript 2.1, and I don't know what adoption rates are like. If there's an addition to the JS code in this library, it'll require anyone using TS to update to 2.1 to use it at all.

Now that Microsoft is involved, DefinitelyTyped has many more resources available, and the discussion about versioning is happening again, although it's related to library version rather than TS version. It seems that 2.1 features will be allowed on DefinitelyTyped one month after the release of the new version.
microsoft/types-publisher#3

Lot to think about here. Lot of uncertainty in the future. I'd like to know what all of you think.

[Jessidhia]

DefinitelyTyped does still recommend bundling over publishing to @types, though.

I wonder if that policy should be revisited with the new publishing system...

[threehams]

I'll take a look at why Angular 2 publishes to DefinitelyTyped, even though the entire project is written with Typescript. That may have more to do with public vs. private typings, though - generating a public d.ts file is on the Typescript roadmap as a planned feature.

[aikoven]

@threehams I agree that it's a pain, although I think that using DT again is a step back. Its problems are well-known and they are the reason why Typings was created. IMO it has one advantage over Typings: you don't have to choose between different typings for same package. With Typings it's common to end up with conflicts arising because of that.

Btw, starting with TS 2.0 it's easy to use NPM for typings, so Typings isn't needed anymore. E.g. if we publish typed-reselect to NPM, we can just add it to dependencies and then point TS to it:

// tsconfig.json
{
  "compilerOptions": {
    "baseUrl": ".",
    "paths": {
      "reselect": ["node_modules/typed-reselect"]
    }
  }
}

Still, there's the same problem as with Typings: if there isn't one official typings package, it may be difficult to choose between them.

Also there's an overhead for referencing each typings package in tsconfig.json. Maybe someday TS will be smart enough to find typings in node_modules automatically.

To me the main reason to bundle typings is that it makes them official, which lets the community to focus around the single variant and avoid partitioning.

However, the versioning problem is much bigger than I thought before. It involves both source package version, which must be bumped for each breaking change in typings, as well as TS version.

The best solution I see now, is keeping typings in the separate repo and package, but close to the source repo (e.g. in reactjs org), and pointing to them in readme.

[threehams]

Publishing types in a separate package sounds like a major divergence from other projects. Is there any precedent for that? Wouldn't we also still need to do major version releases (to avoid breaking semver) when the typings change?

This is an unsolved problem. However, since reselect's code is so stable, I would be in favor of keeping the definitions within this repo for now, and follow / contribute to the larger discussions happening. There is a lot of work being done on the DT types-2.0 branch (related to the types-publisher creating @types modules), and the long-term plan seems to be close to what you're describing here - with the one source of truth being individual modules within the npm @types organization.

(edit: @types on github seems unrelated to https://www.npmjs.com/~types)

I've spent a good part of the day tracking down and reading discussions, but there's still more out there. Some highlights:

Someone equally as confused as me, gradually figuring it out:
typings/typings#746

Discussion about mass changes to definitions within DT as part of Types 2.0:
typings/typings#738

Pull request intended to bring everything in the typings project together into @types:
microsoft/types-publisher#4

[aikoven]

@threehams Thanks for your research.

However, since reselect's code is so stable, I would be in favor of keeping the definitions within this repo for now

We'd still have to bump major when we update typings for new TS versions. There are some features in roadmap that allow more accurate typings for reselect, e.g. Variadic types.

If we publish typings as a separate package, we can have an independent versioning schema that would satisfy semver for both source package and typings. Although we'd have to somehow provide a mapping from source version to typings version(s).

If I understand correctly, microsoft/types-publisher#4 proposes a way to keep typings in a separate repo (not DT), but still be able to install it via @types/. IMO it's the best possible solution to this.

[threehams]

I think I've played with these typings as much as I can, and I'm good to merge these. Thanks aikoven for making these and frankwallis for reviewing!

As for where to put these typings over time? Let's keep watching those couple discussions on aliasing and versioning (I'm subscribed to them all). For now, we're past the 30 day grace period for new TS versions which is the current standard, so anyone on < 2.1 can stay with reselect 2. We should mention this in the release notes, though.

I'm not too concerned about variadic types for now. It's been on the roadmap for a while, there's no clear consensus yet, and the current scope doesn't cover reselect's API (rest arguments before callback). Plus, if it were covered, it should be backwards compatible, since it will only expand the number of params, not restrict them.

[threehams]

@aikoven @frankwallis Any further concerns before merging, or very strong opinions on splitting these to another repo right now?

[aikoven]

Plus, if it were covered, it should be backwards compatible

I'm not 100% sure about this, since it would change generic arguments, which would break code that used old signatures with arguments specified. Still, there's a good chance that variadic kinds wouldn't work for our case at all.

Anyway, I think this would only happen in distant future, and maybe by that time there's more clear way for publishing types.

[chrisdevereux]

Since discussion of this PR seems to have paused, just thought I'd give it a bump and a +1. Would be really great to have a typesafe selector param.

My 2¢: Isn't it always the case that a breaking change may not affect a library user
if they don't use its full functionality?

[frankwallis]

For the record I think that these typings would be better off outside of the repository so they can be independently versioned and will be like the majority of other packages. IMHO the only packages which should include their own typings are those written in typescript as there is much less likelihood of having to update the typings file independently from the code.

Regarding adding another generic parameter, this will be possible to do without breaking existing code when you can specify a default value for generic parameters. This is expected to drop in the next few months in typescript 2.3.0

[threehams]

This pull request has been held up by the "here or outside" question for a while, so I'm making the call to merge it into this repo for v3.0. The discussion (within TS + Microsoft) around better handling of typings in separate modules hasn't gone much further since we last discussed this, so I'd rather not wait any longer for that to be resolved.

I'm watching the relevant issues in Typescript and types-publisher. We can revisit it as we narrow / change typings due to new features in TypeScript.