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

refactor(typescript): upgrade to TS 4.1, simplify and generalize types #480

Closed
wants to merge 0 commits into from

Conversation

eXamadeus
Copy link
Contributor

@eXamadeus eXamadeus commented Jan 30, 2021

EDIT:

See #480 (comment) for "status" tracker of the changes suggested by everyone

Also, when I updated my master branch it purged this PR of the code. The latest commit for reference was 66fcf90. You can see the original changes @ eXamadeus#3 or #487.

What

Upgrades to the latest version of TypeScript: 4.1. I think this will be breaking changes. I mean it isn't a change to the code, but the TS updates might require some slight modifications to user code if they were using the ParametricSelector or Selector types directly.

Changes

  • types no longer suffer "death from a thousand overloads"
  • createSelector can accept any number of selectors
    • the limit is only in the depth of recursion TS allows, which is 50 levels as of now
    • why this arbitrary number? I dunno ask the TS guys
  • removed ParametricSelector as the base Selector type is now smart enough to "encompass" this role

CI/Build Chores

  • updated Travis targets to still maintained versions of node
  • fixed broken build scripts
  • updated TypeScript testing strategy to use https://github.com/microsoft/dtslint
    • created /types folder and moved tests there
    • added symlink to test folder (easiest way to setup new test)

@codecov
Copy link

codecov bot commented Jan 30, 2021

Codecov Report

Merging #480 (66fcf90) into master (0c25739) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #480   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  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 e93fe66...66fcf90. Read the comment docs.

.travis.yml Outdated
Comment on lines 3 to 6
- "10"
- "12"
- "14"
- "15"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The node versions here were ancient. I updated them because the new test tooling just simply won't run in old unsupported versions.

package.json Outdated
@@ -17,7 +17,7 @@
},
"scripts": {
"compile:commonjs": "better-npm-run compile:commonjs",
"compile:umdmin": "uglifyjs dist/reselect.js -mt -o dist/reselect.min.js",
"compile:umdmin": "uglifyjs dist/reselect.js -m -o dist/reselect.min.js",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The -t flag is not valid, so I removed it.

package.json Outdated
@@ -42,7 +42,7 @@
}
},
"test:typescript": {
"command": "typings-tester --dir typescript_test"
"command": "dtslint --expectOnly types"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a superior testing suite to typings-tester. This is also the officially supported type testing module by the TypeScript team.

types/index.d.ts Outdated
@@ -0,0 +1 @@
../src/index.d.ts
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a symlink...unfortunately dtslint is pretty annoying with it's lack of configurability.

In order to get the tests to run, you need the type definitions and the "tests" to be located in the same directory. Since I didn't want to touch the main /src directory, since it's part of the bundling path, I decided to just make a simple symlink here to let the tests run against the main type definitions.

It all works, but I would understand if there was pushback here. Although I do think that git-based symlinks work on every OS nowadays.

types/test.ts Outdated
Comment on lines 88 to 89
declare function connect<S, R, P>(selector: Selector<S, R, P[]>):
(component: Component<R & S & ([P] extends [never] ? {} : P)>) => Component<P>;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Selector types were modified, so we needed to update this helper

Copy link
Contributor

@nickserv nickserv Feb 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed that R and P are swapped here. Would that impact back compatibility (positively or negatively)?

Copy link
Contributor Author

@eXamadeus eXamadeus Feb 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not here in the test, but it might cause issues in client usage of Selector. This is something that I should revert though.

I changed the order without realizing because I rewrote the types from scratch. I'm definitely going to fix the order here, since it's a pointless change that would just inconvenience people.

types/test.ts Outdated

function testConnect() {
connect(
createSelector(
(state: {foo: string}) => state.foo,
foo => ({foo}),
foo => ({changeUp: foo}),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This made the test more obvious that it was "changing" the return type values to something different.

types/test.ts Outdated
});

const connected = connect(
createSelector(
(state: {foo: string}) => state.foo,
(state: never, props: {bar: number}) => props.bar,
(foo, bar) => ({foo, baz: bar}),
(state: {foo: string}, props: {bar: number}) => props.bar,
Copy link
Contributor Author

@eXamadeus eXamadeus Jan 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

never is now a proper bottom type and will not assign across a heterogenous collection of selectors.

The state object should always be the same regardless of the selector, so TypeScript is doing us a favor here by forcing users to use a heterogenous State type or any.

types/test.ts Outdated
Comment on lines 108 to 113
(foo, bar) => ({foo, baz: !!bar}),
)
)(props => {
const foo: string = props.foo;
const bar: number = props.bar;
const baz: number = props.baz;
// typings:expect-error
const baz: boolean = props.baz;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made the conversion of types clearer here for future developers.

types/test.ts Outdated
Comment on lines 213 to 217
(state: any) => state.foo,
(state: any) => state.foo,
(state: any) => state.foo,
(state: any) => state.foo,
(state: any) => state.foo,
Copy link
Contributor Author

@eXamadeus eXamadeus Jan 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-explicit any is generally disallowed, we will enforce this via our type definitions. This can be disabled with a tsconfig.json change on a user-by-user basis.

types/test.ts Outdated
@@ -225,20 +228,20 @@ function testArrayArgument() {
const selector = createSelector([
(state: {foo: string}) => state.foo,
(state: {foo: string}) => state.foo,
(state: never, props: {bar: number}) => props.bar,
(state, props: {bar: number}) => props.bar,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implicit any values can be used in place of never for heterogenous State selectors.

types/test.ts Outdated
@@ -387,7 +390,7 @@ function testDefaultMemoize() {
if (index === 0)
return a === b;

return a.toString() === b.toString();
return `${a}` === `${b}`;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TypeScript bug that was fixed...

@markerikson
Copy link
Contributor

Wow, thanks for putting this together!

The biggest issue atm is that @ellbee , the primary maintainer for Reselect, has been mostly inactive for a while. Tim Dorr and I have commit rights to the repo as owners of the reduxjs GH org, but we don't have publish rights on NPM. So, even if we merge this, we don't currently have a way to publish a new build.

I'm going to try reaching out to @ellbee and seeing if we can get added as publishers.

Copy link
Member

@phryneas phryneas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added some comments. I don't think this needs a new type testing framework honestly, if anything the testing framework could be removed in the long run if we were to target only 3.9 and above.
Right now, redux toolkit, which depends on reselect goes down to 3.5, so that should probably stay supported for now though.

As for backwards compatibility: I'd suggest keeping existing types with their names as they are and deprecating them.
In addition to that, I'd keep the old type definition in a second file and use typesVersions to have TS choose the new one only for TS >=4.1

types/test.ts Outdated
@@ -20,10 +20,10 @@ function testSelector() {

const foo: string = selector({foo: 'bar'});

// typings:expect-error
// $ExpectError
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this only needs to be tested on more modern versions of TS, this could also just be // @ts-expect-error - that way no test framework would be required at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that makes sense. I would prefer to use the // @ts-expect-error directive. If we need to support 3.5 for redux-toolkit, we probably need to keep the // $ExpectError directive though.

The main reason that I upgraded from typings-tester was for compatibility reasons with later versions of TypeScript and for somewhat selfish development reasons. The error messages from dtslint were very helpful in making sure I covered all the edge cases correctly in the new typings. Seeing a descriptive error in the output was a lot easier than having to just see "error @ line ###".

I don't mind downgrading now, if you think it's important. But if we do want to support things < 3.9 we'll need some sort of tooling to help with that.

Copy link
Contributor Author

@eXamadeus eXamadeus Jan 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One other note, I think the main reason that something like dtslint is useful is that it tests for type compatibility across a range of TS versions where the // @ts-expect-error directive only works for the installed version, right?

I think that would be a compelling reason to actually keep something like dtslint around, right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Over on redux toolkit, we use a test matrix that just runs tsc on the files with each tested TypeScript version.
https://github.com/reduxjs/redux-toolkit/blob/next/.github/workflows/tests.yml

The nice thing about // @ts-expect-error is the usage in the editor. If there is an error where one is expected, everything is fine. If the error is absent, you get squiggly lines. So you can just look into the editor to spot where your test breaks, you don't even need to run the tests.

Especially for writing the tests, that's a big thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, that's a good idea! I think we can do something similar with TravisCI here right?

If I have the go-ahead to do that, I don't think there are any differences between 3.5 and 3.9 (in regards to this library) so we could just say that this only supports 3.5 and above while only testing 3.9 + using the // @ts-expect-error comments.

Basically, we can validate that 3.5, 3.6, 3.7, and 3.8 work with these typings (we know they do because of all the tests we've been running in this PR) but only setup automation to check 3.9 and above. I guess we could say this is "verified again 3.5-3.8, and actively tested against 3.9+".

I'm super new to OSS library maintenance, so I'm down for whatever you guys suggest. You're the experts haha.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or I can still do the matrix and use the // $ExpectError directive for 3.5+ and // @ts-expect-error for the new typings. Either way seems good to me. But the removal of the testing library seems like a more "future-proof" solution.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A build matrix would be possible with Travis, but since Travis CI is pretty much kicking out open source projects by restricting build times & becoming more & more unreliable, it would probably be a good idea to switch over to GH actions anyways.

As for testing older TS versions than 3.9... I'd be fine not to, since I don't expect any bigger changes coming up in the near future and the current state has already been tested for a long time. @markerikson what do you think about this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If @markerikson is good with it, I'll do the switch over to GH actions here too. It's the way of the future anyway, right? 😆

I could also totally break that up into another PR. Just let me know what you would prefer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As for testing older TS versions than 3.9... I'd be fine not to, since I don't expect any bigger changes coming up in the near future and the current state has already been tested for a long time.

That would also make the testing a lot easier because you wouldn't need to do the swapping of the test comments or maintain separate type files. I was looking at the way you guys did it in the toolkit and that last bit of the job looked a little gnarly with all the sed stuff.

src/index.d.ts Outdated

export type OutputParametricSelector<S, P, R, C> = ParametricSelector<S, P, R> & {
resultFunc: C;
export type Selector<S = any, R = unknown, P extends never | (readonly any[]) = any[]> = [P] extends [never] ? (state: S) => R : (state: S, ...params: P) => R;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My suggestion would be to keep Selector, ParametricSelector and OutputParametricSelector in as it they are, mark them as @deprecated and use a new name for this type to avoid a breaking change.

Copy link
Contributor Author

@eXamadeus eXamadeus Feb 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, so I think if I address the suggestion here these will be backward compatible right?

This is because the new Selector is just a combined version of the old Selector and Parametric Selector. The only thing that could be considered breaking, in my opinion, is the "extra args" bit of the ParametricSelectors. Those were always just passed through as ...any[] where they are not fully typed. In every other instance, I think these types will behave exactly the same, no?

See this playground link for an example of what I'm talking about. I'm on the fence if this would be considered a bug fix or a breaking change. In reality, it's kind of a bug fix right? If they do provide extra args in any of the selectors, they will behave the same and be typed more correctly than before.

src/index.d.ts Outdated Show resolved Hide resolved
src/index.d.ts Outdated
: []
: Rest extends ((...args: any) => infer S)[] ? S[] : [];

export function createSelector<Selectors extends SelectorArray, Result>(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The breaking change in the arguments to createSelector would be kinda expected for me - I'd be okay with that.

src/index.d.ts Outdated Show resolved Hide resolved
@eXamadeus
Copy link
Contributor Author

As for backwards compatibility: I'd suggest keeping existing types with their names as they are and deprecating them.
In addition to that, I'd keep the old type definition in a second file and use typesVersions to have TS choose the new one only for TS >=4.1

I'm embarrassed to admit that I had no idea that could be done. I've never written package typings before so that thought never crossed my mind. I will absolutely do this though!

@eXamadeus
Copy link
Contributor Author

eXamadeus commented Feb 6, 2021

Just as an update, I plan to get around to updating this with the suggestions this weekend. I'm sorry I didn't reply/update this sooner, it's been a busy week at work.

I am very thankful for the reviews though. I also plan to resolve those and make corrections in my code as soon as I am able.

@markerikson
Copy link
Contributor

Hey, wanted to follow up on this.

I'd like to split the work in this PR into a couple different PRs if at all possible.

First, per #482 , I definitely want to modernize the CI setup, including switching from TravisCI to Github Actions and dropping old versions of Node. However, I think that should be its own PR.

From there, yes, I do want to update any dependencies and actual TS typings.

Not sure if it'd be easier to close this PR and then create two new ones?

@markerikson markerikson mentioned this pull request Feb 7, 2021
@markerikson
Copy link
Contributor

Also: my other concern atm is that it looks like some of the changes in here are probably enough to justify a Reselect 5.0 release. However, if we do a 5.0, it seems like there are also a number of other functionality changes that should be considered as well, such as #401 , and even rewriting the code itself in TS.

Meanwhile, there's open PRs like #465 , which seems small but useful, and could be put out in a new 4.x release.

Given that, can we figure out a way to scope the changes so that we can have a 4.x release soon with some additional bits of cleanup, and then do a larger effort to work towards a 5.x?

@eXamadeus
Copy link
Contributor Author

eXamadeus commented Feb 7, 2021

Given that, can we figure out a way to scope the changes so that we can have a 4.x release soon with some additional bits of cleanup, and then do a larger effort to work towards a 5.x?

Sure. I really like that idea. We can do a 4.x PR that is only non-breaking upgrades. A non-exhaustive list:

Then I can start a 5.0 release w/:

I would love to include #401 (well some variant of it) in the 5.0 release PR, but I'm not sure there is a consensus on the names for the new API. I have read through all the posts and related issues (that took a while BTW 😆) and think it's a great idea. I'm of the opinion it shouldn't be called createKeySelector (which I think @josepot agrees with).

My personal opinion is that it should be called createDynamicSelector AND that it should still be able to accept the state. The "dynamic selector" moniker would just let people know that it won't be considered for caching purposes. This is a slightly less "leaky" way to name it. What do you think @josepot? createDynamicSelector should still behave the same as createKeySelector and will still allow for things like shared selectors. Heck, it might even enable things with can't think of right now. I am not sure of any downsides to still allowing the state. I think most people wouldn't use it, but I don't see why it would be a problem either.

I have no problem making both of these PRs. Or I can contribute to them if you'd rather control that process. Just let me know.

@markerikson
Copy link
Contributor

@eXamadeus yep, sounds good. Let's do the build updates first. In fact, let's split this up into separate PRs for :

  1. CI changes
  2. Dependency updates
  3. Any minor fixes we feel are appropriate for a 4.x release.

I think it's worth having a larger planning discussion around what 5.x should include. There's a bunch of open PRs that I could see conflicting. I think it's also important to analyze what weaknesses Reselect currently has and what use cases it doesn't convert well atm, and look at other alternatives in the ecosystem for comparison.

Related to all this, I've been looking at https://github.com/dai-shi/proxy-memoize as a possible better alternative to Reselect that we could start recommending. Discussion thread for that topic is at reduxjs/react-redux#1653.

However, I'm also interested in improving Reselect itself too. If we're going to do so, I want to have a plan laid out.

I'll try to create a discussion thread tonight or tomorrow.

@eXamadeus
Copy link
Contributor Author

@markerikson Sounds good, I'll have them up tonight or tomorrow (at least the CI one for sure).

Question about the dependency updates: which updates does reselect want to entertain? If we decided to just assume TS 3.9+ support we can drop the testing libraries completely. If that's the route we want to go, that's probably the easiest one. Also, do you want to do a generic upgrade of other dependencies while were at it? Or just remove the testing libraries and update to say 3.9+ is supported?

Either way, I can create the CI PR and a "minor fixes" PR that follows that w/ #465 in it.

@markerikson
Copy link
Contributor

Yeah, I still want to do this in phases. There were several open "update deps" PRs I closed today. Like, this is apparently still on Babel 6 (!).

I think we can certainly ignore TS < 3.5, and it's probably not unreasonable to go 3.9+. Of course, as we just saw, 3.1+ appears to be an issue :)

@eXamadeus
Copy link
Contributor Author

I think we can certainly ignore TS < 3.5, and it's probably not unreasonable to go 3.9+. Of course, as we just saw, 3.1+ appears to be an issue :)

Yeah, it's only two of the tests that are broken (sort of). The actual library in production is already affected by this, so it's not like removing the tests would cause a breaking change. It's basically just acknowledging that the types work as expected for 2.9 and 3.0, but don't behave correctly for 3.1+. Maybe we should make a bug fix for that in 4.x?

The new typings for TS 4.1+ do actually work for this tested scenario, so the test is brought back for TS 4.1+. If you're alright with it, we can just make this PR only do the type upgrade to TS 3.9+ with no breaking changes (so we can keep this as reselect v4)? I am still planning to make that other PR that will be for the 5.0 release w/ TS 4.1+ typings.

Of course, both of these would require the work done in #483 to use the new CI changes. Let me know if you want me to repurpose this PR or create another one for the "reselect v4 type updates (TS 3.9+)".

@markerikson
Copy link
Contributor

Hah, I'm kinda getting lost on what's where now :)

Yeah, let's try things in this order:

  • Turn off the couple failing tests in the GH Actions PR, as the failing tests are not due to the GH Actions switch, and get that merged in
  • Update any other dependencies and build tooling
  • Figure out if there's a reasonable fix to the types that could be made in a 4.x release
  • Hold off on a larger rewrite of the types, because this will be affected by any noticeable changes to the API that might be involved in a v5

@markerikson
Copy link
Contributor

@eXamadeus: so, it turns out that this bug was already reported by @nickmccurdy two years ago :)

#420

types/test.ts Outdated
Comment on lines 88 to 89
declare function connect<S, R, P>(selector: Selector<S, R, P[]>):
(component: Component<R & S & ([P] extends [never] ? {} : P)>) => Component<P>;
Copy link
Contributor

@nickserv nickserv Feb 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed that R and P are swapped here. Would that impact back compatibility (positively or negatively)?

Comment on lines 1 to 14
{
"compilerOptions": {
"module": "commonjs",
"lib": ["es6"],
"noImplicitAny": true,
"noImplicitThis": true,
"strictNullChecks": true,
"strictFunctionTypes": true,
"noEmit": true,

"baseUrl": ".",
"paths": { "src": ["."] }
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use "strict": true or would it be too noisy?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think that would be fine. This was the recommended tsconfig for the old testing library (typings-tester?) I didn't think about updating it, but using strict is definitely worth it IMO.

I'll make sure that's added to whatever final PR goes in. Thanks for the catch!

@nickserv
Copy link
Contributor

nickserv commented Feb 8, 2021

Whoops sorry, I reviewed this without reading the discussion first. I'm now following the repo so if there are new PRs I should be able to review them. Let me know if I can help with anything else.

@eXamadeus
Copy link
Contributor Author

eXamadeus commented Feb 8, 2021

Hey @nickmccurdy this is still the best one to review the type changes I suppose. But @markerikson brings up a good point that these type changes need to wait until a v5 release.

Here's a list of things I'm planning on doing now:

For the deps PRs, I can do those too. Looks like #461 and #381 look like good candidates there. I would think the babel upgrade (#381) should probably go first, then #461 can go on top of that. Would you like me to pull those together in a deps PR as well? Or is that something you guys want to do?

@markerikson
Copy link
Contributor

@eXamadeus Yep, please go ahead and do a deps PR as well if you have time.

I really appreciate your effort and contributions here!

@eXamadeus
Copy link
Contributor Author

I'm going to make issues to track the "bullet points" I made earlier and I'll edit the comment to link to them.

@markerikson
Copy link
Contributor

FYI, we do already have an "upgrade Babel" PR at #381 , which Andarist just rebased. However, it now conflicts with the removal of the Travis file.

Not sure what the best option is for resolving that atm.

@eXamadeus
Copy link
Contributor Author

eXamadeus commented Feb 8, 2021

FYI, we do already have an "upgrade Babel" PR at #381 , which Andarist just rebased. However, it now conflicts with the removal of the Travis file.

Not sure what the best option is for resolving that atm.

I'll comment on that PR, but he just made the same "changes" we just made to the node versions essentially. It looks like just deleting the .travis.yml file in his PR will resolve the conflict and should just work™.

@markerikson
Copy link
Contributor

Good call! Just did that via the web UI and I see the GH actions kicking in now.

@eXamadeus
Copy link
Contributor Author

eXamadeus commented Feb 8, 2021

Whoops, I should have made a branch on my fork and not used master. When I updated my master branch it purged this PR of the code.

The latest commit here for reference is 66fcf90. You can see the changes, as they originally were, @ eXamadeus#3 or #487.

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.

4 participants