[No QA]Add port flag to allow iOS and Android to run simultaneously by AndrewGable · Pull Request #11813 · Expensify/App

AndrewGable · 2022-10-13T17:21:47Z

Details

Adds a --port flag to the metro/react native cli commands for iOS and Android to allow all four clients to be run at the same time.

Fixed Issues

$ https://github.com/Expensify/Expensify/issues/233635

Tests

Delete your current pods (rm -rf ios/Pods)
Reinstall them (cd ios && pod install)
Run iOS, Android, Desktop, and Web all at the same time in different terminal tabs
Verify any changes are hot reloaded by all four clients at once (verifying metro and webpack builds are OK to run at once)

PR Review Checklist

PR Author Checklist

PR Reviewer Checklist

The reviewer will copy/paste it into a new comment and complete it after the author checklist is completed

QA Steps

No QA, internally tested by me and @tgolen

Screenshots

Web

Desktop

iOS

Android

melvin-bot · 2022-10-13T17:21:51Z

npm has a package.json file and a package-lock.json file. It seems you updated one without the other, which is usually a sign of a mistake. If you are updating a package make sure that you update the version in package.json then run npm install

melvin-bot · 2022-10-13T17:44:21Z

npm has a package.json file and a package-lock.json file. It seems you updated one without the other, which is usually a sign of a mistake. If you are updating a package make sure that you update the version in package.json then run npm install

melvin-bot · 2022-10-13T17:47:29Z

npm has a package.json file and a package-lock.json file. It seems you updated one without the other, which is usually a sign of a mistake. If you are updating a package make sure that you update the version in package.json then run npm install

AndrewGable · 2022-10-13T17:48:19Z

There are changes to package.json but there are no changes to package-lock.json.

jasperhuangg · 2022-10-13T19:37:18Z

Having an issue with my iOS simulator:

╰─ pod install

[!] Invalid `Podfile` file: [!] /usr/local/bin/node -e try {console.log(require('@react-native-community/cli').bin);} catch (e) {console.log(require('react-native/cli').bin);}

/Users/jasperhuang/Expensidev/App/node_modules/@react-native-community/cli/build/index.js:156
    cmd.option(opt.name, opt.description ?? '', opt.parse || (val => val), typeof opt.default === 'function' ? opt.default(rest[0]) : opt.default);
                                          ^

SyntaxError: Unexpected token '?'
    at Module._compile (internal/modules/cjs/loader.js:892:18)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:973:10)
    at Module.load (internal/modules/cjs/loader.js:812:32)
    at Function.Module._load (internal/modules/cjs/loader.js:724:14)
    at Module.require (internal/modules/cjs/loader.js:849:19)
    at require (internal/modules/cjs/helpers.js:74:18)
    at Object.<anonymous> (/Users/jasperhuang/Expensidev/App/node_modules/react-native/cli.js:13:11)
    at Module._compile (internal/modules/cjs/loader.js:956:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:973:10)
    at Module.load (internal/modules/cjs/loader.js:812:32)
.

 #  from /Users/jasperhuang/Expensidev/App/ios/Podfile:15
 #  -------------------------------------------
 #
 >    config = use_native_modules!
 #
 #  -------------------------------------------

Looking into this right now, but can't test until I figure this out

parasharrajat · 2022-10-14T19:16:14Z

My android server is not starting cmd npm run android is getting stuck at

AndrewGable · 2022-10-17T17:41:30Z

@parasharrajat - What OS are you running on?

@jasperhuangg - Did you ever resolve your iOS issues? Can you post in Slack and tag me?

parasharrajat · 2022-10-17T18:08:20Z

@parasharrajat - What OS are you running on?

This was Linux. I will try to setup Android on Mac for this PR. Running other WEB | IOS | Desktop for now.

parasharrajat · 2022-10-17T18:50:22Z

Ok, this is working fine but the Android build is not starting on Linux.

AndrewGable · 2022-10-17T18:54:46Z

And Android starts on main in your environment, correct? e.g. without the port flag? Can you get anymore flags via a verbose flag?

parasharrajat · 2022-10-17T19:26:20Z

On Linux android works this way.

I start a metro server in a terminal react-native start or npm run start.
Then I run npm run android in another tab.

so, I think the issue is that with port option npm run android is not able to find the metro server which means the metro server may be starting on a different port.

AndrewGable · 2022-10-17T19:30:35Z

Ok so since I am not a Linux user I'd love your suggestions on how to fix this. Why doesn't npm run android start a metro server like it does on MacOS? In order to fix this you can run npm run start -- --port=8083 then npm run android, but that flag is going to be hard to remember to run.

parasharrajat · 2022-10-17T19:54:27Z

Yeah, you are correct. It should start the metro server automatically. I will try to find a solution to that. But I suggest we change the npm run start to npm run start --port=8083 if that is aligned with this PR.

AndrewGable · 2022-10-17T19:59:46Z

I think one issue is that we do not know whether to start the iOS port or Android port for npm run start?

parasharrajat · 2022-10-17T20:02:31Z

Yeah, that's the point. I can't tell if this issue exists on windows as well. But we can make an exception for Linux. I can remember to run the command correctly. I will probably find the solution to the automatic start problem.

parasharrajat

LGTM.

PR Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- iOS / native
- Android / native
- iOS / Safari
- Android / Chrome
- MacOS / Chrome
- MacOS / Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product was added in all src/languages/* files
- I verified any copy / text that was added to the app is correct English and approved by marketing by tagging the marketing team on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

🎀 👀 🎀 C+ reviewed

jasperhuangg

tested and works well 👍

OSBotify · 2022-10-17T22:09:38Z

🚀 Deployed to staging by @jasperhuangg in version: 1.2.17-2 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	failure ❌
🕸 web 🕸	success ✅

OSBotify · 2022-10-19T11:45:26Z

🚀 Deployed to production by @sketchydroide in version: 1.2.17-4 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

parasharrajat · 2022-11-18T22:48:22Z

Sorry for the delayed ping. I just noticed this was not paid for c+. Following the C+ process, can someone assign a BZ to look into this?
cc: @AndrewGable

AndrewGable · 2022-11-18T22:49:38Z

@Expensify/bug-zero - Can someone handle this payment? Thanks!

puneetlath · 2022-11-19T16:44:52Z

Created an issue for it here: #12881

Add port flag to allow iOS and Android to run simultaneously

6749b38

Change iPad port and remove dead code

8ef1c5b

AndrewGable self-assigned this Oct 13, 2022

AndrewGable changed the title ~~Add port flag to allow iOS and Android to run simultaneously~~ [No QA]Add port flag to allow iOS and Android to run simultaneously Oct 13, 2022

AndrewGable marked this pull request as ready for review October 13, 2022 17:47

AndrewGable requested a review from a team as a code owner October 13, 2022 17:47

melvin-bot Bot requested review from jasperhuangg and parasharrajat and removed request for a team October 13, 2022 17:48

parasharrajat approved these changes Oct 17, 2022

View reviewed changes

jasperhuangg approved these changes Oct 17, 2022

View reviewed changes

jasperhuangg merged commit 4f1e6ec into main Oct 17, 2022

jasperhuangg deleted the andrew-port branch October 17, 2022 20:58

OSBotify mentioned this pull request Oct 17, 2022

Deploy Checklist: New Expensify 2022-10-17 #11920

Closed

10 tasks

mallenexpensify added the Bug Something is broken. Auto assigns a BugZero manager. label Oct 28, 2022

puneetlath mentioned this pull request Nov 19, 2022

[$1000] Payment for C+ on pull/11813 #12881

Closed

amyevans mentioned this pull request Jun 21, 2023

Remove port to fix Flipper #21243

Merged

57 tasks

Uh oh!

Conversation

AndrewGable commented Oct 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Fixed Issues

Tests

PR Review Checklist

PR Author Checklist

PR Reviewer Checklist

QA Steps

Screenshots

Web

Desktop

iOS

Android

Uh oh!

melvin-bot Bot commented Oct 13, 2022

Uh oh!

melvin-bot Bot commented Oct 13, 2022

Uh oh!

melvin-bot Bot commented Oct 13, 2022

Uh oh!

AndrewGable commented Oct 13, 2022

Uh oh!

jasperhuangg commented Oct 13, 2022

Uh oh!

parasharrajat commented Oct 14, 2022

Uh oh!

AndrewGable commented Oct 17, 2022

Uh oh!

parasharrajat commented Oct 17, 2022

Uh oh!

parasharrajat commented Oct 17, 2022

Uh oh!

AndrewGable commented Oct 17, 2022

Uh oh!

parasharrajat commented Oct 17, 2022

Uh oh!

AndrewGable commented Oct 17, 2022

Uh oh!

parasharrajat commented Oct 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AndrewGable commented Oct 17, 2022

Uh oh!

parasharrajat commented Oct 17, 2022

Uh oh!

parasharrajat left a comment

Choose a reason for hiding this comment

PR Reviewer Checklist

Uh oh!

jasperhuangg left a comment

Choose a reason for hiding this comment

Uh oh!

OSBotify commented Oct 17, 2022

Uh oh!

OSBotify commented Oct 19, 2022

Uh oh!

parasharrajat commented Nov 18, 2022

Uh oh!

AndrewGable commented Nov 18, 2022

Uh oh!

puneetlath commented Nov 19, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

AndrewGable commented Oct 13, 2022 •

edited

Loading

parasharrajat commented Oct 17, 2022 •

edited

Loading