Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: open Metro in the correct terminal #310

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
3787169
Put `isPackagerRunning` function on `cli-tools` to share between iOS …
lucasbento Apr 11, 2019
8c7fb99
Fix `terminal` argument not working when not being provided
lucasbento Apr 11, 2019
fd41e95
Fallback to the default terminal of the machine when starting the pac…
lucasbento Apr 11, 2019
27d0792
Delete `isPackagerRunning` as it was moved to `tools`
lucasbento Apr 11, 2019
fb1b91b
Add `terminal` argument to `run-ios`
lucasbento Apr 11, 2019
9a18d84
Launch Metro from within the `runIOS` command
lucasbento Apr 11, 2019
753b606
Remove code & add `—terminal` argument
lucasbento Apr 12, 2019
ac9b684
Try using `REACT_TERMINAL` before the default terminal
lucasbento Apr 12, 2019
119900a
Put `isPackagerRunning` function on `cli-tools` to share between iOS …
lucasbento Apr 11, 2019
485e6fb
Fix `terminal` argument not working when not being provided
lucasbento Apr 11, 2019
4249a49
Fallback to the default terminal of the machine when starting the pac…
lucasbento Apr 11, 2019
29edba4
Delete `isPackagerRunning` as it was moved to `tools`
lucasbento Apr 11, 2019
3c1500a
Add `terminal` argument to `run-ios`
lucasbento Apr 11, 2019
97263a7
Launch Metro from within the `runIOS` command
lucasbento Apr 11, 2019
9f54934
Remove code & add `—terminal` argument
lucasbento Apr 12, 2019
e273506
Try using `REACT_TERMINAL` before the default terminal
lucasbento Apr 12, 2019
5577cc3
Add tool function to get the default user terminal
lucasbento Apr 17, 2019
5c9e283
Fix `terminal` arg type
lucasbento Apr 17, 2019
e397a82
Remove spread and specify entry twice instead
lucasbento Apr 17, 2019
62ca068
Merge branch 'feature/correct-terminal' of https://github.com/lucasbe…
lucasbento Apr 17, 2019
c396b63
Improve `args` being passed through functions
lucasbento Apr 17, 2019
aa6375e
Reduce code duplication
lucasbento Apr 17, 2019
c4bb670
Put `device` and `udid` variable up in the scope
lucasbento Apr 17, 2019
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
15 changes: 7 additions & 8 deletions packages/platform-android/src/commands/runAndroid/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,14 @@ import type {ConfigT} from '../../../../cli/src/tools/config/types.flow';

import adb from './adb';
import runOnAllDevices from './runOnAllDevices';
import isPackagerRunning from './isPackagerRunning';
import tryRunAdbReverse from './tryRunAdbReverse';
import tryLaunchAppOnDevice from './tryLaunchAppOnDevice';
import getAdbPath from './getAdbPath';
import {logger} from '@react-native-community/cli-tools';
import {
isPackagerRunning,
logger,
getDefaultUserTerminal,
} from '@react-native-community/cli-tools';

// Verifies this is an Android project
function checkAndroid(root) {
Expand Down Expand Up @@ -226,11 +229,7 @@ function installAndLaunchOnDevice(
);
}

function startServerInNewWindow(
port,
terminal = process.env.REACT_TERMINAL,
reactNativePath,
) {
function startServerInNewWindow(port, terminal, reactNativePath) {
/**
* Set up OS-specific filenames and commands
*/
Expand Down Expand Up @@ -359,7 +358,7 @@ export default {
command: '--terminal [string]',
description:
'Launches the Metro Bundler in a new window using the specified terminal path.',
default: '',
default: getDefaultUserTerminal(),
},
],
};
134 changes: 47 additions & 87 deletions packages/platform-ios/src/commands/runIOS/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,18 +17,23 @@ import type {ConfigT} from '../../../../cli/src/tools/config/types.flow';
import findXcodeProject from './findXcodeProject';
import parseIOSDevicesList from './parseIOSDevicesList';
import findMatchingSimulator from './findMatchingSimulator';
import {logger, CLIError} from '@react-native-community/cli-tools';
import {
logger,
CLIError,
getDefaultUserTerminal,
} from '@react-native-community/cli-tools';

type FlagsT = {
simulator: string,
configuration: string,
scheme: ?string,
projectPath: string,
device: ?string,
device: ?(string | true),
udid: ?string,
packager: boolean,
verbose: boolean,
port: number,
terminal: ?string,
};

function runIOS(_: Array<string>, ctx: ConfigT, args: FlagsT) {
Expand Down Expand Up @@ -66,65 +71,36 @@ function runIOS(_: Array<string>, ctx: ConfigT, args: FlagsT) {
}),
);

if (args.device) {
const selectedDevice = matchingDevice(devices, args.device);
const device = ((args.device: any): string);
const udid = ((args.udid: any): string);
if (device || udid) {
Copy link
Member

Choose a reason for hiding this comment

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

This is not clean, inside we need to branch 2 more times, which is not really more readable and efficient. What would you tell about this refactoring?

  1. If there's no device nor udid, we run on simulator straight away.
  2. If there are no devices found, we bail early.
  3. We run device/udid branching inside matchingDevice fn (inlining matchingDeviceByUdid into it)
  4. If device matches, run on device
  5. If not, print appropriate messages depending on whether device or udid was passed.
--- a/packages/platform-ios/src/commands/runIOS/index.js
+++ b/packages/platform-ios/src/commands/runIOS/index.js
@@ -64,6 +64,12 @@ function runIOS(_: Array<string>, ctx: ConfigT, args: FlagsT) {
     }`,
   );

+  const {device, udid} = args;
+
+  if (!device && !udid) {
+    return runOnSimulator(xcodeProject, scheme, args);
+  }
+
   const devices = parseIOSDevicesList(
     // $FlowExpectedError https://github.com/facebook/flow/issues/5675
     child_process.execFileSync('xcrun', ['instruments', '-s'], {
@@ -71,33 +77,31 @@ function runIOS(_: Array<string>, ctx: ConfigT, args: FlagsT) {
     }),
   );

-  const device = ((args.device: any): string);
-  const udid = ((args.udid: any): string);
-  if (device || udid) {
-    const selectedDevice = device
-      ? matchingDevice(devices, device)
-      : matchingDeviceByUdid(devices, udid);
+  if (devices.length === 0) {
+    return logger.error('No iOS devices connected.');
+  }
+
+  const selectedDevice = matchingDevice(devices, device, udid);

   if (selectedDevice) {
     return runOnDevice(selectedDevice, scheme, xcodeProject, args);
   }

-    if (devices && devices.length > 0) {
-      const message = device
-        ? `Could not find device with the name: "${device}". Choose one of the following:\n${printFoundDevices(
-            devices,
-          )}`
-        : `Could not find device with the udid: "${udid}". Choose one of the following:\n${printFoundDevices(
-            devices,
-          )}`;
-
-      return logger.error(message);
+  if (device) {
+    return logger.error(
+      `Could not find device with the name: "${String(
+        device,
+      )}". Choose one of the following:\n${printFoundDevices(devices)}`,
+    );
   }

-    return logger.error('No iOS devices connected.');
+  if (udid) {
+    return logger.error(
+      `Could not find device with the udid: "${udid}". Choose one of the following:\n${printFoundDevices(
+        devices,
+      )}`,
+    );
   }
-
-  return runOnSimulator(xcodeProject, scheme, args);
 }

 async function runOnSimulator(xcodeProject, scheme, args: FlagsT) {
@@ -331,7 +335,10 @@ function xcprettyAvailable() {
   return true;
 }

-function matchingDevice(devices, deviceName) {
+function matchingDevice(devices, deviceName, udid) {
+  if (udid) {
+    matchingDeviceByUdid(devices, udid);
+  }

Copy link
Member Author

Choose a reason for hiding this comment

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

I usually prefer to return as early as possible, even if it needs to have negative conditionals, with a quick look I agree with the diff you sent.

@grabbou thoughts on this? Asking because you pointed that you didn't like the conditionals I introduced on previous commits.

Copy link
Member

Choose a reason for hiding this comment

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

I actually disagree this is not clean. The first branch is runOnDevice - you can select device by name or id. The second branch is runOnSimulator.

Anyway, I don't have a strong preference here. Let's merge it as soon as possible as I want to ship it for next RC of React Native.

We can tweak it later.

Copy link
Member

Choose a reason for hiding this comment

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

PS. I like @thymikee structure better, but at the same time, I feel like @lucasbento keeps doing tweaking that is far beyond his PR. I'd just get it merged to not discourage them from further PRs!

Copy link
Member

Choose a reason for hiding this comment

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

@grabbou feel free to merge it as is now. Hopefully @lucasbento will not be discouraged by our nitpicks 😅

const selectedDevice = device
? matchingDevice(devices, device)
: matchingDeviceByUdid(devices, udid);

if (selectedDevice) {
return runOnDevice(
selectedDevice,
scheme,
xcodeProject,
args.configuration,
args.packager,
args.verbose,
args.port,
);
return runOnDevice(selectedDevice, scheme, xcodeProject, args);
}

if (devices && devices.length > 0) {
// $FlowIssue: args.device is defined in this context
logger.error(`Could not find device with the name: "${args.device}".
Choose one of the following:${printFoundDevices(devices)}`);
} else {
logger.error('No iOS devices connected.');
const message = device
? `Could not find device with the name: "${device}". Choose one of the following:\n${printFoundDevices(
devices,
)}`
: `Could not find device with the udid: "${udid}". Choose one of the following:\n${printFoundDevices(
devices,
)}`;

return logger.error(message);
}
} else if (args.udid) {
lucasbento marked this conversation as resolved.
Show resolved Hide resolved
// $FlowIssue: args.udid is defined in this context
return runOnDeviceByUdid(args, scheme, xcodeProject, devices);
}

return runOnSimulator(xcodeProject, args, scheme);
}

function runOnDeviceByUdid(
args: FlagsT & {udid: string},
scheme,
xcodeProject,
devices,
) {
const selectedDevice = matchingDeviceByUdid(devices, args.udid);

if (selectedDevice) {
runOnDevice(
selectedDevice,
scheme,
xcodeProject,
args.configuration,
args.packager,
args.verbose,
args.port,
);
return;
return logger.error('No iOS devices connected.');
}

if (devices && devices.length > 0) {
// $FlowIssue: args.udid is defined in this context
logger.error(`Could not find device with the udid: "${args.udid}".
Choose one of the following:\n${printFoundDevices(devices)}`);
} else {
logger.error('No iOS devices connected.');
}
return runOnSimulator(xcodeProject, scheme, args);
}

async function runOnSimulator(xcodeProject, args, scheme) {
async function runOnSimulator(xcodeProject, scheme, args: FlagsT) {
let simulators;
try {
simulators = JSON.parse(
Expand Down Expand Up @@ -175,10 +151,7 @@ async function runOnSimulator(xcodeProject, args, scheme) {
xcodeProject,
selectedSimulator.udid,
scheme,
args.configuration,
args.packager,
args.verbose,
args.port,
args,
);

const appPath = getBuildPath(args.configuration, appName, false, scheme);
Expand Down Expand Up @@ -213,34 +186,23 @@ async function runOnSimulator(xcodeProject, args, scheme) {
);
}

async function runOnDevice(
selectedDevice,
scheme,
xcodeProject,
configuration,
launchPackager,
verbose,
port,
) {
async function runOnDevice(selectedDevice, scheme, xcodeProject, args: FlagsT) {
const appName = await buildProject(
xcodeProject,
selectedDevice.udid,
scheme,
configuration,
launchPackager,
verbose,
port,
args,
);

const iosDeployInstallArgs = [
'--bundle',
getBuildPath(configuration, appName, true, scheme),
getBuildPath(args.configuration, appName, true, scheme),
'--id',
selectedDevice.udid,
'--justlaunch',
];

logger.info(`installing and launching your app on ${selectedDevice.name}...`);
logger.info(`Installing and launching your app on ${selectedDevice.name}...`);

const iosDeployOutput = child_process.spawnSync(
'ios-deploy',
Expand All @@ -257,21 +219,13 @@ async function runOnDevice(
}
}

function buildProject(
xcodeProject,
udid,
scheme,
configuration,
launchPackager = false,
verbose,
port,
) {
function buildProject(xcodeProject, udid, scheme, args: FlagsT) {
return new Promise((resolve, reject) => {
const xcodebuildArgs = [
xcodeProject.isWorkspace ? '-workspace' : '-project',
xcodeProject.name,
'-configuration',
configuration,
args.configuration,
'-scheme',
scheme,
'-destination',
Expand All @@ -281,7 +235,7 @@ function buildProject(
];
logger.info(`Building using "xcodebuild ${xcodebuildArgs.join(' ')}"`);
let xcpretty;
if (!verbose) {
if (!args.verbose) {
xcpretty =
xcprettyAvailable() &&
child_process.spawn('xcpretty', [], {
Expand All @@ -291,7 +245,7 @@ function buildProject(
const buildProcess = child_process.spawn(
'xcodebuild',
xcodebuildArgs,
getProcessOptions(launchPackager, port),
getProcessOptions(args),
);
let buildOutput = '';
let errorOutput = '';
Expand Down Expand Up @@ -418,15 +372,15 @@ function printFoundDevices(devices) {
return output;
}

function getProcessOptions(launchPackager, port) {
if (launchPackager) {
function getProcessOptions({packager, terminal, port}) {
if (packager) {
return {
env: {...process.env, RCT_METRO_PORT: port},
env: {...process.env, RCT_TERMINAL: terminal, RCT_METRO_PORT: port},
};
}

return {
env: {...process.env, RCT_NO_LAUNCH_PACKAGER: true},
env: {...process.env, RCT_TERMINAL: terminal, RCT_NO_LAUNCH_PACKAGER: true},
};
}

Expand Down Expand Up @@ -499,5 +453,11 @@ export default {
default: process.env.RCT_METRO_PORT || 8081,
parse: (val: string) => Number(val),
},
{
command: '--terminal [string]',
description:
'Launches the Metro Bundler in a new window using the specified terminal path.',
default: getDefaultUserTerminal(),
},
],
};
4 changes: 4 additions & 0 deletions packages/tools/src/getDefaultUserTerminal.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
const getDefaultUserTerminal = (): ?string =>
process.env.REACT_TERMINAL || process.env.TERM_PROGRAM;

export default getDefaultUserTerminal;
2 changes: 2 additions & 0 deletions packages/tools/src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,5 +3,7 @@
*/
export {default as logger} from './logger';
export {default as groupFilesByType} from './groupFilesByType';
export {default as isPackagerRunning} from './isPackagerRunning';
export {default as getDefaultUserTerminal} from './getDefaultUserTerminal';

export * from './errors';