refactor: Unify logging #146

sidferreira · 2019-02-04T11:48:17Z

Summary:

Based on #95, replaced console.(log|warn|error) by logger.(info|warn|error).
Used regular expressions but everything seems to be working as should.

Test Plan:

All tests are passing, no other change was required.

thymikee · 2019-02-04T11:50:38Z

packages/cli/src/generator/printRunInstructions.js

-  console.log(`   Open ${relativeXcodeProjectPath} in Xcode`);
-  console.log('   Hit the Run button');
+  logger.info(chalk.white.bold('To run your app on iOS:'));
+  logger.info(`   cd ${absoluteProjectDir}`);


logger.info will prepend INFO to the message, we don't want it to be here. Can you fold it down to a single logger.info call with a nice template string inside?

thymikee · 2019-02-04T11:52:00Z

packages/cli/src/logIOS/logIOS.js

@@ -37,7 +38,7 @@ async function logIOS() {

  const device = findAvailableDevice(devices);
  if (device === undefined) {
-    console.log(chalk.red('No active iOS device found'));
+    logger.info(chalk.red('No active iOS device found'));


Suggested change

logger.info(chalk.red('No active iOS device found'));

logger.error('No active iOS device found');

thymikee · 2019-02-04T11:52:31Z

packages/cli/src/runAndroid/runAndroid.js

@@ -33,7 +34,7 @@ function checkAndroid(root) {
 */
 function runAndroid(argv: Array<string>, config: ContextT, args: Object) {
  if (!checkAndroid(args.root)) {
-    console.log(
+    logger.info(
      chalk.red(


this should be logger.error without chalk

thymikee · 2019-02-04T11:52:49Z

packages/cli/src/runAndroid/runAndroid.js

    } else if (result === 'unrecognized') {
-      console.warn(
+      logger.warn(
        chalk.yellow('JS server not recognized, continuing with build...')


Suggested change

chalk.yellow('JS server not recognized, continuing with build...')

'JS server not recognized, continuing with build...'

this (and similar) still need to be addressed

sidferreira · 2019-02-04T11:52:56Z

Cannot call `logger.info` because array type [1] is incompatible with string [2] in array element.

   packages/cli/src/runAndroid/runAndroid.js:138:7
   138|       logger.info(devices);
              ^^^^^^^^^^^^^^^^^^^^

References:
   packages/cli/src/runAndroid/adb.js:37:24
    37| function getDevices(): Array<string> {
                               ^^^^^^^^^^^^^ [1]
   packages/cli/src/util/logger.js:10:34
    10| const info = (...messages: Array<string>) => {
                                         ^^^^^^ [2]

Should we change the type to accept arrays or actually split the parameter?

thymikee · 2019-02-04T11:53:12Z

packages/cli/src/runAndroid/runAndroid.js

@@ -47,14 +48,14 @@ function runAndroid(argv: Array<string>, config: ContextT, args: Object) {

  return isPackagerRunning(args.port).then(result => {
    if (result === 'running') {
-      console.log(chalk.bold('JS server already running.'));
+      logger.info(chalk.bold('JS server already running.'));


Suggested change

logger.info(chalk.bold('JS server already running.'));

logger.info('JS server already running.');

thymikee · 2019-02-04T11:54:24Z

packages/cli/src/runAndroid/runAndroid.js

@@ -101,7 +102,7 @@ function buildAndRun(args) {
        adbPath
      );
    }
-    console.log(chalk.red('Argument missing for parameter --deviceId'));
+    logger.info(chalk.red('Argument missing for parameter --deviceId'));


Suggested change

logger.info(chalk.red('Argument missing for parameter --deviceId'));

logger.error('Argument missing for parameter --deviceId');

thymikee · 2019-02-04T11:54:35Z

packages/cli/src/runAndroid/runAndroid.js

        chalk.yellow('JS server not recognized, continuing with build...')
      );
    } else {
      // result == 'not_running'
-      console.log(chalk.bold('Starting JS server...'));
+      logger.info(chalk.bold('Starting JS server...'));


Suggested change

logger.info(chalk.bold('Starting JS server...'));

logger.info('Starting JS server...');

@thymikee remove all chalk.bold?

sidferreira · 2019-02-04T11:54:56Z

log(chalk.red => error
Remove all bold
chalk.yellow => warn

thymikee · 2019-02-04T11:55:17Z

packages/cli/src/runAndroid/runAndroid.js

-      console.log(devices);
+      logger.info(`Could not find device with the id: "${args.deviceId}".`);
+      logger.info('Choose one of the following:');
+      logger.info(devices);


please fold it into single logger.error

thymikee · 2019-02-04T11:55:24Z

packages/cli/src/runAndroid/runAndroid.js

    }
  } else {
-    console.log('No Android devices connected.');
+    logger.info('No Android devices connected.');


Suggested change

logger.info('No Android devices connected.');

logger.error('No Android devices connected.');

thymikee

I've left a bunch of suggestions in certain places, but they hold true for all the code here. One of the reason we use logger is to get rid of extra chalk styles as they should be controlled uniformly by the logger, so please remove those. Also there are places where error is more appropriate than info – I mark some of them but please review the rest of the diff as well

thymikee · 2019-02-04T11:58:56Z

Should we change the type to accept arrays or actually split the parameter?

just spread the devices:

logger.info(...devices);

sidferreira · 2019-02-04T12:06:17Z

module.exports = function copyMiddleware(req, res, next) {
  if (req.url === '/copy-to-clipboard') {
    const ret = copyToClipBoard(req.rawBody);
    if (!ret) {
      logger.warn(chalk.red('Copy button is not supported on this platform!'));
    }
    res.end('OK');
  } else {
    next();
  }
};

error or warn? @thymikee

thymikee · 2019-02-04T12:07:28Z

I'd go with regular warn for now :)

thymikee · 2019-02-04T12:46:14Z

packages/cli/src/server/util/debugger-ui/debuggerWorker.js

        return;
      }
      hasWarned = true;
-      console.warn(
+      logger.warn(


we don't want logger in debugger-ui because it runs in a browser

thymikee

last few bits, and we're good to merge it :)

thymikee · 2019-02-04T13:01:23Z

packages/cli/src/runIOS/runIOS.js

+    logger.info('');
+    logger.info('** INSTALLATION FAILED **');
+    logger.info('Make sure you have ios-deploy installed globally.');
+    logger.info('(e.g "npm install -g ios-deploy")');


can we fold this?

thymikee · 2019-02-04T13:01:45Z

packages/cli/src/runAndroid/tryRunAdbReverse.js

@@ -23,13 +23,13 @@ function tryRunAdbReverse(packagerPort: number | string, device: string) {
      adbArgs.unshift('-s', device);
    }

-    console.log(chalk.bold(`Running ${adbPath} ${adbArgs.join(' ')}`));
+    logger.info(chalk.bold(`Running ${adbPath} ${adbArgs.join(' ')}`));


Suggested change

logger.info(chalk.bold(`Running ${adbPath} ${adbArgs.join(' ')}`));

logger.info(`Running ${adbPath} ${adbArgs.join(' ')}`);

sidferreira · 2019-02-04T13:16:31Z

react-native-cli/packages/cli/src/init/init.js
  53:3  error  Unused eslint-disable directive (no problems were reported from 'import/no-unresolved')

  // eslint-disable-next-line import/no-unresolved
  const reactNativePackageJson = require('react-native/package.json');

not mine, but it's showing up

thymikee · 2019-02-04T13:18:24Z

53:3 error Unused eslint-disable directive (no problems were reported from 'import/no-unresolved')

Try rebasing/merging master, as after this PR #144 we dropped react-native as a dev dependency

thymikee · 2019-02-04T13:19:28Z

packages/cli/src/runIOS/runIOS.js

-      logger.info('Choose one of the following:');
-      printFoundDevices(devices);
+      logger.error(`Could not find device with the name: "${args.device}".
+Choose one of the following:${printFoundDevices(devices)}`);


printFoundDevices outputs logs by itself, so this is wrong

@thymikee I changed it to be a list.
There's another place which the output is a list (I think for Android). I changed to be like this to keep some sort of standard. But LMK if I need to revert.

Ok, that's good (name could be different now, but there's too much changes to this PR already :D) Thanks for bearing with me!

refactor(logger): replaces all console uses

7a8b4bd

thymikee reviewed Feb 4, 2019

View reviewed changes

thymikee requested changes Feb 4, 2019

View reviewed changes

Sid Ferreira added 5 commits February 4, 2019 10:14

refactor(logger): tweaks chalk usage

5a2160e

fix: remove unused chalk

c7df37f

fix: remove chalk.yellow and unused chalk

9fc3e06

fix: changes some info to error

01f2a2f

Merge branch 'master' into fix/replaceLoggers

259aee6

thymikee reviewed Feb 4, 2019

View reviewed changes

fix: revert Debugger changes

5a2dfd0

thymikee approved these changes Feb 4, 2019

View reviewed changes

fix: remove pending bold, join outputs

7721ffd

thymikee reviewed Feb 4, 2019

View reviewed changes

thymikee merged commit 486a65a into react-native-community:master Feb 4, 2019

	logger.info(chalk.red('No active iOS device found'));
	logger.error('No active iOS device found');

	chalk.yellow('JS server not recognized, continuing with build...')
	'JS server not recognized, continuing with build...'

	logger.info(chalk.bold('JS server already running.'));
	logger.info('JS server already running.');

	logger.info(chalk.red('Argument missing for parameter --deviceId'));
	logger.error('Argument missing for parameter --deviceId');

	logger.info(chalk.bold('Starting JS server...'));
	logger.info('Starting JS server...');

	logger.info('No Android devices connected.');
	logger.error('No Android devices connected.');

	logger.info(chalk.bold(`Running ${adbPath} ${adbArgs.join(' ')}`));
	logger.info(`Running ${adbPath} ${adbArgs.join(' ')}`);

refactor: Unify logging #146

refactor: Unify logging #146

Conversation

sidferreira commented Feb 4, 2019

Summary:

Test Plan:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sidferreira commented Feb 4, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sidferreira commented Feb 4, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thymikee left a comment

Choose a reason for hiding this comment

thymikee commented Feb 4, 2019

sidferreira commented Feb 4, 2019

thymikee commented Feb 4, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thymikee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sidferreira commented Feb 4, 2019 • edited Loading

thymikee commented Feb 4, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sidferreira commented Feb 4, 2019 •

edited

Loading

sidferreira commented Feb 4, 2019 •

edited

Loading