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

TypeScript update for 4.x #486

Merged
merged 21 commits into from Oct 17, 2021
Merged

Conversation

eXamadeus
Copy link
Contributor

@eXamadeus eXamadeus commented Feb 8, 2021

Update TypeScript without causing breaking changes

Upgrade TypeScript to 3.9 and fix any broken issues that can be fixed without a major version update. Any PRs that can be included to fix the typings without a major version update will be pulled into this PR.

TODO:

Fixes

Add tests for

Includes PRs:

#465 (@micahbales): Use Callable Type Signature for Memoize Callback in createSelectorCreator

Addresses issue microsoft/TypeScript#20007 by adding a callable type signature for the memoize callback in createSelectorCreator. This avoids a type error when using Lodash Memoize as the first argument in createSelectorCreator.

Related

@coveralls
Copy link

coveralls commented Feb 8, 2021

Pull Request Test Coverage Report for Build 1350079775

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 97.727%

Totals Coverage Status
Change from base Build 1246092029: 0.0%
Covered Lines: 21
Relevant Lines: 21

💛 - Coveralls

@coveralls
Copy link

Pull Request Test Coverage Report for Build 548532703

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 97.727%

Totals Coverage Status
Change from base Build 548417046: 0.0%
Covered Lines: 21
Relevant Lines: 21

💛 - Coveralls

@codecov
Copy link

codecov bot commented Feb 8, 2021

Codecov Report

Merging #486 (4556d0f) into master (f4a2173) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #486   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            2         2           
  Lines           21        21           
=========================================
  Hits            21        21           

@markerikson
Copy link
Contributor

Side note: why are we getting three different code coverage report comments? :)

@eXamadeus
Copy link
Contributor Author

eXamadeus commented Feb 9, 2021

Side note: why are we getting three different code coverage report comments? :)

Haha, good question! There is at least two because Codecov and Coveralls are both running in the workflow (lol, we never did decide on just one).

I guess that Coveralls is having trouble distinguishing the main repo from the fork 🤷? Regardless, seems like a good reason to drop Coveralls and use Codecov instead. WDYT?

I can make a separate PR to pick one of the two. It's just a few lines in the workflow file.

@eXamadeus
Copy link
Contributor Author

Side note: HOLY SMOKES BATMAN, LOOK AT THOSE LINES REMOVED FROM THE PACKAGE-LOCK.JSON!
image

@markerikson
Copy link
Contributor

I'm for whatever provides the most useful info and the least amount of annoyance :)

As far as I know Reselect is the only one of the Redux org repos that has code coverage reports turned on in PRs, and given the very small size of the library, I'm not sure I find that all that useful. (Granted, I'm also not the person who spent most of their time maintaining this lib in the first place :) )

@markerikson
Copy link
Contributor

Code-wise, this seems reasonable so far. Looks like you've mostly swapped the type tests for tsc itself, and tweaked some of the arg definitions in how they extend functions?

@eXamadeus
Copy link
Contributor Author

Code-wise, this seems reasonable so far. Looks like you've mostly swapped the type tests for tsc itself, and tweaked some of the arg definitions in how they extend functions?

Yup! And set the baseline TS version to 3.9 (mainly to make the testing easier)


I'm for whatever provides the most useful info and the least amount of annoyance :)

As far as I know Reselect is the only one of the Redux org repos that has code coverage reports turned on in PRs, and given the very small size of the library, I'm not sure I find that all that useful. (Granted, I'm also not the person who spent most of their time maintaining this lib in the first place :) )

I'll make a PR to remove Coveralls, and I guess if anyone gets angry I'll just blame it on you 😆

On a more serious note: we could leave the "removal" PR up for a while and see if someone really feels adamant about it. Give it a week or so and if no one says anything we could merge it right? It's super easy to add back in whenever.

I'm also not convinced Coveralls was even running anytime recently:

image

@eXamadeus
Copy link
Contributor Author

Side note: made #488 to track discussion on the coverage reporters.

@markerikson
Copy link
Contributor

Hmm. Just bumped the TS test matrix to cover TS 4.2+ as well, and those immediately started failing:

Error: typescript_test/test.ts(285,5): error TS2769: No overload matches this call.
  The last overload gave the following error.
    Type '(state: {    foo: string;}) => string' is not assignable to type 'ParametricSelector<unknown, unknown, unknown>'.
      Types of parameters 'state' and 'state' are incompatible.
        Type 'unknown' is not assignable to type '{ foo: string; }'.
          Type 'number' is not assignable to type 'ParametricSelector<unknown, unknown, unknown>'.
Error: typescript_test/test.ts(302,7): error TS7006: Parameter 'foo1' implicitly has an 'any' type.

@markerikson
Copy link
Contributor

I've updated the typedefs to use the types from eXamadeus#3 , which are way shorter and hopefully better as well.

Since that requires TS 4.2+, I've dropped <4.2 from the test matrix. To keep those semi-supported, I've moved the existing typedefs into a nested folder and added a typesVersions field to point to them for <4.2.

I tried doing a local yalc publish and installing that build of Reselect into a TS project I already had on my machine. I can confirm that if I switch that project and VS Code between TS 3.9 and 4.4, that I see the two different definitions of createSelector being used.

- Extracted standalone `EqualityFn` type
- Removed incorrect `index` param from equality args
- Added specialized overload of `createSelectorCreator` specifically
  for usage with `defaultMemoize`, for better inference

Ref: reduxjs#384 (comment)
@markerikson
Copy link
Contributor

Just published this current PR as https://github.com/reduxjs/reselect/releases/tag/v4.1.0-alpha.0 for folks to try out.

@markerikson markerikson added this to the 4.1 milestone Oct 17, 2021
@markerikson markerikson marked this pull request as ready for review October 17, 2021 18:13
@markerikson
Copy link
Contributor

Good enough. Merging this, and following up with a PR that actually converts the source to TS.

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