search

Khan / flow-to-ts

Convert flow code to typescript
https://flow-to-ts.netlify.com
393 stars 75 forks source link

Priorities & current state #282

Open meandmax opened 3 years ago

meandmax commented 3 years ago

Hey @kevinbarabash,

we are currently evaluating which tool we want to use and invest time and engineers into in order to migrate our codebase from flow to TypeScript. For that reason I would be keen to hear which issues have the highest priorities from your perspective.

For context, our codebase is 3.5M+ lines of source code written in flow and react. The only other tool that is competing with flow-to-ts is https://github.com/Airtable/typescript-migration-codemod which was open sourced just recently.

Thanks for all the work you have already done! Max

kevinbarabash commented 3 years ago

Hi @meandmax. Thanks for reaching out. Here's a list of issues that may or may not be high priority depending on your codebase:

The most complicated issue is preserving formatting. The project previously had a vendored version of a built copy of @babel/generator with various modifications to improve the position of comments in the output. Having to re-apply fixes to built code seemed like it would be harder to maintain than keeping a modified version of the source up to date. This is why the project now consumes babel modules from via a submodule. I haven't gotten around to porting the modifications that I had previously made to @babel/generator to the submodule.

meandmax commented 3 years ago

Nice, thank you very much!

kevinbarabash commented 3 years ago
  • What are your thoughts about extending react transforms? Do you think there are some which are missing?

Extending React transforms to other libraries is captured by #226. If there's react types that are missing that you need please file tickets for them. Probably best to group them together into a single ticket so they can be handled at once.

  • Do you know if and which types between the core libraries of the 2 type systems are missing?

I hadn't really looked at the core libraries outside of React. I doubt there will be DOM or standard library type definitions that appear in Flow but are completely missing in TypeScript. It feels like TypeScript keeps it's core library definitions pretty up to date.

  • We also would like to convert type statements to interface statements if possible, any concerns?

This is something that should be configurable.

  • Regarding the comment in #207, I can only speak for our codebase and I think it would not make sense to convert suppression comments, it would only make sense to strip them and bulk add suppressions in TypeScript at the end of the conversion.

Makes sense to me. I've closed #207 and opened #284 instead.

Pike commented 3 years ago

A few comments from someone just going through this. This is kinda the talking points of a blog post, if I get to it. PRs are https://github.com/mozilla/pontoon/pulls?q=is%3Apr+label%3Atypescript, bug list is https://bugzilla.mozilla.org/showdependencytree.cgi?id=1685565&hide_resolved=0.

One decision we made too late was "convert tests or not". Maybe not at the same time? Ignoring tests makes your problem smaller, and at least in our code base, the tests weren't using flow to allow for shortcuts (like invalid arguments for mocked functions).

What worked was just trying, and we ran that attempt in automation. Well, we still do as of now. You can see our workflow on https://github.com/mozilla/pontoon/blob/5e1d0022370f775690b4284a395d25105c81b9fa/.github/workflows/frontend.yml#L57. That way we had a current list of new issues, and could fix some of them ahead of time. This involved in particular external libraries that come with typescript definitions which have incompatible types on the flow side. We've built quite a few library definitions. I've also added some @ts-ignore ahead of time with suggested fixes, to ease fixing things like window.setTimeout etc.

Re code formatting, I spent a day fixing 20k lines of code. Blank lines look like an annoyance, but for some of our files there were enough to throw off git's copy-move detection, which gets really unwieldy. And then, the comment reordering is actually nasty, as sometimes comments from from the top of the else clause to the end of the if clause and so forth. Make sure to inline utilities (#239 ) to help the diff size, too.

Things we'll like put into follow-ups:

HTH

kevinbarabash commented 3 years ago

@Pike thanks for posting this. I kind of figured that formatting differences would cause issues with large codebases. I'm still planning to do work in this area, but I'm busy with other projects at the moment.

kevinbarabash commented 3 years ago

And then, the comment reordering is actually nasty, as sometimes comments from from the top of the else clause to the end of the if clause and so forth.

@Pike could you file an issue with an example of this?

Pike commented 3 years ago

Maybe I'm remembering this wrong, I've not kept my fixups as a separate commit. The places I could reproduce right now all seem to be covered by #230, left my playground there.