Implement TypeScript in New Expensify

[roryabraham]

Proposal: Utilize TypeScript to reduce bugs in NewDot

📄 Link to the design doc

docs.google.com/document/d/1LNcgYeYyovQ88gNjLIgBqBReLtbLAbxeAbna9QKxeAQ/edit#

🖇️ Other links

• TypeScript configurations
• File by file migration plan

Strategy

Our strategic roadmap for the next couple years is Reunification – moving all of our existing users from OldDot to NewDot, and it’s going to look like a completely new product. During this transition period, there’s an increased risk for churn, and stability of the platform is more important than ever. Any lines of defense between our customers and bad code will pay dividends during any tumultuous times to come.

Problem

JavaScript has been very popular for front-end development for a long time, and over the years it has evolved with new developer-friendly features that make it easier to GSD. Overall, it’s pretty great. However, it is still plagued by one fundamental problem: runtime errors stemming from incorrect types. This common class of problem leads to bugs, ranging in severity from showing weird things on the screen to crashing the whole app.

While there are many bugs that can be caused by incorrect types, the most common example of a runtime type safety error is Cannot access property X of undefined. A quick GitHub search reveals that over the last couple years we have introduced hundreds of this type of bug into the NewDot codebase, each of them stemming from this same root cause – attempting to access a property on a variable whose value is undefined. When this occurs, the app crashes.

Solution

Let's commit to adopting TypeScript in New Expensify, an awesome superset of JavaScript that gives us the benefits of using a compiled language like C++! That way, type errors can be caught at compile-time instead of run-time, and a whole class of common bugs will be eliminated from our product. #BugZero here we come.

Furthermore, let’s engage the near-limitless capacity of our open-source community to do it in parallel to our existing roadmap, without pausing any other feature development in the meantime.

Tasks

• [x] Post Proposal (full Problem/Solution statement) in #expensify-open-source
• [x] Wait at least one full business day, and until the post has a majority (2/3) of positive reactions (👍)
• [x] Paste Proposal in the space above with a link to the Slack thread
• [x] Email strategy@expensify.com and paste in the Proposal
• [x] Fill out the High-level overview of the problem, Timeline, and Terminology sections of the Design Doc
• [x] Email strategy@expensify.com (continue the same email chain as before) with the link to your Design Doc
• [x] Host a pre-design meeting (example) in #expensify-open-source to discuss any necessary details in public before filling out the High-level of proposed solution section.
• [x] Fill out the High-level of proposed solution section
• [x] Email stategy@expensify.com again with links to the doc and pre-design conversation in Slack
• [x] Add the DesignDocReview label to get the High-level of proposed solution section reviewed
• [x] Respond to any questions or concerns and bring up blockers in Slack to get a consensus if necessary
• [x] Confirm that the doc has the minimum necessary number of reviews before proceeding
• [x] Host another pre-design meeting in #expensify-open-source to ask for engineering feedback on the technical solution.
• [x] Fill out the Detailed implementation of the solution and related sections.
• [x] Re-add the DesignDocReview label to this issue
• [x] Respond to any questions or concerns and bring up blockers in Slack to get consensus if necessary
• [x] Confirm that the doc has the minimum necessary number of reviews before proceeding
• [x] Email strategy@expensify.com one last time to let them know the Design Doc is moving into the implementation phase
• [x] Implement the changes
• [ ] Send out a follow up email to strategy@expensify.com once everything has been implemented and do a Project Wrap-Up retrospective that provides:
  • Summary of what we accomplished with this project
  • What went well?
  • What could we have done better?
  • What did we learn?

[AndrewGable]

I have read and reviewed this Design Doc!

[hayata-suenaga]

Is this a super high priority? It looks like it was created with a Daily tag for everyone to review in one day, which normally we reserve for things that are impacting the next couple waves. @danielrvidal

This issue has the "daily" label because this project involves external vendors who already started working on some tasks. Migration is one of those projects that don't have a clear deadline. But they're also the kind of projects that benefit from a short lifespan.

As this design dos is rather short, we thought we could justify requesting reviews with the daily label 🙇

[flodnv]

I have read and reviewed this Design Doc!

[johnmlee101]

I have read and reviewed this Design Doc!

[puneetlath]

I have read and reviewed this Design Doc!

[maddylewis]

I have read and reviewed this Design Doc!

[NickTooker]

I have read and reviewed this Design Doc!

[danielrvidal]

I have read and reviewed this Design Doc!

[PauloGasparSv]

I have read and reviewed this Design Doc!

[twisterdotcom]

I have read and reviewed this Design Doc!

[roryabraham]

We've seen good activity and detailed reviews have been moving along nicely.

[hayata-suenaga]

The detailed review is done 🎉

Here are the next steps.

For the assignment and division of tasks and tracking of subtasks of these four tasks, we delegate them to SWM and CallStack. Please communicate with each other to divide responsibilities 🙇 @fabioh8010 @blazejkustra

For each of these four tasks, I created a GH issue. We'll eventually put these issues inside a GH Project we'll create for the TypeScript migration. Please report any progress in the respective issue.

I provided rough deadlines for each task. These are more like guidelines rather than strict deadlines. Please provide your own estimate on the completion time of the task in the issue itself.

1. Define TypeScript guidelines

• GitHub: https://github.com/Expensify/App/issues/20623
• Who: SWM and CallStack.
• Rough deadline: We want to send the guideline for review by internal engineers by the end of this week

A rough overview of the guideline is in the design doc. Let's create the first version of the guideline in app/contributingGuides/TYPESCRIPT_STYLE.md Let's add succinct code examples as needed. Create sub-files if necessary to divide the guideline into sections.

Please write the guideline following conventions widely used in open-source communities and also those already used in other guidelines in App (ex. usage of shall, should, or must).

2. Prepare libraries for migration

• Who: SWM and CallStack.
• Rough deadline: By mid-next week if possible (I don't know how complicated it will be to creat type definitions for onyx)

Create type definitions for react-native-onyx and expensify-common. Until these definitions are complete, we cannot move to the migration of App.

• react-native-onyx GH isssue: https://github.com/Expensify/react-native-onyx/issues/260

• expensify-common GH issue: https://github.com/Expensify/expensify-common/issues/547

3. Prepare the GH project and issues

• GH issue: This task doesn't have a corresponding GH issue.
• Who: SWM and CallStack.
• Rough deadline: End of this week

@fabioh8010 is already testing GitHub's Project feature and figuring out how to create migration issues.

4. Setup App for TypeScript

• GH issue: https://github.com/Expensify/App/issues/20626
• Who: SWM and CallStack.
• Rough deadline: PoC PR is getting quite complete. Do you think we can finish it by the end of this week?

[melvin-bot[bot]]

@neil-marcellini, @roryabraham, @hayata-suenaga Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[neil-marcellini]

The PR to set up the app for typescript is under review and I just finished my review. I'm eager to keep helping out mostly by reviewing.

[roryabraham]

The main PR to integrate TypeScript toolchain into E/App is in review. Exciting!

[neil-marcellini]

I started reviewing the Typescript guidelines PR today.

[blazejkustra]

PR with type definitions for expensify-common is ready here. Please check it out! 👀

[hayata-suenaga]

With amazing work by @fabioh8010 and @blazejkustra, we're on a good track 🚀 There are the latest updates!

• RP for adding type definition files for expensify-common is under review
• GH project for TS migration was created. (it's still private, but the visibility will be changed to public soon)
• @mountiny authored a PR for a script that creates GH issues for all migration tasks
• TS guideline is under review. @hayata-suenaga is going to review it this weekend.
• PR for setting up configurations for TypeScript on NewDot is under review and almost ready

Next Steps

• Start working on writing type definition files for react-native-onyx
• CallStack and SWM dividing work for the initial set of migration tasks (migration of leaf files)
• Publish TypeScript guideline
• Post a slack comment about TypeScript migration on #expensify-open-source channel
  • Tell contributors not to post proposals to migration issues yet

[hayata-suenaga]

The TypeScript GH Project now has the public visibility

[melvin-bot[bot]]

@neil-marcellini, @roryabraham, @hayata-suenaga Whoops! This issue is 2 days overdue. Let's get this updated quick!

[hayata-suenaga]

A new PR for TS guidelines was created, and I expect the review to be finished by the end of this week.

@fabioh8010 is working on creating type definition files for react-native-onyx

script for creating GH migration issues is ready

We should start thinking about typing core data. The PoC PR for this is already made here by @thiagobrez.

cc: @fabioh8010 @blazejkustra

[neil-marcellini]

We're planning to merge the expensify-common PR by EOD and the App set up PR by EOW.

[hayata-suenaga]

• Finalizing the decision regarding the module organization strategy here

• Asked the progress on typing core pieces of data on slack

[cead22]

I have read and reviewed this Design Doc!

[hayata-suenaga]

Current progress

• react-native-onyx PR is in progress.
• expensify-common PR has been merged.
• TS setup PR is finalizing with additional ESLint rules
• TS guideline PR is almost ready
• Typing core data is WIP
• script for creating migration GH issues are ready

[hayata-suenaga]

Requested internal engineers' review on the core data PR in NewDot here.

[roryabraham]

Progress is ongoing. We were discussing a related change (that can happen separately from the main TS migration) in these threads:

• https://expensify.slack.com/archives/C0593T50UHG/p1688115737721429
• https://expensify.slack.com/archives/C01GTK53T8Q/p1688767129082749

I personally think the most significant decisions that are being made are how we are typing react-native-onyx, and I sadly am not up-to-date on the latest there

[hayata-suenaga]

the most significant decisions that are being made are how we are typing react-native-onyx, and I sadly am not up-to-date on the latest there

@roryabraham

The draft PR for creating definition files in Onyx is going here.

[hayata-suenaga]

End of week update

• The guideline PR is done and waiting everyone's approval.
• PR for adding type definition for Onyx is progressing nicely. We don't know if we want to migrate to useOnyx along with the TS migration. The discussion is happening here
• PR for tying Onyx data is being reviewed

The following PRs are waiting for the migration to start

• TS setup for App
• Script for creating migration GH issues
• De-structure props

Action items for next week

• Decide if we migrate to useOnyx during TS migration
• Finish TS-Onyx PR

[roryabraham]

Had a sync with @hayata-suenaga today. The main action item at the moment is getting all the reviews in for this big PR

[hayata-suenaga]

Mid week update

• TS guideline is almost ready. Lets merge it by the end of this week and announce it on the open source channel so that contributors will have time to look over the guideline before the migration starts (migration tasks will be only opened to contributors in the later stage of the migration, but they might encounter merge conflicts and have to edit TypeScript code)

• Onyx + TS PR is still being actively reviewed and worked on. Let's aim to merge this by the end of Wednesday next week 👍

• @blazejkustra let us know if you need more help from internal engineers for the core Onyx data + TS PR. Seems like all files are reviewed by internal engineers at least once.

• Let's run the script to create all migration tasks once the Onyx + TS PR and core Onyx data + TS PR are merged next week.

• (internal) We probably have to edit the Web-E review process to update the core Onyx data typing when the shape of backend data sent to App changes. @roryabraham , @mountiny, and @neil-marcellini do you have any idea how we can make sure that engineers are reminded to update the TS type definitions for Onyx data when the backend data shapes change?

[mountiny]

@roryabraham , @mountiny, and @neil-marcellini do you have any idea how we can make sure that engineers are reminded to update the TS type definitions for Onyx data when the backend data shapes change?

I am not sure, maybe some action/ webhook analyzing the code changes

@blazejkustra @fabioh8010 any recommendations?

[blazejkustra]

@roryabraham , @mountiny, and @neil-marcellini do you have any idea how we can make sure that engineers are reminded to update the TS type definitions for Onyx data when the backend data shapes change?

I am not sure, maybe some action/ webhook analyzing the code changes

@blazejkustra @fabioh8010 any recommendations?

So, there are couple of ways to do it:

1. Custom-Built Codegen: Although we don't use GraphQL on the backend, we can draw some inspiration from GraphQL. The concept is to implement a code generator, which could automatically generate TS type definitions based on our backend data shape. Writing a custom codegen for a PHP backend might be a challenging task. Such approach might require some investment of time and resources initially but has the potential to make the backend-frontend sync easier in the longer run.
2. Publicly Defined Schema: Similar approach, publicly define the schema and utilize it at both ends - backend and frontend. JSON Schema could be utilized for that. To ensure that backend is in sync with the schema, input and output of every request would be validated with the schema. On the frontend we could either create types manually and validate incoming data in the runtime or use json-schema-to-typescript to generate TS types automatically.
3. New Development Process: New step in the review process, whenever backend data shapes are changed require a change on the frontend. That could be backend developer responsibility to make sure that changes are reflected in the types on frontend. Couple ideas:
   • Additional checkbox in checklist on the backend PR
   • Automation that checks data shapes on each commit on the backend, if needed require a frontend change (not sure if that's possible because I don't have access to the backend)
   • Runtime validation in the App, so that types defined in the App are compared with what we get from the API (Zod could be useful for that)

[hayata-suenaga]

@mountiny I was asking how we can change the internal process (add new checklist item, etc) for Web-E PRs so that engineers are reminded to change the type definition of the backend data changes.

should we just edit the PR template for Web-E? Should we also add a checklist for Auth, too?

[mountiny]

@hayata-suenaga I am curious though if checklist will be enough to check for this. I think we can start with it and see if we will have people ignoring it

[hayata-suenaga]

@mountiny yes I think the option #2 @blazejkustra suggested is something we want to consider down the line

but I'm thinking something we can implement right now that at least do something toward ensuring type consistency between front end and backend.

I created an internal issue to update templates for Web-E and Auth PRs, I gonna make PRs for them today

[roryabraham]

@roryabraham , @mountiny, and @neil-marcellini do you have any idea how we can make sure that engineers are reminded to update the TS type definitions for Onyx data when the backend data shapes change?

I think we should punt this problem (or maybe rumor of a problem) for now and just focus on getting TypeScript built out in a leaner V1. Long-term I love the idea of having a shared, versioned schema for Onyx used by both the front-end and back-end to automate the compatibility between layers. But I feel strongly that we're getting ahead of ourselves here.

We don't currently have anything to ensure Onyx schemas are compatible in the front-end and back-end, and it's been working mostly ok. The amount of type safety we'll get in the front-end alone will be a huge win.

FWIW, if someone's updating something in Onyx in PHP, they'll probably try to access that property in E/App, and notice that the type is wrong. I kind of think that's enough to start.

[roryabraham]

That said, if we're going to do anything I agree with @mountiny's suggestion that we can add a webhook comment on API PRs like:

I noticed that you changed some code that has to do with Onyx. Did you adjust the Onyx schema? If so, make sure you adjust the corresponding Onyx type declaration in E/App.

We could probably do that pretty easily but I don't think it should be a top priority for this project.

[hayata-suenaga]

Will merge the guideline PR this afternoon and post the announcement

[neil-marcellini]

I'm continuing to review the PRs to type Onyx data in App and to add type declarations for Onyx. Both are very close I think.

[hayata-suenaga]

Mid-week update

• The TS guideline PR has been merged, and announcements are posted in the #expensify-open-source and #engineering-chat (internal) slack channels
• App TS Setup PR is ready. However, @fabioh8010 discovered that TS is very slow when integrated on VSCode. Needs investigation here to see if this is an issue that happens to everybody.
• The review for Onyx core data + TS PR progresses a lot thanks to @neil-marcellini. What is the time estimate for this PR to be merged?

Once the above two PRs are merged, we can execute this script to create migration GH issues and officially kick off the TS migration.

Side notes

I realized there are additional libraries being developed right now at Expensify for App

• https://github.com/Expensify/react-native-x-maps
• https://github.com/Expensify/react-native-x-draggable-list

We probably have to rewrite them in TypeScript or write TS definitions for them at some point. Just a note. I'll keep track of the related project and update here.

[blazejkustra]

The review for https://github.com/blazejkustra/App/pull/1 progresses a lot thanks to @neil-marcellini. What is the time estimate for this PR to be merged?

@hayata-suenaga There is a TODO list in the description of the PR explaining what’s left. Currently the only blocker is react-native-onyx PR, once it's merged, I’ll install the typed version of Onyx and the PR should be ready.

[hayata-suenaga]

Merge Schedule

App TS Setup PR ->

Onyx + TS PR -> Core Onxy Data in App

[hayata-suenaga]

Weekly Update

• I created a GH issue to dump notes on things we have to remember during the migration. Please feel free to post anything as notes to the issue.

• [ ] A performance issue with TypeScript Intellisense was discovered in the App TS Setup PR. One source of the issue was identified and fixed. @blazejkustra is working on the second source of the issue. The latest update is here.

• [ ] Fabio has done amazing work so far with Onyx + TS PR. He's finalizing on some details of the PR but it's very close to the finish line 🥳

• [ ] @Blajez addresses all PR reviews swiftly and Onyx core data PR is ready. Please give a final look and approve it if it looks good. The PR is, however, on hold for #20179 (the App TS Setup PR).

  Also, Blajez, the PR is for your own branch. could you make a PR for the main branch of App? <img width="400" alt="Screenshot 2023-07-28 at 10 51 28 AM" src="https://github.com/Expensify/App/assets/98560306/777520a4-273d-49c0-a344-6bf6a09fcb08"

[neil-marcellini]

Heads up, I'm going to be OOO Monday and Tuesday next week so please don't let me be a blocker for anything while I'm out.

[roryabraham]

Update from @blazejkustra today here

[blazejkustra]

Mid-week update

• App TS Setup PR was merged yesterday 🚀 (there was a regression reported, a fix is already merged)
• Add type declarations PR is under review, @fabioh8010 is making the final touches.
• Original Onyx type core data is now recreated on Expensify/App, here is a brand new PR. It is blocked by https://github.com/Expensify/react-native-onyx/pull/267.

After react-native-onyx & Onyx type core data PRs are merged, we can proceed to phase 2 of Typescript migration! 🚀

[hayata-suenaga]

I didn't do end of week update last week 😭 but it's already time for...

Mid-week update

• Onyx PR: Create definition files Thank you @fabioh8010 to really dig into TypeScript and type Onyx as strictly as possible. I think this PR is ready to be merged. Just checking we can actually merge this in slack.
• App PR: Type core data This PR has been reviewed. Thank you @blazejkustra for handling this big PR 🙇 As @blazejkustra described here and as I described in this slack message, there was a major update in Onyx and the version is being bumped in App in this PR. So we have to hold the App Setup PR for it (because the App Setup PR increases the Onyx version further).

Once the App Setup PR is merged, then we're officially and finally ready to start migration.

We first have to run this script (sorry internal link) to create GH migration issues in the TS migration project (this one should be public link).

If I remember correctly, we will start migration from leaf files and the first phase of the migration is conducted only by CallStack and SWM engineers. Checking with @blazejkustra and @fabioh8010 how these expert agencies plan to coordinate with each other in allocating issues.

[hayata-suenaga]

End of week update

• Onyx PR: Create type declaration files Merged 🥳 🥳 🥳 ! Thank you so much @fabioh8010 and @blazejkustra for your amazing work on this one

• App PR: Type core data As @blazejkustra described https://github.com/Expensify/App/pull/24054 and as I described in this slack message, there was a major update in Onyx and the version is being bumped in App in https://github.com/Expensify/App/pull/24054 which changes APP code to work with the major change in Onyx. So we have to hold our PR for it.

Once the App Setup PR is merged, then we're officially and finally ready to start migration.

Next week, we gonna run this PHP script (internal link) with the updated CSV file that @fabioh8010 is going to provide to create migration issues.

In the initial phase of the migration:

• SWM is planning to assign one more engineer
• CallStack is planning to assign two more engineers

SWM and CallStack are going to coordinate with each other to assign issues to themselves.

We have to make sure that engineers from these organizations have permissions to assign themselves to issues on GH.

Additional Note: @blazejkustra and @fabioh8010 are OOO on next Monday and Tuesday

[hayata-suenaga]

Quick Update

Here is the latest update in the issue that is blocking @blazejkustra's Onyx core data App PR. The blocking PR hasn't been merged yet.

Once the above PR is merged, we gonna also merge this PR that adds a new checklist item in the backend PRs (Web-E and Auth) to encourage engineers to update the Onyx Data TS definitions if they change data shape.

Once @fabioh8010 returns from OOO, we can run this PHP script (internal link) to create GH issues for migration tasks with the updated CSV file that @fabioh8010 is going to provide us.

Also, we were originally planning to de-structure component props before the migration starts, but the --fix for eslint didn't work well with the react/destructuring-assignment rule, and I could't de-structure props in batch. So we have to manually de-structure components when migrating to TS.

I believe this rule is not currently turned on for TS files. So I gonna open a PR for it so that ESLint warns if we forget destructure props when migrating.