Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Priorities & current state #282

Open
meandmax opened this issue May 17, 2021 · 7 comments
Open

Priorities & current state #282

meandmax opened this issue May 17, 2021 · 7 comments

Comments

@meandmax
Copy link

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
Copy link
Contributor

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
Copy link
Author

Nice, thank you very much!

  • What are your thoughts about extending react transforms? Do you think there are some which are missing?
  • Do you know if and which types between the core libraries of the 2 type systems are missing?
  • We also would like to convert type statements to interface statements if possible, any concerns?
  • Regarding the comment in Convert suppression comments #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.

@kevinbarabash
Copy link
Contributor

kevinbarabash commented May 20, 2021

  • 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 Convert suppression comments #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
Copy link

Pike commented May 27, 2021

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:

  • Tighten tsc config, we started really lax
  • Incorporate more external types
  • Typing fixes that are better documented in typescript than they were in flow, and thus can be done better.
  • Refactor files we've thrown under the bus via @ts-nocheck. Yes, also a shortcut we're taking.

HTH

@kevinbarabash
Copy link
Contributor

@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
Copy link
Contributor

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
Copy link

Pike commented May 29, 2021

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants