1685565 - Switch from FlowType to Typescript

Status: RESOLVED FIXED

(Webtools Graveyard :: Pontoon, enhancement, P3)

Description

[abowler]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:83.0) Gecko/20100101 Firefox/83.0

Steps to reproduce:

Currently the Translate app uses FlowType for static type checking.

Actual results:

Given that Flow is not as widely used it is lacking in support and community resources.

Expected results:

Typescript is very widely used and has a large community of support around it. Switching to Typescript would open up a very large support ecosystem.

Comment 1

[Axel Hecht]

I came across https://skovhus.github.io/blog/flow-to-typescript-migration/ as a recipe, pointing to https://github.com/Khan/flow-to-ts as part of the procedure.

I'm tempted to take a stab at this.

Comment 2

[Axel Hecht]

Attachment: npx madge output

I didn't get very far here. Not sure if this is just me not knowing how to set up ts, or more.

The error messages I get from tsc complain about type definitions not being found. Which didn't make a lot of sense, so I ended up guessing that it's a bad way of saying "circular dependency problem". That held true even after I tweaked flow-to-tsc locally to keep import type. Not sure if this is the actual problem, though.

Just to add context, I ran npx madge --circular src

Comment 3

[Julen Ruiz Aizpuru]

Typescript is very widely used and has a large community of support around it. Switching to Typescript would open up a very large support ecosystem.

I agree with the sentiment here. I always need to find ways to make things work in my editor (vscode), and then always be left with the feeling type checking may not be working... editor auto-complete largely doesn't work, and attempts to fix that end up in frustration and wasted hours in debugging.

As time permits, I'm happy to help out in this migration and collaborate with whoever wants to take the lead.

Comment 4

[Julen Ruiz Aizpuru]

Hey, I've managed to run flow-to-tsc successfully, and while there's still a lot to do (fix linting configs, fix compilation errors, adjust type constructs, ...), I'd like to work on this. Given that the changes will be invasive, how do we want to proceed with this? Target a separate branch, stabilize / review individual changes, then merge to master, or just do it all in one go? Please note filename extensions will be changed, too.

Comment 5

[Matjaz Horvat [:mathjazz]]

Thanks for taking it! :)

I assume diff will be massive, and will make code review daunting. Perhaps you can migrate module by module, each in a separate commit on the same branch? That way we could eventually still merge everything in one go, through one PR, and wouldn't need to simultaneously support Flow and TypeScript (not sure how big of an effort that is).

Comment 6

[Axel Hecht]

Are there things we can prepare in the flowed code ahead of time?

Not speaking for Julen, but I expect that module-by-module will be hard, given how intertwined the frontend code is.

Comment 7

[Matjaz Horvat [:mathjazz]]

Good points, Axel!

Adrian, I'm curious what migration strategy would you advocate for.

Comment 8

[abowler]

I agree with Matjaz that trying to do a thorough review of the conversion on the entire code base would be difficult to accomplish with certainty. However, Flow and Typescript do NOT communicate with one another making a gradual approach a bit more challenging.

I don't have a definitive answer to that problem, but I did find this article that documented how Zapier migrated their 500K line code base using an iterative approach that might be helpful: https://medium.com/@michaelsholty/migrating-500k-lines-of-flow-code-to-typescript-15a8cad43fec

Comment 9

[Julen Ruiz Aizpuru]

I'm not advocating this, but just to share an experience, in my previous job I witnessed a Flow -> TS migration of a large codebase (along the Zapier size), and the taken approach was:

• stop the world, i.e. code freeze the repo
• migrate components/modules one at a time, reviewing PRs in the process

There was a good pool of full-time devs to devote to the task, so that indeed helped.

With the steps I took the other day (flow-to-ts + some follow-up fixes), basically all modules are already ts(x). Compilation can be accomplished even with type errors (in CRA via TSC_COMPILE_ON_ERROR = true), but I think the goal here is to add proper type checking to every module and remove all errors (maybe without enforcing strict at first?)

I'm not sure how many people we can have actively migrating modules and reviewing PRs, but going one component/module at a time is worth considering if it makes it easier to offload the work across potential contributors.

That said, I don't want to underestimate the effort, but I sense this codebase is not big enough not to finish the task in a few days of active commitment (there, I said it), so there might be a bit of overthought here.

How about I share my ongoing work in a typescript branch, and from there on we move one module at a time?

Comment 10

[Julen Ruiz Aizpuru]

(In reply to Axel Hecht from comment #6)

Are there things we can prepare in the flowed code ahead of time?

I'm afraid I'm not experienced enough with neither Flow nor TypeScript to answer this properly.

Comment 11

[Adrian Gaudebert]

I don't have much experience with migrations like this. Based on what everyone said, I'm guessing a code freeze followed by a focus on this migration process might be the solution with the lowest friction. Any code changes during the migration process are doomed to cause conflicts, so avoiding that definitely would be a plus. As to how to share the load, I'm afraid what Axel said about inter-dependencies will prove true, making migrating a single module be close to migrating the whole thing. Maybe it's fine to have a branch that has broken code, and still do things module by module, not caring if it doesn't fully work until all modules are migrated, then do a pass at solving the remaining issues?

Comment 12

[GitHub Bugzilla PR Linker]

Attachment: Link to GitHub pull-request: https://github.com/mozilla/pontoon/pull/1869

Comment 13

[Axel Hecht]

I think we should move the conversation on how to make progress towards this from PR https://github.com/mozilla/pontoon/pull/1869 over to this bug.

One topics is the list of open dependencies for typescript definitions:

• [ ] diff-match-patch
• [ ] react-content-marker
• [ ] react-time-ago

The other topic is if there's an incremental path, and if modules are a good step stone. If they are, the question is how to track them, and I'd suggest to file bugs for each, and set deps on this bug.

Which leads me to the conversation between Julen and me about modules:

Pike:

Are there good success criteria for a module?

Julen:

is no errors in strict mode good enough? or do you have something else in mind?

My counter question would be, is a module going to be error-free as long as it depends on modules with errors? Or would this be "no errors attributed to the module in question"?

I guess this boils down to cross-module errors. On which side of the border do we fix those, and does that represent a bounded problem?

Comment 14

[Matjaz Horvat [:mathjazz]]

(In reply to Axel Hecht from comment #13)

The other topic is if there's an incremental path, and if modules are a good step stone. If they are, the question is how to track them, and I'd suggest to file bugs for each, and set deps on this bug.

And if they are, are core "modules" a good place to start, since they rarely depend on other modules?

Note that this can also be the case for modules in the modules folder, but I've only found unsavedchanges to meet the criteria.

Comment 15

[Julen Ruiz Aizpuru]

Thanks Axel for bringing the conversation here, and sorry for such a late follow-up — a lot has been going on and didn't have enough time and energy to get back to this. This was also the reason why I was advocating to move forward with the PR I provided, hoping someone else could also continue with the effort without it being "waiting" on me. Likewise, part of the reason why I suggest using Pontoon modules as a stepping stone.

Just in case, and before we get confused talking about different things, let's agree on differentiating a "Pontoon module" (semantically organized collection of source code, i.e. core/*, modules/*) and an "ES module" (ES source file with import/export statements). Here we are suggesting to move forward porting one "Pontoon module" at a time.

Regarding cross-module dependencies, I believe it's best to start from those modules which are more frequently depended upon (because they are less likely to internally depend on code from other Pontoon modules, hence it reduces the chance for cross-module errors), and as such I'd err on the side of considering a Pontoon module error-free as in your second definition, i.e. "no errors attributed to the module in question".

I tried multiple dependency graphing options but couldn't get them work with our baseUrl setting and absolute imports, so I wrote a throwaway script to get this info by grepping for core/ and modules/ imports. Module names are followed by the number of modules depending upon them:

core/api (53)
core/locale (32)
core/navigation (31)
core/entities (29)
core/utils (26)
core/user (26)
core/editor (26)
core/term (15)
core/plural (14)
modules/unsavedchanges (11)
core/project (10)
core/stats (10)
core/notification (9)
modules/history (9)
modules/batchactions (8)
modules/teamcomments (7)
core/resource (6)
modules/search (5)
core/comments (4)
core/l10n (4)
core/translation (4)
modules/otherlocales (4)
modules/machinery (4)
core/placeable (3)
core/lightbox (3)
modules/genericeditor (3)
core/diff (2)
core/loaders (2)
modules/navbar (1)
modules/resourceprogress (1)
modules/projectinfo (1)
modules/terms (1)
modules/entitydetails (1)
modules/entitieslist (1)
modules/interactivetour (1)
modules/fluenteditor (1)
modules/fluentoriginal (1)

So my suggestion would be to file 37 bugs for those modules mentioned above, depending on this particular bug (any volunteers to do that? 😬), but strictly follow a top-down order when migrating modules. Thoughts or any other specific suggestions to move forward?

In the meantime, I'll start rebasing the existing PR to account for the latest changes in master.

Comment 16

[Axel Hecht]

FYI, the flow-to-ts code has published an update to support ´import type´ as 0.3.0

Comment 17

[Axel Hecht]

We've talked about this some more in today's Pontoon call.

For now, it seems hard to commit to finish the migration to typescript on a definite timeline. We also can't stop the world, or continents thereon.

Thus we're looking for smaller incremental steps here.

There's a chance that we will get an easier migration path when we update flow first, and the plan we came up with was to upgrade flow incrementally. I'll file a rolling bug for that. Basically, folks having some cycles to invest will bisect an upgrade to flow that includes a number of fixes to make they're comfortable with. No discrete goal to migrate to, just make a step forward.

Hopefully that will get us closer to a migration to TS eventually, but also give us the best of the flow world in the meantime.

There's more open questions for the actual goal in the TS world for our frontend code, too. In particular how to handle cross-module dependencies on the type level, and how to handle cross-module dependencies on the code level. Concretely, do all cross-module dependencies go through foo/index.ts, or would it be better to have types be independent through something like foo/types.ts. Or more functional granularity?

Also, thanks to Julen here to help out. It's really important to have an idea of the costs ahead of us. Only then can we balance them with other planned work.

Comment 18

[Matjaz Horvat [:mathjazz]]

Thanks for the update, Axel! Seems like https://github.com/mozilla/pontoon/pull/1888 is taking us in the right direction.

Also, Greg just shared his "braindump" with me about switching from flow to TS for the Firefox DevTools Profiler:
https://github.com/firefox-devtools/profiler/issues/2931

Comment 19

[Axel Hecht]

I resolved bug 1704393, because we've decided to keep the tests in JavaScript in general, and potentially add or convert individual tests after the migration to TypeScript.

Comment 20

[Matjaz Horvat [:mathjazz]]

Attachment: GitHub Pull Request (#1938)

Comment 21

[GitHub Bugzilla PR Linker]

Attachment: Link to GitHub pull-request: https://github.com/mozilla/pontoon/pull/1942

Comment 22

[GitHub Bugzilla PR Linker]

Attachment: Link to GitHub pull-request: https://github.com/mozilla/pontoon/pull/1943

Comment 23

[GitHub Bugzilla PR Linker]

Attachment: Link to GitHub pull-request: https://github.com/mozilla/pontoon/pull/1944

Comment 24

[GitHub Bugzilla PR Linker]

Attachment: Link to GitHub pull-request: https://github.com/mozilla/pontoon/pull/1945

Comment 25

[GitHub Bugzilla PR Linker]

Attachment: Link to GitHub pull-request: https://github.com/mozilla/pontoon/pull/1949

Comment 26

[Axel Hecht]

https://github.com/mozilla/pontoon/commit/a77b8cdb6735925027f61515b0c8e6f63e3f578a landed, the initial migration is done.

The remaining work will be in follow-ups, so marking this one FIXED.