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

[v8] Port React-Redux to TypeScript #1737

Closed
markerikson opened this issue Jun 22, 2021 · 38 comments
Closed

[v8] Port React-Redux to TypeScript #1737

markerikson opened this issue Jun 22, 2021 · 38 comments

Comments

@markerikson
Copy link
Contributor

React-Redux is currently written in plain JS, and our types have been maintained by the community over in DefinitelyTyped at https://github.com/DefinitelyTyped/DefinitelyTyped/tree/master/types/react-redux/ .

Whenever we get around to writing a React-Redux version 8, we intend to port the library to be written in TypeScript directly.

While we don't yet have plans to work on a new version, it's likely that might happen in the next few months because of React 18 and the Concurrent Rendering changes in that release.

It would be helpful if we could start the work of porting React-Redux to TS now, even if it's just a draft on a branch, so that we have that complex work done ahead of time and can then look at any compat changes for React 18 separately without trying to do both at once.

We maintainers probably don't have the time to tackle this ourselves right now, but I'd love to see the community work on that port so we get it ready.

@chuckdries
Copy link
Contributor

I'm down to help! If no one else has done so by the time I get off work today, I'll spin up a branch and at least configure typescript so we can start working.

@markerikson
Copy link
Contributor Author

Tell you what. In the interest of keeping collaboration centralized, I'm going to go ahead and create an integration branch here in the main repo. That way we can have PRs and code progress in one place.

@markerikson
Copy link
Contributor Author

markerikson commented Jun 22, 2021

Okay, I just created a new https://github.com/reduxjs/react-redux/tree/typescript-port branch . Let's use that as the target for all PRs for this effort. I'll create a draft integration PR for visibility once we have a first couple commits on that branch.

A couple points to consider:

  • The goal for now is to exactly match current React-Redux v7.2.x behavior in both logic and TS. In other words, we're not trying to add any new features or change any TS types, for now. I want an exact match of current functionality as a baseline. Then we can use that as a starting point for any future logic or types changes. That does mean porting over all the ridiculously complicated helper types and overloads that are currently in the React-Redux typedefs.
  • We might want to consider pulling over the ESBuild-powered build toolchain that we recently set up for Redux Toolkit. It's slightly less critical here because we aren't going to have multiple entry points the way we now have in RTK, but it's something that we know works and is probably less complicated than using TSDX or similar. See the scripts folder in the RTK repo for the current source of the build toolchain.

@vinay-deshmukh
Copy link

@markerikson

Is there any plan / structure on how contributors should start converting .js to .ts?
For example, first the hooks folder, then the connect folder.

So that, we don't get two people working on the same set of files at the same time.

Perhaps something like "sub-issues" to this issue to keep track of which files are being worked on.

@markerikson
Copy link
Contributor Author

What you see here is all the discussion so far :)

I can help provide some direction and coordination a bit, but I don't have time to be seriously active with this effort for now. So, this needs to be a collaborative thing. That includes filing issues or anything else that needs to be done to help track the work.

@markerikson
Copy link
Contributor Author

Okay, we've got some basic TS build integration in place, and the first couple files (context and Provider) ported over.

I suspect some of the pieces of connect could be done one file at a time, particularly the selector/map* related files.

@OsidAbu-alrub
Copy link

Hello, I'd love to help

@tony-go
Copy link
Contributor

tony-go commented Jun 29, 2021

Hi Guys 👋

Very nice initiative 🙌

I'd like to start something on connectAdvanced.js and deps asuseIsomorphicLayoutEffect (Subscription seems to have been migrated already ).

Let me know If it's good.

@markerikson If you think I could start on something "better". Let me know ^^

@markerikson
Copy link
Contributor Author

markerikson commented Jun 29, 2021

I just merged #1743 , which converted several of the smaller files to TS.

If we look at the https://github.com/reduxjs/react-redux/tree/typescript-port branch , here's what's left file-wise atm:

  • /src:
    • the entry point files
  • /src/components
    • connectAdvanced.js
  • /src/connect
    • all files (mapStateToProps.js, etc)
  • /src/hooks:
    • useSelector.js
  • /src/utils
    • the batchedUpdates files

There's also all the test files that will need to be converted over, and I don't know if we have the test setup itself configured to build / run TS files yet. We also will probably need to copy-paste over additional types and typedefs from the current @types/react-redux package.

I think we ought to save the three biggest pieces of the actual source for last: useSelector.js, connect.js, and connectAdvanced.js.

So, I'd suggest folks should claim a couple of the other files in /connect and work on those first.

@tony-go
Copy link
Contributor

tony-go commented Jun 29, 2021

Well @markerikson ^^

I'll start something on mapStateToProps.

Ok for y'all ?

@markerikson
Copy link
Contributor Author

Yep, go for it.

@myNameIsDu
Copy link
Contributor

myNameIsDu commented Jun 30, 2021

@markerikson hi
Can I change useSelector?Or something else? I fear that someone else is altering these files

@markerikson
Copy link
Contributor Author

@myNameIsDu : no one's claimed it yet, but I think it might be good if I try doing that one myself. Should have some time to try it this weekend.

@myNameIsDu
Copy link
Contributor

@myNameIsDu : no one's claimed it yet, but I think it might be good if I try doing that one myself. Should have some time to try it this weekend.

Well, if there's anything I can do, please let me know. I'd be happy to take part in it

@tony-go
Copy link
Contributor

tony-go commented Jul 2, 2021

Hi there 👋

I can follow up with mapDispatchToProps WDYT ?

@markerikson
Copy link
Contributor Author

@tony-go yep, go right ahead!

@reduxjs reduxjs deleted a comment from CoolRaja001 Jul 3, 2021
@markerikson
Copy link
Contributor Author

As a heads-up: we recently converted the RTK repo to Yarn v2, and I'm working to convert the React-Redux repo to Yarn v2 based on this typescript-port branch as well.

If anyone else is planning to do PRs for this, be aware that this change is coming (hopefully shortly).

@markerikson
Copy link
Contributor Author

Yarn v2 switch done via #1751 . Please update your clones/forks as needed.

@markerikson
Copy link
Contributor Author

markerikson commented Jul 6, 2021

FYI, I'm starting to tackle connectAdvanced.js, which also strongly overlaps with connect.js and selectorFactory.js, so I'm doing all three at once. @tony-go is doing mapDispatchToProps.js.

I think this is the last of the meaningful files that need to be converted. However:

  • The entry point files need to be converted, and need to export all relevant types
  • We still need to get our test files converted to TS
  • We don't have the build setup outputting TS typedefs

So, still plenty left to do here.

@markerikson
Copy link
Contributor Author

I just did a bunch of reworking of the connect types to better match what's in the existing @types/react-redux typedefs. I also added a typetests file based on what was in DT.

@StevenLangbroek , @myNameIsDu : I think there's a good chance this could fix whatever type issues you were seeing while converting tests. Try merging/rebasing and let me know what happens.

@myNameIsDu
Copy link
Contributor

@StevenLangbroek How should I cooperate with you?:smiley:

@myNameIsDu
Copy link
Contributor

OK, I will start by fixing the file under test=>hooks. If you want to get involved, please be careful not to clash

@myNameIsDu
Copy link
Contributor

I will fixing the file under test=>components. If you want to get involved, please be careful not to clash 😃

@StevenLangbroek
Copy link

Hey @myNameIsDu! So regarding how to collaborate, I think (given the fact that Github doesn't support Stacked PRs) we have 2 options, either is fine for me:

  1. You work in your own fork, submit PRs against my fork/branch (since it has the basic setup there)
  2. I give you maintainer access to my fork, we can work in the same branch if we go folder by folder / task by task

I think 2 is easiest, if you'd rather work some other way I'd love to hear it.

@myNameIsDu
Copy link
Contributor

myNameIsDu commented Jul 13, 2021

Hey @myNameIsDu! So regarding how to collaborate, I think (given the fact that Github doesn't support Stacked PRs) we have 2 options, either is fine for me:

  1. You work in your own fork, submit PRs against my fork/branch (since it has the basic setup there)
  2. I give you maintainer access to my fork, we can work in the same branch if we go folder by folder / task by task

I think 2 is easiest, if you'd rather work some other way I'd love to hear it.

I also think the second of these two methods easier

However, I don't think it's that troublesome, We just need to separate our files so that there is no conflict 😏

@myNameIsDu
Copy link
Contributor

@markerikson If want to delete ConnectAdvanced, will the test case be rewritten? If so, I will not continue to convert it

@markerikson
Copy link
Contributor Author

Yeah, you can see that my PR in #1771 deletes that test case file. So, ignore that - it'll go away when #1771 gets merged.

@myNameIsDu
Copy link
Contributor

@markerikson What do you need me to do next? Because you are changing to connect, I did not change connect.test

@markerikson
Copy link
Contributor Author

markerikson commented Jul 26, 2021

@myNameIsDu The connect tests file should stay basically the same - that's intentional. I think I did delete one or two tests in that PR, but that's it.

It's just that connectAdvanced is going away.

I'd say go ahead and convert connect.spec.ts to TS right now, and I'll take care of doing the merge with that other PR myself later.

@myNameIsDu
Copy link
Contributor

@myNameIsDu The connect tests file should stay basically the same - that's intentional. I think I did delete one or two tests in that PR, but that's it.

It's just that connectAdvanced is going away.

I'd say go ahead and convert connect.spec.ts to TS right now, and I'll take care of doing the merge with that other PR myself later.

Ok, I'm going to convert connect.spec.js, which is exciting 😏

@markerikson
Copy link
Contributor Author

I've just merged the TS integration PR at #1739 into master!

I created a 7.x branch off of master right before I merged, and we'll use that for any future maintenance fixes on v7.

We still have some test files left to convert, but we can continue that work against master. I'll leave the issue open just until we get the rest of the files converted, then close it.

Thank you SO much to everyone who's contributed to this effort!

@markerikson
Copy link
Contributor Author

The actual TS conversion work is done, including all source and test files, and we're now up to https://github.com/reduxjs/react-redux/releases/tag/v8.0.0-beta.0 . Closing this - thank you everyone who helped!

@markerikson markerikson unpinned this issue Apr 13, 2022
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

7 participants