-
Notifications
You must be signed in to change notification settings - Fork 905
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add invoking CLI's scripts for launching Metro in run-ios
command
#2021
feat: add invoking CLI's scripts for launching Metro in run-ios
command
#2021
Conversation
@@ -117,6 +118,14 @@ async function runIOS(_: Array<string>, ctx: Config, args: FlagsT) { | |||
} "${chalk.bold(xcodeProject.name)}"`, | |||
); | |||
|
|||
await runPackager( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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; | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other options that comes to my mind is to run the packager in the same window where we start run-ios/android
command.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
run/build-ios
commandsrun-ios
commands
run-ios
commandsrun-ios
command
packages/cli-platform-ios/src/commands/buildIOS/buildProject.ts
Outdated
Show resolved
Hide resolved
e9df890
to
75e3ee1
Compare
✅ Endorsed! |
packages/cli-plugin-metro/src/commands/start/startServerInNewWindow.ts
Outdated
Show resolved
Hide resolved
name: '--terminal <string>', | ||
description: | ||
'Launches packager in a new window using the specified terminal path.', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 CLIstart
) is called and something is already on the configured Metro port. react-native start --terminal
switches modes to integrate thestartServerInNewWindow.js
behaviour.
Then, we only expose startCommand
and it can be used by doctor
, run-android
, run-ios
.
cc @thymikee Does this seem feasible?
👋, 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:
Note When starting Android or iOS app from watchMode it runs also Added interactive part 👀
CleanShot.2023-08-01.at.13.39.09.mp4
CleanShot.2023-08-01.at.13.41.13.mp4Also added log to inform user about on which port Metro bundler is running: Important This change is backward compatible, previously |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is awesome work 💯
High-level feedback
- 1/ We can probably achieve a better organisation of responsibilities here.
- Maybe
--terminal
on thestart
command is the wrong place to do full-on process management (my bad). It would be really good to keepcli-plugin-metro
's responsibilities as clean as possible. - Therefore:
cli-plugin-metro
: Let's reduce thestart
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 viacli-tools
) also. Therefore it's these commands which will wrapstart
, which doesn't need to know where it's running.
- Maybe
- 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 incli-tools
.
packages/cli-plugin-metro/src/commands/start/startServerInNewWindow.ts
Outdated
Show resolved
Hide resolved
Hm, I'm thinking about this solution, and it is slightly better that doing this whole logic behind
Okay, let's leave just prompt with proposition port change.
#2041 - I will rebase on top of this PR once will be merged 👍 |
Sure, let's move as much logic as necessary into
I think yes for now — or perhaps in the root |
@huntie So I moved some logic into |
5c3b84b
to
691c96a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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! 🙌🏻
@@ -3,6 +3,11 @@ import {logger, hookStdout} from '@react-native-community/cli-tools'; | |||
import execa from 'execa'; | |||
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'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Merge with existing '@react-native-community/cli-tools'
import.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, I will do it after #2041 will be merged 👍
Summary: Update `cli-commands` package to reflect latest source `react-native-community/cli-plugin-metro` changes. - react-native-community/cli#2043 - react-native-community/cli#2021 - react-native-community/cli#2024 Changelog: [Internal] Differential Revision: D48311214 fbshipit-source-id: 2fe6d8301990982e589f54c7c75fb7dec008c2f6
Summary: Update `cli-commands` package to reflect latest source `react-native-community/cli-plugin-metro` changes. - react-native-community/cli#2043 - react-native-community/cli#2021 - react-native-community/cli#2024 Changelog: [Internal] Differential Revision: D48311214 fbshipit-source-id: c5c9fa40729de2c84517d3e1c3b4fdae71ae7bab
Summary: Pull Request resolved: facebook#38944 - Update `cli-commands` package to reflect latest source `react-native-community/cli-plugin-metro` changes. - react-native-community/cli#2021 - react-native-community/cli#2024 - react-native-community/cli#2043 - react-native-community/cli#2047 ### To do WARNING: ~~This PR is non-functional until the next CLI alpha is published and bumped in `package.json` — since it depends on corresponding new APIs in `react-native-community/cli-tools` (react-native-community/cli#2021). This package (and the upcoming work which integrates it) has been tested against locally linked copies of latest CLI.~~ - [x] Bump CLI dependencies when next alpha published. - [x] Ensure Metro bump from `0.77.0` to `0.78.0` is consistently applied with this. Changelog: [Internal] Reviewed By: motiz88 Differential Revision: D48066179 fbshipit-source-id: bb90bf5776facc8b39050e1555e0b7009d046d99
Summary: Update `cli-commands` package to reflect latest source `react-native-community/cli-plugin-metro` changes. - react-native-community/cli#2043 - react-native-community/cli#2021 - react-native-community/cli#2024 Changelog: [Internal] Differential Revision: D48311214 fbshipit-source-id: 1fa5f775972165541713a0a1c9e7702dba14b485
Summary: Pull Request resolved: facebook#38944 - Update `cli-commands` package to reflect latest source `react-native-community/cli-plugin-metro` changes. - react-native-community/cli#2021 - react-native-community/cli#2024 - react-native-community/cli#2043 - react-native-community/cli#2047 ### To do WARNING: ~~This PR is non-functional until the next CLI alpha is published and bumped in `package.json` — since it depends on corresponding new APIs in `react-native-community/cli-tools` (react-native-community/cli#2021). This package (and the upcoming work which integrates it) has been tested against locally linked copies of latest CLI.~~ - [x] Bump CLI dependencies when next alpha published. - [x] Ensure Metro bump from `0.77.0` to `0.78.0` is consistently applied with this. Changelog: [Internal] Reviewed By: motiz88 Differential Revision: D48066179 fbshipit-source-id: 32aee8890db0c65791fe640c8993d06fc200f4ff
Summary: Update `cli-commands` package to reflect latest source `react-native-community/cli-plugin-metro` changes. - react-native-community/cli#2043 - react-native-community/cli#2021 - react-native-community/cli#2024 Changelog: [Internal] Differential Revision: D48311214 fbshipit-source-id: 26aaee808bf815ec2f48b936d4c9883bcc6026ef
Summary: Update `cli-commands` package to reflect latest source `react-native-community/cli-plugin-metro` changes. - react-native-community/cli#2043 - react-native-community/cli#2021 - react-native-community/cli#2024 Changelog: [Internal] Differential Revision: D48311214 fbshipit-source-id: 986264a7cc23f3e6adee05781946d57d0c579081
Summary: Update `cli-commands` package to reflect latest source `react-native-community/cli-plugin-metro` changes. - react-native-community/cli#2043 - react-native-community/cli#2021 - react-native-community/cli#2024 Changelog: [Internal] Differential Revision: D48311214 fbshipit-source-id: c415984bbac10cc4b4e0c6031d80eb0f2db95eb0
Summary: Pull Request resolved: facebook#38944 - Update `cli-commands` package to reflect latest source `react-native-community/cli-plugin-metro` changes. - react-native-community/cli#2021 - react-native-community/cli#2024 - react-native-community/cli#2043 - react-native-community/cli#2047 ### To do WARNING: ~~This PR is non-functional until the next CLI alpha is published and bumped in `package.json` — since it depends on corresponding new APIs in `react-native-community/cli-tools` (react-native-community/cli#2021). This package (and the upcoming work which integrates it) has been tested against locally linked copies of latest CLI.~~ - [x] Bump CLI dependencies when next alpha published. - [x] Ensure Metro bump from `0.77.0` to `0.78.0` is consistently applied with this. Changelog: [Internal] Reviewed By: motiz88 Differential Revision: D48066179 fbshipit-source-id: d08f0f8c3bd10f4de96b545b8ded7a4b22e6ae27
Summary: Update `cli-commands` package to reflect latest source `react-native-community/cli-plugin-metro` changes. - react-native-community/cli#2043 - react-native-community/cli#2021 - react-native-community/cli#2024 Changelog: [Internal] Differential Revision: D48311214 fbshipit-source-id: d867f1295f5c36ff96c4a595dd5833c0ee5769f5
Summary: Update `cli-commands` package to reflect latest source `react-native-community/cli-plugin-metro` changes. - react-native-community/cli#2043 - react-native-community/cli#2021 - react-native-community/cli#2024 Changelog: [Internal] Differential Revision: D48311214 fbshipit-source-id: 457644ba45c12be8fc22587b746e924dd8bf234a
Summary: Update `cli-commands` package to reflect latest source `react-native-community/cli-plugin-metro` changes. - react-native-community/cli#2043 - react-native-community/cli#2021 - react-native-community/cli#2024 Changelog: [Internal] Differential Revision: D48311214 fbshipit-source-id: cf74bc09f5a3afc272979e2cdcbe6aa695cb80fd
Summary: Pull Request resolved: facebook#38944 - Update `cli-commands` package to reflect latest source `react-native-community/cli-plugin-metro` changes. - react-native-community/cli#2021 - react-native-community/cli#2024 - react-native-community/cli#2043 - react-native-community/cli#2047 ### To do WARNING: ~~This PR is non-functional until the next CLI alpha is published and bumped in `package.json` — since it depends on corresponding new APIs in `react-native-community/cli-tools` (react-native-community/cli#2021). This package (and the upcoming work which integrates it) has been tested against locally linked copies of latest CLI.~~ - [x] Bump CLI dependencies when next alpha published. - [x] Ensure Metro bump from `0.77.0` to `0.78.0` is consistently applied with this. Changelog: [Internal] Reviewed By: motiz88 Differential Revision: D48066179 fbshipit-source-id: f83828efe2b6bb8da1bb2e11c523f71da601e46f
Summary: Update `cli-commands` package to reflect latest source `react-native-community/cli-plugin-metro` changes. - react-native-community/cli#2043 - react-native-community/cli#2021 - react-native-community/cli#2024 Changelog: [Internal] Differential Revision: D48311214 fbshipit-source-id: 4c339af9715c80fa45ed4348564461c5703530c2
Summary: Update `cli-commands` package to reflect latest source `react-native-community/cli-plugin-metro` changes. - react-native-community/cli#2043 - react-native-community/cli#2021 - react-native-community/cli#2024 Changelog: [Internal] Differential Revision: D48311214 fbshipit-source-id: 122353191146a84284eb3d821cdc2e327b57e902
Summary: Update `cli-commands` package to reflect latest source `react-native-community/cli-plugin-metro` changes. - react-native-community/cli#2043 - react-native-community/cli#2021 - react-native-community/cli#2024 Changelog: [Internal] Differential Revision: D48311214 fbshipit-source-id: ab3de8428423bfe3b8c63055662a5ba91dcf2236
Summary: Update `cli-commands` package to reflect latest source `react-native-community/cli-plugin-metro` changes. - react-native-community/cli#2043 - react-native-community/cli#2021 - react-native-community/cli#2024 Changelog: [Internal] Differential Revision: D48311214 fbshipit-source-id: 091047372137e4dfaead6221544d47872da5a878
Summary: Update `cli-commands` package to reflect latest source `react-native-community/cli-plugin-metro` changes. - react-native-community/cli#2043 - react-native-community/cli#2021 - react-native-community/cli#2024 Changelog: [Internal] Differential Revision: D48311214 fbshipit-source-id: 36c63f21dded683ab5b6f1f8205b8e0529e83d9c
Summary: Pull Request resolved: facebook#38944 - Update `cli-commands` package to reflect latest source `react-native-community/cli-plugin-metro` changes. - react-native-community/cli#2021 - react-native-community/cli#2024 - react-native-community/cli#2043 - react-native-community/cli#2047 ### To do WARNING: ~~This PR is non-functional until the next CLI alpha is published and bumped in `package.json` — since it depends on corresponding new APIs in `react-native-community/cli-tools` (react-native-community/cli#2021). This package (and the upcoming work which integrates it) has been tested against locally linked copies of latest CLI.~~ - [x] Bump CLI dependencies when next alpha published. - [x] Ensure Metro bump from `0.77.0` to `0.78.0` is consistently applied with this. Changelog: [Internal] Reviewed By: motiz88 Differential Revision: D48066179 fbshipit-source-id: 101b7c9e529105a89d4ed68616432e3899c6c817
Summary: Update `cli-commands` package to reflect latest source `react-native-community/cli-plugin-metro` changes. - react-native-community/cli#2043 - react-native-community/cli#2021 - react-native-community/cli#2024 Changelog: [Internal] Differential Revision: D48311214 fbshipit-source-id: 0fea3cba2fb4f7300c2ddde5b13d4e4a4ad2733b
Summary: Update `cli-commands` package to reflect latest source `react-native-community/cli-plugin-metro` changes. - react-native-community/cli#2043 - react-native-community/cli#2021 - react-native-community/cli#2024 Changelog: [Internal] Differential Revision: D48311214 fbshipit-source-id: 4d8c3d9cb98a300587b98c3d81d07f544ffa314e
Summary: Pull Request resolved: facebook#38944 - Update `cli-commands` package to reflect latest source `react-native-community/cli-plugin-metro` changes. - react-native-community/cli#2021 - react-native-community/cli#2024 - react-native-community/cli#2043 - react-native-community/cli#2047 ### To do WARNING: ~~This PR is non-functional until the next CLI alpha is published and bumped in `package.json` — since it depends on corresponding new APIs in `react-native-community/cli-tools` (react-native-community/cli#2021). This package (and the upcoming work which integrates it) has been tested against locally linked copies of latest CLI.~~ - [x] Bump CLI dependencies when next alpha published. - [x] Ensure Metro bump from `0.77.0` to `0.78.0` is consistently applied with this. Changelog: [Internal] Reviewed By: motiz88 Differential Revision: D48066179 fbshipit-source-id: 393a6ebb06be94fb331908a47c8d409f64e63f77
Summary: Update `cli-commands` package to reflect latest source `react-native-community/cli-plugin-metro` changes. - react-native-community/cli#2043 - react-native-community/cli#2021 - react-native-community/cli#2024 Changelog: [Internal] Differential Revision: D48311214 fbshipit-source-id: 3f1b607cd756c8a20c41cdb7a49ba79dfa173821
Summary: Pull Request resolved: #38944 - Update `cli-commands` package to reflect latest source `react-native-community/cli-plugin-metro` changes. - react-native-community/cli#2021 - react-native-community/cli#2024 - react-native-community/cli#2043 - react-native-community/cli#2047 ### To do WARNING: ~~This PR is non-functional until the next CLI alpha is published and bumped in `package.json` — since it depends on corresponding new APIs in `react-native-community/cli-tools` (react-native-community/cli#2021). This package (and the upcoming work which integrates it) has been tested against locally linked copies of latest CLI.~~ - [x] Bump CLI dependencies when next alpha published. - [x] Ensure Metro bump from `0.77.0` to `0.78.0` is consistently applied with this. Changelog: [Internal] Reviewed By: motiz88 Differential Revision: D48066179 fbshipit-source-id: b3dc8891cf33e537788f942dcaddff4d2f11a31f
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug - #2219
) { | ||
if (!terminal) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or only apply this check on platforms other than windows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
} | ||
|
||
if (packager) { | ||
await startServerInNewWindow( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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:
run-ios
- Should start Metro from script located innode_modules/.bin/launchPackager.command
✅TODO:
Checklist