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

Fix for run-android on windows. #2236

Merged
merged 2 commits into from Dec 27, 2023

Conversation

aajahid
Copy link
Contributor

@aajahid aajahid commented Dec 24, 2023

Summary:

Fixes #2219

Fixes run-android command on windows.
The issue is reported here - #2219
Bug was introduced on #2021

Issue is - there is no default terminal defined for windows. In the above mentioned PR - terminal param became mandatory. So it was failing.

It also fixes where it tries to run startServerInNewWindow with async call, which causes CLI to hang indefinitely. And the native build never starts.

Test Plan:

24.12.2023_18.49.28_REC.mp4

Checklist

  • Documentation is up to date to reflect these changes.
  • Follows commit message convention described in CONTRIBUTING.md

@@ -61,7 +61,7 @@ async function runAndroid(_argv: Array<string>, config: Config, args: Flags) {
);

if (startPackager) {
await startServerInNewWindow(
startServerInNewWindow(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mind also adding comment why awaiting this function breaks things?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Copy link
Collaborator

@szymonrybczak szymonrybczak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! 🙏

@cortinico
Copy link
Member

Is this related to facebook/react-native#42074 ?

@thymikee thymikee merged commit a36eefc into react-native-community:main Dec 27, 2023
10 checks passed
@szymonrybczak
Copy link
Collaborator

Let's backport this to 0.73 cc @thymikee

thymikee pushed a commit that referenced this pull request Dec 27, 2023
* Fix for `run-android` on windows.

* Comment added explaining why awaiting is not used

---------

Co-authored-by: Abdullah Al Jahid <jahid@chaldal.net>
@sunnylqm
Copy link
Contributor

sunnylqm commented Jan 6, 2024

and we need a cli release first to fix on rn side?

@szymonrybczak
Copy link
Collaborator

and we need a cli release first to fix on rn side?

Yes, that's how it works.

@cortinico
Copy link
Member

Is this related to facebook/react-native#42074 ?

@szymonrybczak was this related to the linked issue? If so, can we make sure we pick it in a release?

@szymonrybczak
Copy link
Collaborator

If so, can we make sure we pick it in a release?

Yes, we'll handle this today.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Windows: Cannot start server in new window because no terminal app was specified
5 participants