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

[HybridApp] Configure HybridApp setup process and local environment #49845

Open
Julesssss opened this issue Sep 27, 2024 · 20 comments
Open

[HybridApp] Configure HybridApp setup process and local environment #49845

Julesssss opened this issue Sep 27, 2024 · 20 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Daily KSv2 Engineering Improvement Item broken or needs improvement. Internal Requires API changes or must be handled by Expensify staff

Comments

@Julesssss
Copy link
Contributor

P/S authored by @staszekscp here

Problem

One of our biggest headaches is a smooth HybridApp local setup for the developers. I noticed that maybe the most problematic part was to wrap the head around the monorepo structure, and the fact that we have to remember about updating both repositories. This is how the structure looks like:

  • 📂 Mobile-Expensify
    • 📂 Android: Expensify Classic Android specific code
    • 📂 app: Expensify Classic JavaScript logic (aka YAPL)
    • 📂 iOS: Expensify Classic iOS specific code
    • 📂 react-native: git submodule that is pointed to Expensify/App
      • 📂 android: New Expensify Android specific code (not a part of HybridApp native code)
      • 📂 ios: New Expensify iOS specific code (not a part of HybridApp native code)
      • 📂 src: New Expensify TypeScript logic

Solution

For the beginning we should create a simple bash script to recognise if we’re currently working on the HybridApp. It could look up one directory, and see if in its package.json the value for the name key is mobile-expensify. 💻

Then, let’s migrate/create the following scripts in Expensify/App repo in order to minify overhead during the transition to HybridApp.

  • setup-hybrid-app - NEW - helper script (in parent Expensify/Mobile-Expensify repo) that would do all necessary setup to start working with the HybridApp. Its responsibilities would be to:
    • help you to set your fork as the NewDot submodule’s origin
    • install dependencies for both, ND and OD
  • pull-old-dot - NEW - helper script that would go to OD repo, and pull the changes, so you don’t have to jump between directories
  • postinstall - add execution of npm i command in OldDot repo. It would also print out information if your OldDot repo is out of date, and inform you if you should run npm run pull-old-dot - the OldDot repo doesn’t change so often, so it’s easy to forget about it 😄
  • ios, pod-install, ipad, ipad-sm, android, clean - if they’re executed in the HybridApp, run the commands in the HybridApp context (eg. build/clean HybridApp, not NewDot). Otherwise let them work as before. If someone works on the HybridApp, but would like to build NewDot, we can add a --nd flag. In this way running npm run ios --nd in HybridApp would build a standalone NewDot application

By introducing or modifying the scripts I’d like to limit to the minimum unnecessary switches between OldDot and NewDot. We could stay focused on the most important work in the React Native code, but in case of emergency we could jump up, and see what’s going on in OldDot! 😄

The key takeaway is that on HybridApp we’ll build the native code from another place, therefore the android and ios folders in Expensify/App can be used only to build standalone NewDot, not the HybridApp. I know it may be a bit confusing at this moment, and we’ll point out in the README 😄 Nevertheless by using the proposed commands we should be able to minimise that problem. The android/ios folders will have to stay, though, because many OS contributors will only have access to NewDot, as it is at this moment!

@Julesssss Julesssss added Engineering Weekly KSv2 Improvement Item broken or needs improvement. Internal Requires API changes or must be handled by Expensify staff labels Sep 27, 2024
@Julesssss Julesssss self-assigned this Sep 27, 2024
@staszekscp
Copy link
Contributor

Hey! I'll be working on this issue!

@trjExpensify trjExpensify changed the title Configure HybridApp setup process and local environment [HybridApp] Configure HybridApp setup process and local environment Sep 30, 2024
@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Weekly KSv2 labels Oct 4, 2024
@AndrewGable
Copy link
Contributor

Sorry it's taken me a little bit to write this down, I think the goal is this:

New contributor from open source world

  1. git clone [email protected]:Expensify/App.git
  2. npm install
  3. npm run android / cd iOS && pod install && cd .. && npm run ios
  4. This should run only New Expensify (they won't have access to Mobile-Expensify)
  5. They should be hitting the staging API (they don't have access to dev API)

Existing contributor from open source world

No changes, everything should work the same

New Contributor+/Expert Contributor from Software Mansion

  1. git clone [email protected]:Expensify/App.git
  2. npm setup-hybrid (This should clone Mobile-Expensify up one directory)
  3. npm install (This should install Mobile-Expensify packages, patches, and env as well)
  4. npm run android / cd iOS && pod install && cd .. && npm run ios
  5. This should run HybridApp (they do have access to Mobile-Expensify)
  6. They should be hitting the staging API (they don't have access to dev API)

Existing Contributor+/Expert Contributor from Software Mansion

  1. npm setup-hybrid (This should clone Mobile-Expensify up one directory)
  2. npm install (This should install Mobile-Expensify packages, patches, and env as well)
  3. npm run android / cd iOS && pod install && cd .. && npm run ios
  4. This should run HybridApp (they do have access to Mobile-Expensify)
  5. They should be hitting the staging API (they don't have access to dev API)

New Expensify employee

  1. git clone [email protected]:Expensify/App.git
  2. npm setup-hybrid (This should clone Mobile-Expensify up one directory)
  3. npm install (This should install Mobile-Expensify packages, patches, and env as well)
  4. npm run android / cd iOS && pod install && cd .. && npm run ios
  5. This should run HybridApp (they do have access to Mobile-Expensify)
  6. They should hit whatever API they have configured in .env in the root of App

Existing Expensify employee

  1. npm setup-hybrid (This should clone Mobile-Expensify up one directory)
  2. npm install (This should install Mobile-Expensify packages, patches, and env as well)
  3. npm run android / cd iOS && pod install && cd .. && npm run ios
  4. This should run HybridApp (they do have access to Mobile-Expensify)
  5. They should hit whatever API they have configured in .env in the root of App

Questions:

  1. Are we able to combine npm install and npm setup-hybrid steps? Maybe we can check to see if the user has access quickly, then clone the repo in the same flow.
  2. Are we all in agreement that this is the most ideal DevX?

@AndrewGable
Copy link
Contributor

cc @mateuuszzzzz @war-in ☝️ for any other ideas, questions, etc.

@Julesssss
Copy link
Contributor Author

This is a huge change that we're not ready to move forward yet.

@staszekscp
Copy link
Contributor

Hey @AndrewGable! Thanks for the post! I like the idea with running the setup-hybrid-app in the Expensify/App repo - I'll just have to investigate how to connect the two repos in this scenario, because we've hardcoded the react-native submodule's name, but it seems that we would have to set it more dynamically 😄 I'm just worried that it may cause some unexpected problems - after all we'll have to change the path to the current working directory (instead of $SOME_DIR/App, we'll have to add a directory and insert the whole repo to ``$SOME_DIR/Mobile-Expensify/App`. I'm afraid that it may break some local setups, but I'll have to investigate further to see what may be the real repercussions 😄

Are we able to combine npm install and npm setup-hybrid steps? Maybe we can check to see if the user has access quickly, then clone the repo in the same flow.

If we correctly get and setup the Mobile-Expensify repo it should be straightforward - I've already done a similar thing in my previous POC. 😄

npm run android / cd iOS && pod install && cd .. && npm run ios

Should these commands be executed in App or Mobile-Expensify? If we'd like to do it in App, we would have to move up one directory to Mobile-Expensify - otherwise we would install NewDot's pods. I don't really see a perfect way to solve this problem, though... I came up with modifying the npm run pod-install command, because otherwise we would have to change directories manually, which may be confusing. 🤔

@AndrewGable
Copy link
Contributor

I think ideally these commands are all run in App

@staszekscp
Copy link
Contributor

Thanks! In that case I've started experimenting with the repo to see how to implement such flow - I'll come back with an update once I have some conclusions 😄

@staszekscp
Copy link
Contributor

staszekscp commented Oct 18, 2024

Hey @AndrewGable @Julesssss! I've played with the repo, and I come with first conclusions, so let me refer to the desired steps for the HybridApp setup 😄

Analysis

  1. git clone [email protected]:Expensify/App.git

I think it's a good idea to keep this as the first step for all contributors!

  1. npm setup-hybrid (This should clone Mobile-Expensify up one directory)

I like the idea of executing the command in the App repository, it would keep the setup easier. However I've noticed a handful of problems with simply cloning Mobile-Expensify up one directory.

Firstly, we would have to do some pretty nasty stuff with changing the directory structure, which may seem suspicious, and unintentionally break some local setups. The script would have to clone the Mobile-Expensify, copy and remove the current directory in order to insert it into react-native. Apart from that the devs would be forced to reopen the IDE, too.

My suggestion would be to try to clone the repo at the same level as App is located, and then set the react-native submodule to have the same origin as the App repo. The devs would have to open a new IDE window, but the general structure could be pretty clean:

  • 📂 Mobile-Expensify (HybridApp)
    • 📂 react-native (NewDot)
  • 📂 App (NewDot)

I've tried to create a symlink connection between App and react-native in order to have only one NewDot repo, but unfortunately React Native is apparently not ready for symlinks yet. Apart from multiple build errors, I bumped into a 9-year-old watchman issue, where devs asked for symlink support, unfortunately still not resolved. 😢

Nevertheless the potential drawback of having two repos doesn't have to be so devastating, since it would give us a simple way of switching workspaces in order to build standalone NewDot

  1. npm install (This should install Mobile-Expensify packages, patches, and env as well)

This step should be pretty straightforward - I've already done it in my previous POC. We could also combine it with the previous step, but I don't think it would be a big problem for devs to execute it on their own.

  1. npm run android / cd iOS && pod install && cd .. && npm run ios

The npm run android/ios commands would be easily replaceable - I've done a similar thing in my POC. I think that if the devs have both - App, and Mobile-Expensify we could simplify the usage to run only HybridApp within Mobile-Expensify, without any additional flags that would allow devs to build the standalone NewDot app. If they want to do that, they can simply change workspaces.

The problem appears with the pod install command. We cannot just swap the ios directory content, because it would break YAPL, or be visible as change in git. Alternatively we could change the Podfile script to execute the OldDot's Podfile, but it seems to be too "magic", and confusing (eg. Pods would be installed in some other directory). Because of that I would like to keep the current directory structure with a number of enhancements:

  1. Let's encourage people to use npm run pod-install instead of cd ios && pod install. Whenever the latter is executed we can eg. throw an error. On the other hand npm run pod-install would execute the OldDot's Podfile without need to change directories.
  2. In order to avoid confusion we can recommend "hiding" android/ios folders in the IDE. We can provide a simple VSC and WebStorm workspace settings file, so people accidentally don't change them while working on the HybridApp. This way in the whole workspace they would see only one android/ios folder, and it would be helpful eg. while cleaning the pods.
  3. We can add a git commit hook to check if someone has changed NewDot's native code while working on HybridApp. We can also add a similar check to npm run andorid/ios to make sure if someone hasn't change wrong native code by accident.

As an additional suggestion I think we should change the submodule's name from react-native to eg. new-dot or new-dot-hybrid, to avoid confusion - it seems like it's the react-native core code, rather than NewDot. Also, we could get rid of paths like: node_require('../react-native/node_modules/react-native/scripts/react_native_pods.rb'), which includes the path to react-native installed in node_modules

Summary

Having said all this, my suggestion would be to create such flow:

  1. git clone [email protected]:Expensify/App.git -> if it's a new contributor
  2. npm run setup-hybrid -> clone Mobile-Expensify
  3. Close IDE and change workspace to Mobile-Expensify/new-dot
  4. npm install in new-dot (old react-native) -> install Mobile-Expensify packages, patches, and env as well
  5. npm run android / npm run pod-install && npm run ios -> run new-dot (old react-native) should build HybridApp

To sum up there are two changes between this suggestion, and @AndrewGable's proposal:

  1. The devs would have to change workspace after the HybridApp setup
  2. The devs would have to run npm run pod-install instead of cd iOS && pod install, but since the npm run pod-install is already a recommended way of installing pods in NewDot that shouldn't be a big deal if we throw an error whenever someone executes cd iOS && pod install

The current flow for contributors without an access to Mobile-Expensify shouldn't change.

If you have any questions, please feel free to ask! I'll gladly explain everything that is unclear! 🙏

@AndrewGable
Copy link
Contributor

I need to dig into the problems mentioned in step 2, I think if we can get that to work than we will be a lot better off than having two repos

@staszekscp
Copy link
Contributor

staszekscp commented Oct 21, 2024

It's worth to point out that if we're going to stay with one repo, I think we should have a way of building a standalone NewDot application locally - it could be beneficial eg. for debugging some HybridApp-specific problems. 😄 We could eg. add/modify some scripts to start a separate NewDot builds.

On the other hand we can create a script that would leave the devs with only one project that contains HybridApp structure. The previous App project can be eg. copied to react-native submodule. I've tested this solution, and unfortunately my IDE doesn't really understand what's going on, and it would have to be restarted - we could do it from the terminal, but I think it seems suspicious to build a script that would modify folder structure, path to the current workspace, and restart the IDE 😞

I think if we'd like to move on in this direction, we should create an additional prompt with clear information what is going on 😄

@melvin-bot melvin-bot bot added Monthly KSv2 and removed Weekly KSv2 labels Nov 14, 2024
Copy link

melvin-bot bot commented Nov 14, 2024

This issue has not been updated in over 15 days. @Julesssss, @staszekscp eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

@github-project-automation github-project-automation bot moved this from HIGH to Done in [#whatsnext] #quality Dec 11, 2024
@Julesssss Julesssss reopened this Dec 11, 2024
@Julesssss Julesssss mentioned this issue Dec 11, 2024
50 tasks
@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Dec 12, 2024
@melvin-bot melvin-bot bot changed the title [HybridApp] Configure HybridApp setup process and local environment [HOLD for payment 2024-12-19] [HybridApp] Configure HybridApp setup process and local environment Dec 12, 2024
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Dec 12, 2024
Copy link

melvin-bot bot commented Dec 12, 2024

Reviewing label has been removed, please complete the "BugZero Checklist".

Copy link

melvin-bot bot commented Dec 12, 2024

The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.74-8 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2024-12-19. 🎊

For reference, here are some details about the assignees on this issue:

@Julesssss
Copy link
Contributor Author

We merged and resolved a few build issues yesterday. There is also this crash due to yapl not building correctly, I am close to a fix here.

We'll then continue to keep an eye out for local build issues. I will need to share a message for internal engineers at some point too.

@Julesssss Julesssss changed the title [HOLD for payment 2024-12-19] [HybridApp] Configure HybridApp setup process and local environment [HybridApp] Configure HybridApp setup process and local environment Dec 12, 2024
@Julesssss Julesssss added Daily KSv2 and removed Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production labels Dec 12, 2024
@Julesssss
Copy link
Contributor Author

Shared details with internal engineers here.

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Daily KSv2 labels Dec 13, 2024
@melvin-bot melvin-bot bot changed the title [HybridApp] Configure HybridApp setup process and local environment [HOLD for payment 2024-12-20] [HybridApp] Configure HybridApp setup process and local environment Dec 13, 2024
Copy link

melvin-bot bot commented Dec 13, 2024

The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.75-6 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2024-12-20. 🎊

For reference, here are some details about the assignees on this issue:

@Julesssss
Copy link
Contributor Author

Working through internal build issue here.

@Julesssss Julesssss changed the title [HOLD for payment 2024-12-20] [HybridApp] Configure HybridApp setup process and local environment [HybridApp] Configure HybridApp setup process and local environment Dec 19, 2024
@Julesssss
Copy link
Contributor Author

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Dec 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Daily KSv2 Engineering Improvement Item broken or needs improvement. Internal Requires API changes or must be handled by Expensify staff
Projects
Development

No branches or pull requests

3 participants