feat: add invoking CLI's scripts for launching Metro in `run-ios` command by szymonrybczak · Pull Request #2021 · react-native-community/cli

szymonrybczak · 2023-07-18T10:20:08Z

Summary:

Recently @huntie removed "Start packager" phase from template, which means that right now when running run-ios command, packager won't be started. I synced implementations between platforms (run-android command was using CLI's script for some time).
This change also fixes problem with starting Metro in monorepos setups (see #1799).

Test Plan:

Clone the repository and do all the required steps from the Contributing guide
Run this command:

run-ios - Should start Metro from script located in node_modules/.bin/launchPackager.command ✅

TODO:

Windows support

Checklist

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

packages/cli-platform-ios/src/commands/buildIOS/index.ts

tido64 · 2023-07-18T11:22:59Z

packages/cli-platform-ios/src/commands/runIOS/index.ts

    } "${chalk.bold(xcodeProject.name)}"`,
  );

+  await runPackager(


We should be able to disable this with --no-packager.

Unrelated: I was actually hoping we could remove the packager from the run-android command instead :P

We should be able to disable this with --no-packager.

We're already able to this :) See here:

cli/packages/cli-platform-ios/src/commands/runIOS/index.ts

Lines 609 to 612 in ba362e6

{

name: '--no-packager',

description: 'Do not launch packager while building',

},

And in function body I check for this:

cli/packages/cli-plugin-metro/src/commands/start/runPackager.ts

Lines 11 to 13 in b374145

if (!packager) {

return;

}

Question is, maybe we should consider disabling running packager by default? I think this would require a poll on how people use it in the wild. I myself run the server myself in a terminal tab that suits me. But I've also seen devs relying on automatic opening of dev server and not sure if they would even realize it's missing

Other options that comes to my mind is to run the packager in the same window where we start run-ios/android command.

I think this would require a poll on how people use it in the wild

FYI I'm up for this, and/or making the most informed decision. Originally I was with @tido64 in thinking we'd drop the behaviour on Android to align. But @szymonrybczak's changes, with an opt-out flag, look pretty useful to me.

While this item of feedback is still unresolved, I'd like to push for this PR to be merged and released anyway, since it is a direct dependency for facebook/react-native#38944 and incoming Debugging functionality in React Native. These are all alpha releases, and we can follow up on enhancing this behaviour in a follow-up PR.|

cc @thymikee

packages/cli-plugin-metro/src/commands/start/runPackager.ts

packages/cli-platform-ios/src/commands/buildIOS/buildProject.ts

packages/cli-plugin-metro/src/commands/start/runPackager.ts

packages/cli-platform-ios/README.md

huntie · 2023-07-24T13:02:49Z

✅ Endorsed!

packages/cli-plugin-metro/src/commands/start/runPackager.ts

packages/cli-plugin-metro/src/commands/runPackager/index.ts

packages/cli-doctor/src/tools/healthchecks/packager.ts

packages/cli-plugin-metro/src/commands/start/startServerInNewWindow.ts

huntie · 2023-07-28T15:39:53Z

packages/cli-plugin-metro/src/commands/runPackager/index.ts

+      name: '--terminal <string>',
+      description:
+        'Launches packager in a new window using the specified terminal path.',


Main feedback ⬇️

Interesting, can we merge this all into the start command — aligning with your proposal in react-native-community/discussions-and-proposals#613? Specifically:

The improved error message behaviour when start (RN CLI start) is called and something is already on the configured Metro port.

react-native start --terminal switches modes to integrate the startServerInNewWindow.js behaviour.

Then, we only expose startCommand and it can be used by doctor, run-android, run-ios.

cc @thymikee Does this seem feasible?

szymonrybczak · 2023-08-01T11:57:59Z

👋, update from my side. I implemented whole feedback, and I added features discussed under this RFC. I changed implementation of handling keystrokes. Before it wasn't possible to pause interactivity (to show the prompt), but right now it was possible. Without this change, after showing prompt in watch mode the user wouldn't be able to handle any keystroke.

Rigth now flow looks like now:

run-android starts bundler via CLI (node cli/path/ start command)
run-ios starts bundler via CLI (node /cli/path/ start command)

Note

When starting Android or iOS app from watchMode it runs also run-android and run-ios command so it would run again start command, but right now CLI is checking against port is busy or not so user won't see nothing.

Added interactive part 👀

When starting bundler and port is busy CLI will ask whether to terminate process that is running on port user requested. If user wants to kill process, CLI will kill process and starts CLI at request port.

CleanShot.2023-08-01.at.13.39.09.mp4

When user don't want to kill proces, CLI will find the next free port and will ask user whether to start bundler at found port.

CleanShot.2023-08-01.at.13.41.13.mp4

Also added log to inform user about on which port Metro bundler is running:

Important

This change is backward compatible, previously status endpoint would give us packager-status:running response, but right now we're giving {root: "/path/to/project", status: "running}, and I adjusted logic in isPackagerRunning function. What I didn't check but is really important is to check if React Native is somehow related to this response?

packages/cli-platform-ios/src/commands/runIOS/index.ts

packages/cli-plugin-metro/src/commands/start/runServer.ts

packages/cli-tools/src/getPidFromPort.ts

huntie

This is awesome work 💯

High-level feedback

1/ We can probably achieve a better organisation of responsibilities here.
- Maybe --terminal on the start command is the wrong place to do full-on process management (my bad). It would be really good to keep cli-plugin-metro's responsibilities as clean as possible.
- Therefore:
  - cli-plugin-metro: Let's reduce the start command to starting the server only, and erroring generically to say "Could not start dev server: The specified port was taken".
  - cli-plugin-android/ios Let's do process detection here, and move the background terminal creation logic here (likely shared via cli-tools) also. Therefore it's these commands which will wrap start, which doesn't need to know where it's running.
2/ Being able to kill another process is probably not surface area we want to add to CLI (being mindful of when we introduce user expectations). For the run-android/run-ios commands, bailing from starting Metro when the port is taken (with an informational log message) is most likely adequate.
3/ I'd suggest separating the improved KeyPressHandler logic into a new PR — potentially this should live in cli-tools.

packages/cli-plugin-metro/src/commands/start/index.ts

packages/cli-plugin-metro/src/commands/start/startServerInNewWindow.ts

packages/cli-plugin-metro/src/commands/start/watchMode.ts

packages/cli-plugin-metro/src/commands/start/runServer.ts

packages/cli-plugin-metro/src/tools/getProcessIdFromPort.ts

packages/cli-plugin-metro/src/tools/getNextPort.ts

szymonrybczak · 2023-08-01T15:52:53Z

Therefore:

cli-plugin-metro: Let's reduce the start command to starting the server only, and erroring generically to say "Could not start dev server: The specified port was taken".

cli-plugin-android/ios Let's do process detection here, and move the background terminal creation logic here (likely shared via cli-tools) also. Therefore it's these commands which will wrap start, which doesn't need to know where it's running.

Hm, I'm thinking about this solution, and it is slightly better that doing this whole logic behind --terminal option, but do to the second part of this (move this to cli-plugin-android/ios) we would need to use startServerInNewWindow, which of course we can do, by simply moving this function to a cli-tools package. For this we also will need to move relevant scripts, as it wouldn't make sense for them to live in cli-plugin-metro and to access this from cli-tools, and I'm not sure if this would be doable if we're migrating this part of codebase to Core.
And what I'm wondering most - is cli-tools actually good place to keep launchPackager.command, launchPackager.bat? cc. @thymikee

2/ Being able to kill another process is probably not surface area we want to add to CLI (being mindful of when we introduce user expectations). For the run-android/run-ios commands, bailing from starting Metro when the port is taken (with an informational log message) is most likely adequate.

Okay, let's leave just prompt with proposition port change.

3/ I'd suggest separating the improved KeyPressHandler logic into a new PR — potentially this should live in cli-tools.

#2041 - I will rebase on top of this PR once will be merged 👍

huntie · 2023-08-01T16:19:12Z

@szymonrybczak

For this we also will need to move relevant scripts, as it wouldn't make sense for them to live in cli-plugin-metro and to access this from cli-tools, and I'm not sure if this would be doable if we're migrating this part of codebase to Core.

Sure, let's move as much logic as necessary into cli-tools, where necessary to support run-android/run-ios and as not needed by cli-plugin-metro.

is cli-tools actually good place to keep launchPackager.command, launchPackager.bat?

I think yes for now — or perhaps in the root cli package.

szymonrybczak · 2023-08-02T13:55:46Z

@huntie So I moved some logic into run-ios/android and also I move relevant scripts to cli-tools package 👍

huntie

Looks good to me. Might be some tidying we can do in future, but I'm keen to unblock the move of cli-plugin-metro. Huge thanks for this! 🙌🏻

packages/cli-tools/src/launchPackager.bat

packages/cli-plugin-metro/src/commands/start/runServer.ts

packages/cli-tools/src/startServerInNewWindow.ts

huntie · 2023-08-04T12:28:17Z

packages/cli-plugin-metro/src/commands/start/watchMode.ts

 import chalk from 'chalk';
 import {Config} from '@react-native-community/cli-types';
+import {KeyPressHandler} from '../../tools/KeyPressHandler';
+import {addInteractionListener} from '@react-native-community/cli-tools';


nit: Merge with existing '@react-native-community/cli-tools' import.

yeah, I will do it after #2041 will be merged 👍

aajahid

Bug - #2219

aajahid · 2023-12-24T09:12:30Z

packages/cli-tools/src/startServerInNewWindow.ts

  reactNativePath: string,
+  terminal?: string,
 ) {
+  if (!terminal) {


This check is preventing android build on windows. On windows - so far we did not need any terminal value. It always used to fall back to default cmd.exe ( line no 106 )

In this case I think the proper fix should be fixing the getDefaultUserTerminal which currently doesn't have any default terminal for windows.

Or only apply this check on platforms other than windows.

aajahid

Reason of another issue

This works for me. However it only opens a new window and doesn't automatically run andoid, it acts as if you've run react-native start only

#2219

aajahid · 2023-12-24T12:03:49Z

packages/cli-platform-android/src/commands/runAndroid/index.ts

+  }
+
+  if (packager) {
+    await startServerInNewWindow(


await causes the CLI to hang indefinitely. Must be called without await.
Check existing comment at https://github.com/szymonrybczak/cli/blob/732308dfe47cf05d7f5c869fcef08a8fe96ecc7c/packages/cli-tools/src/startServerInNewWindow.ts#L105

szymonrybczak requested a review from huntie July 18, 2023 10:20

szymonrybczak requested review from adamTrz and thymikee as code owners July 18, 2023 10:20

github-actions bot added the feature label Jul 18, 2023

adamTrz reviewed Jul 18, 2023

View reviewed changes

packages/cli-platform-ios/src/commands/buildIOS/index.ts Outdated Show resolved Hide resolved

tido64 reviewed Jul 18, 2023

View reviewed changes

packages/cli-plugin-metro/src/commands/start/runPackager.ts Outdated Show resolved Hide resolved

github-actions bot added the docs Documentation change label Jul 18, 2023

szymonrybczak changed the title ~~feat: add invoking CLI's scripts for launching Metro in run/build-ios commands~~ feat: add invoking CLI's scripts for launching Metro in run-ios commands Jul 18, 2023

szymonrybczak changed the title ~~feat: add invoking CLI's scripts for launching Metro in run-ios commands~~ feat: add invoking CLI's scripts for launching Metro in run-ios command Jul 18, 2023

thymikee reviewed Jul 18, 2023

View reviewed changes

packages/cli-platform-ios/src/commands/buildIOS/buildProject.ts Outdated Show resolved Hide resolved

thymikee reviewed Jul 18, 2023

View reviewed changes

packages/cli-plugin-metro/src/commands/start/runPackager.ts Outdated Show resolved Hide resolved

thymikee reviewed Jul 18, 2023

View reviewed changes

packages/cli-platform-ios/README.md Show resolved Hide resolved

szymonrybczak force-pushed the feat/sync-starting-bundler-implementations branch from e9df890 to 75e3ee1 Compare July 24, 2023 12:01

huntie reviewed Jul 25, 2023

View reviewed changes

packages/cli-plugin-metro/src/commands/start/runPackager.ts Outdated Show resolved Hide resolved

szymonrybczak commented Jul 25, 2023

View reviewed changes

packages/cli-plugin-metro/src/commands/runPackager/index.ts Outdated Show resolved Hide resolved

huntie requested changes Jul 28, 2023

View reviewed changes

szymonrybczak requested a review from huntie August 1, 2023 12:06

TMisiukiewicz reviewed Aug 1, 2023

View reviewed changes

packages/cli-platform-ios/src/commands/runIOS/index.ts Outdated Show resolved Hide resolved

packages/cli-plugin-metro/src/commands/start/runServer.ts Outdated Show resolved Hide resolved

packages/cli-tools/src/getPidFromPort.ts Show resolved Hide resolved

huntie requested changes Aug 1, 2023

View reviewed changes

szymonrybczak force-pushed the feat/sync-starting-bundler-implementations branch 2 times, most recently from 5c3b84b to 691c96a Compare August 2, 2023 14:07

huntie approved these changes Aug 2, 2023

View reviewed changes

packages/cli-tools/src/launchPackager.bat Show resolved Hide resolved

packages/cli-plugin-metro/src/commands/start/runServer.ts Outdated Show resolved Hide resolved

huntie mentioned this pull request Aug 2, 2023

RFC: Improve flow starting Metro react-native-community/discussions-and-proposals#613

Closed

huntie reviewed Aug 4, 2023

View reviewed changes

szymonrybczak mentioned this pull request Aug 14, 2023

fix: change request response in /status endpoint #2056

Merged

2 tasks

szymonrybczak mentioned this pull request Aug 22, 2023

Running an app in debug should not fail due to other apps running packager #1209

Closed

aajahid reviewed Dec 24, 2023

View reviewed changes

aajahid mentioned this pull request Dec 24, 2023

Fix for run-android on windows. #2236

Merged

2 tasks

	{
	name: '--no-packager',
	description: 'Do not launch packager while building',
	},

Conversation

szymonrybczak commented Jul 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary:

Test Plan:

TODO:

Checklist

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

huntie commented Jul 24, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

szymonrybczak commented Aug 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

huntie left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

szymonrybczak commented Aug 1, 2023

Uh oh!

huntie commented Aug 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

szymonrybczak commented Aug 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

huntie left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aajahid left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aajahid left a comment

Choose a reason for hiding this comment

Uh oh!

aajahid Dec 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

szymonrybczak commented Jul 18, 2023 •

edited

Loading

szymonrybczak commented Aug 1, 2023 •

edited

Loading

huntie left a comment •

edited

Loading

huntie commented Aug 1, 2023 •

edited

Loading

szymonrybczak commented Aug 2, 2023 •

edited

Loading

aajahid Dec 24, 2023 •

edited

Loading