feat: add invoking CLI's scripts for launching Metro in run-ios command

[szymonrybczak]

Summary:

Recently @huntieremoved "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:

1. Clone the repository and do all the required steps from the Contributing guide
2. 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

[tido64]

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

[szymonrybczak]

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;
	}

[thymikee]

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

[szymonrybczak]

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

[huntie]

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 react/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

[huntie]

✅ Endorsed!

[huntie]

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]

👋, 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?

[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.

[szymonrybczak]

• 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]

@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]

@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! 🙌🏻

[huntie]

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

[szymonrybczak]

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

[thymikee]

I think we're one small refactor away from merging this. Nice work @szymonrybczak and @huntie!

[aajahid]

Bug - #2219

[aajahid]

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.

[aajahid]

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]

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]

Summary:

Recently @huntieremoved "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:

1. Clone the repository and do all the required steps from the Contributing guide
2. 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

[tido64]

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

[szymonrybczak]

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;
	}

[thymikee]

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

[szymonrybczak]

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

[huntie]

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 react/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

[huntie]

✅ Endorsed!

[huntie]

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]

👋, 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?

[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.

[szymonrybczak]

• 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]

@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]

@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! 🙌🏻

[huntie]

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

[szymonrybczak]

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

[thymikee]

I think we're one small refactor away from merging this. Nice work @szymonrybczak and @huntie!

[aajahid]

Bug - #2219

[aajahid]

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.

[aajahid]

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]

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]

Summary:

Recently @huntieremoved "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:

1. Clone the repository and do all the required steps from the Contributing guide
2. 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

[tido64]

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

[szymonrybczak]

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;
	}

[thymikee]

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

[szymonrybczak]

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

[huntie]

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 react/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

[huntie]

✅ Endorsed!

[huntie]

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]

👋, 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?

[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.

[szymonrybczak]

• 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]

@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]

@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! 🙌🏻

[huntie]

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

[szymonrybczak]

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

[thymikee]

I think we're one small refactor away from merging this. Nice work @szymonrybczak and @huntie!

[aajahid]

Bug - #2219

[aajahid]

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.

[aajahid]

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]

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]

Summary:

Recently @huntieremoved "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:

1. Clone the repository and do all the required steps from the Contributing guide
2. 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

[tido64]

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

[szymonrybczak]

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;
	}

[thymikee]

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

[szymonrybczak]

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

[huntie]

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 react/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

[huntie]

✅ Endorsed!

[huntie]

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]

👋, 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?

[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.

[szymonrybczak]

• 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]

@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]

@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! 🙌🏻

[huntie]

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

[szymonrybczak]

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

[thymikee]

I think we're one small refactor away from merging this. Nice work @szymonrybczak and @huntie!

[aajahid]

Bug - #2219

[aajahid]

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.

[aajahid]

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]

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]

Summary:

Recently @huntieremoved "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:

1. Clone the repository and do all the required steps from the Contributing guide
2. 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

[tido64]

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

[szymonrybczak]

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;
	}

[thymikee]

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

[szymonrybczak]

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

[huntie]

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 react/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

[huntie]

✅ Endorsed!

[huntie]

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]

👋, 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?

[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.

[szymonrybczak]

• 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]

@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]

@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! 🙌🏻

[huntie]

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

[szymonrybczak]

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

[thymikee]

I think we're one small refactor away from merging this. Nice work @szymonrybczak and @huntie!

[aajahid]

Bug - #2219

[aajahid]

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.

[aajahid]

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]

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]

Summary:

Recently @huntieremoved "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:

1. Clone the repository and do all the required steps from the Contributing guide
2. 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

[tido64]

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

[szymonrybczak]

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;
	}

[thymikee]

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

[szymonrybczak]

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

[huntie]

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 react/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

[huntie]

✅ Endorsed!

[huntie]

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]

👋, 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?

[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.

[szymonrybczak]

• 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]

@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]

@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! 🙌🏻

[huntie]

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

[szymonrybczak]

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

[thymikee]

I think we're one small refactor away from merging this. Nice work @szymonrybczak and @huntie!

[aajahid]

Bug - #2219

[aajahid]

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.

[aajahid]

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]

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]

Summary:

Recently @huntieremoved "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:

1. Clone the repository and do all the required steps from the Contributing guide
2. 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

[tido64]

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

[szymonrybczak]

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;
	}

[thymikee]

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

[szymonrybczak]

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

[huntie]

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 react/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

[huntie]

✅ Endorsed!

[huntie]

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]

👋, 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?

[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.

[szymonrybczak]

• 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]

@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]

@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! 🙌🏻

[huntie]

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

[szymonrybczak]

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

[thymikee]

I think we're one small refactor away from merging this. Nice work @szymonrybczak and @huntie!

[aajahid]

Bug - #2219

[aajahid]

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.

[aajahid]

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]

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]

Summary:

Recently @huntieremoved "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:

1. Clone the repository and do all the required steps from the Contributing guide
2. 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

[tido64]

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

[szymonrybczak]

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;
	}

[thymikee]

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

[szymonrybczak]

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

[huntie]

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 react/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

[huntie]

✅ Endorsed!

[huntie]

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]

👋, 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?

[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.

[szymonrybczak]

• 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]

@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]

@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! 🙌🏻

[huntie]

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

[szymonrybczak]

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

[thymikee]

I think we're one small refactor away from merging this. Nice work @szymonrybczak and @huntie!

[aajahid]

Bug - #2219

[aajahid]

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.

[aajahid]

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]

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