-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Remove port to fix Flipper #21243
Remove port to fix Flipper #21243
Conversation
npm has a |
@abdulrahuman5196 Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
npm has a |
This PR updates the scripts, not package(s), so no changes to |
@amyevans Curious question. Does everyone need to reinstall the pods after we push this change? |
As far as I'm aware, no. To be honest I just copied the exact test steps from #11813 to ensure we don't have a regression for the scenario the PR intended to fix. I think removing and reinstalling pods was just an extra precaution to ensure a fresh slate. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OLD Reviewer ChecklistOLD
Screenshots/VideosWebMobile Web - ChromeMobile Web - SafariDesktopiOSAndroid |
I'm not familiar with how the port options affect Flipper, but it does make sense if Flipper is automatically choosing to look at 8081. The port options in the linked shell script are important to keep the --port flag working, and 8081 should just be the default if the script is configured correctly. I only got involved in this conversation because I was wondering why there were hardcoded ports for the mobile apps in the package.json (which then don't affect the builds if you run them from XCode or Android Studio). My best guess is that it helped keep consistent ports for debugging tools? Web and desktop seem to automatically pick a free port, but pick 8080, then 8081, which could cause inconsistent results if a consistent port is needed and a mobile build might already be using 8081. |
@amyevans I found a issue while running this, But its working the other way. If iOS is ran first and then web, web choose 8082 port automatically and starts working. This comment could be related - #21243 (comment) |
@amyevans did you get a chance to look into @abdulrahuman5196's comment above? |
I haven't yet 😅. It's on my list for the next day or two but need to clear out higher priority items first |
Np! Thanks for the update! |
I didn't hit that error, although web took 8080 for me (and pretty much always does). I tried hardcoding |
If web was started first yes, it will take 8080. Desktop would take 8080
This also might not work since 8081 is already taken.
Maybe that could work. Need to test it. But will it hot reload all platforms if required? I think this also could work, but have to anyways check. |
Another suggestion: because adjusting ports for React Native everywhere one might build is difficult, and many tools may expect 8081, update the port-finding logic used by web and desktop to exclude 8081 from the list of options. What I thought we could do The problem |
What's the plan here? Are we still exploring other solutions? |
Yeah, still exploring solutions. Looking at the docs for node-portfinder, it says
And we are already utilizing this to pass
I'll spend some time testing that though. |
Yeah, I'm concerned it's very possible some tooling might expect 8080 for web/desktop when they're launched in isolation since that's been the default for a while, and is typically the port used for similar web apps. But I'm not sure. |
Okay I pushed that change to the PR since it's testing well for me. Confirmed that hot reloading, Flipper, Storybook, and the help docs site work... what else would be good to check? 🤔 (I definitely share the concern about there being some assumptions made somewhere that the port is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM and works well. We should probably announce this to the team so they are aware of the changes, since ports can change and accessing the App would require a change in the url
@abdulrahuman5196 mind giving it another look and finishing up the checklist? 🙏 |
Oh sorry. I was not aware of this PR being ready for review. My bad. Will check and update within a day. |
No worries! And thanks! |
@amyevans I tested the follows and hot reload and others are working properly.
Edit: My bad. That's the behaviour now as well. |
Reviewer Checklist
Screenshots/VideosWebScreen.Recording.2023-07-23.at.5.51.35.PM.movScreen.Recording.2023-07-23.at.5.52.30.PM.movMobile Web - ChromeSame as Web Mobile Web - SafariSame as Web DesktopSame as Web iOSSame as Web AndroidSame as Web |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes looks good and works well. Reviewers checklist is also complete.
All yours.
🎀 👀 🎀
C+ Reviewed
@cristipaval Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
Announced the change in Slack: https://expensify.slack.com/archives/C01GTK53T8Q/p1690224959854549 |
🚀 Deployed to staging by https://github.com/cristipaval in version: 1.3.45-0 🚀
|
🚀 Deployed to production by https://github.com/mountiny in version: 1.3.45-7 🚀
|
@cristipaval @luacmartins @abdulrahuman5196 @amyevans It seems this PR introduced a regression fixed here - you have missed two other places where the port should have been changed. |
Details
Removes the use of a custom port, which was breaking Flipper.
Note: the port was originally introduced in #11813 because it was believed to be the reason iOS and Android couldn't run simultaneously, but there's discussion here and here that that's not the root cause. The test steps ensure we don't introduce a regression related to running platforms simultaneously.
cc @AndrewGable @luacmartins
Fixed Issues
$ #21239
Tests
rm -rf ios/Pods
)cd ios && pod install
)Offline tests
N/A
QA Steps
No QA, this affects dev only
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Web
flipper-test.mov
Mobile Web - Chrome
N/A
Mobile Web - Safari
N/A
Desktop
flipper-test.mov
iOS
flipper-test.mov
Android
flipper-test.mov