Skip to content

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


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! 🙏

@thymikee thymikee requested a review from acoates-ms December 27, 2023 15:53
@cortinico
Copy link
Member

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

@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 <[email protected]>
@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