feat: better error messages with human readable outputs (#609)

* refactor: properly handle invalid template package * refactor: error checks when getting template name * refactor: remove duplicate exception throw * refactor: check if assets dest exists * refactor: add error checks when upgrading * chore: add comment for using process.exit() * refactor: improve error outputs in link command * refactor: use for of loop * refactor: check package before uninstalling * refactor: use CLIError instead of direct exit * refactor: handle potential android pkg name errors * refactor: fix typos/mistakes in error outputs * refactor: add adb & android sdk path checks * refactor: fix imports * refactor: improve install apk error outputs * Apply suggestions from code review Co-Authored-By: Michał Pierzchała <thymikee@gmail.com> * refactor: add init doc link in error output * test: make sure all tests pass * refactor: use try/catch around readFileSync * refactor: do not read build.gradle twice * refactor: clean up code in android linking * fix: packageJson not defined in templateName * refactor: remove unintentional --verbose flag * refactor: make flow very happy * refactor: remove unnecessary variable definition * pkgjson undefined tweak * eslint fix * remove adb/android_home checks because we have doctor * use mockImplementationOnce * revert link changes, as it's going to be removed in next version * inline fns * revert uninstall changes as we're removing that in next version * adjust copy * mock existsSync once * remove unused code Co-authored-by: Michał Pierzchała <thymikee@gmail.com>
react-native-community · Feb 5, 2020 · b110966 · b110966
1 parent 43d2cad
commit b110966
Show file tree

Hide file tree

Showing 11 changed files with 158 additions and 54 deletions.
diff --git a/packages/cli/src/commands/bundle/assetPathUtils.ts b/packages/cli/src/commands/bundle/assetPathUtils.ts
@@ -31,7 +31,7 @@ function getAndroidAssetSuffix(scale: number): string {
     case 4:
       return 'xxxhdpi';
     default:
-      throw new Error('no such scale');
+      return '';
   }
 }
 

diff --git a/packages/cli/src/commands/bundle/saveAssets.ts b/packages/cli/src/commands/bundle/saveAssets.ts
@@ -13,7 +13,7 @@ import fs from 'fs';
 import filterPlatformAssetScales from './filterPlatformAssetScales';
 import getAssetDestPathAndroid from './getAssetDestPathAndroid';
 import getAssetDestPathIOS from './getAssetDestPathIOS';
-import {logger} from '@react-native-community/cli-tools';
+import {logger, CLIError} from '@react-native-community/cli-tools';
 import {AssetData} from './buildBundle';
 
 interface CopiedFiles {
@@ -30,6 +30,12 @@ function saveAssets(
     return Promise.resolve();
   }
 
+  if (!fs.existsSync(assetsDest)) {
+    throw new CLIError(
+      `The specified assets destination folder "${assetsDest}" does not exist.`,
+    );
+  }
+
   const getAssetDestPath =
     platform === 'android' ? getAssetDestPathAndroid : getAssetDestPathIOS;
 

diff --git a/packages/cli/src/commands/init/__tests__/template.test.ts b/packages/cli/src/commands/init/__tests__/template.test.ts
@@ -1,6 +1,7 @@
 jest.mock('execa', () => jest.fn());
 import execa from 'execa';
 import path from 'path';
+import fs from 'fs';
 import * as PackageManger from '../../../tools/packageManager';
 import {
   installTemplatePackage,
@@ -32,7 +33,7 @@ test('installTemplatePackage', async () => {
 
 test('getTemplateConfig', () => {
   jest.mock(
-    `${TEMPLATE_SOURCE_DIR}/node_modules/${TEMPLATE_NAME}/template.config`,
+    `${TEMPLATE_SOURCE_DIR}/node_modules/${TEMPLATE_NAME}/template.config.js`,
     () => ({
       placeholderName: 'someName',
       templateDir: 'someDir',
@@ -42,7 +43,7 @@ test('getTemplateConfig', () => {
     },
   );
   jest.spyOn(path, 'resolve').mockImplementationOnce((...e) => e.join('/'));
-
+  jest.spyOn(fs, 'existsSync').mockImplementationOnce(() => true);
   expect(getTemplateConfig(TEMPLATE_NAME, TEMPLATE_SOURCE_DIR)).toEqual({
     placeholderName: 'someName',
     templateDir: 'someDir',
@@ -51,7 +52,7 @@ test('getTemplateConfig', () => {
     TEMPLATE_SOURCE_DIR,
     'node_modules',
     TEMPLATE_NAME,
-    'template.config',
+    'template.config.js',
   );
 });
 

diff --git a/packages/cli/src/commands/init/__tests__/templateName.test.ts b/packages/cli/src/commands/init/__tests__/templateName.test.ts
@@ -1,28 +1,23 @@
 import {processTemplateName} from '../templateName';
+import fs from 'fs';
 
 const RN_NPM_PACKAGE = 'react-native';
 const ABS_RN_PATH = '/path/to/react-native';
+const ABS_RN_PATH_WINDOWS = 'path/to/react-native';
 
-test('supports file protocol with absolute path', async () => {
-  if (process.platform === 'win32') {
-    console.warn('[SKIP] Jest virtual mocks seem to be broken on Windows');
-    return;
-  }
-  jest.mock(
-    `${ABS_RN_PATH}/package.json`,
-    () => ({
-      name: 'react-native',
-    }),
-    {virtual: true},
-  );
-  expect(await processTemplateName(`file://${ABS_RN_PATH}`)).toEqual({
-    uri: ABS_RN_PATH,
+test('supports file protocol with absolute path', () => {
+  jest.spyOn(fs, 'existsSync').mockImplementationOnce(() => true);
+  jest
+    .spyOn(fs, 'readFileSync')
+    .mockImplementationOnce(() => JSON.stringify({name: 'react-native'}));
+  expect(processTemplateName(`file://${ABS_RN_PATH}`)).toEqual({
+    uri: process.platform === 'win32' ? ABS_RN_PATH_WINDOWS : ABS_RN_PATH,
     name: RN_NPM_PACKAGE,
   });
 });
 
-test('supports npm packages as template names', async () => {
-  expect(await processTemplateName(RN_NPM_PACKAGE)).toEqual({
+test('supports npm packages as template names', () => {
+  expect(processTemplateName(RN_NPM_PACKAGE)).toEqual({
     uri: RN_NPM_PACKAGE,
     name: RN_NPM_PACKAGE,
   });
@@ -37,15 +32,16 @@ test.each`
   ${'@scoped/name@tag'}    | ${'@scoped/name@tag'}    | ${'@scoped/name'}
 `(
   'supports versioned npm package "$templateName" as template name',
-  async ({templateName, uri, name}) => {
-    expect(await processTemplateName(templateName)).toEqual({uri, name});
+  ({templateName, uri, name}) => {
+    expect(processTemplateName(templateName)).toEqual({uri, name});
   },
 );
 
-test('supports path to tgz archives', async () => {
+test('supports path to tgz archives', () => {
   const ABS_RN_TARBALL_PATH =
     '/path/to/react-native/react-native-1.2.3-rc.0.tgz';
-  expect(await processTemplateName(`file://${ABS_RN_TARBALL_PATH}`)).toEqual({
+  jest.spyOn(fs, 'existsSync').mockImplementationOnce(() => true);
+  expect(processTemplateName(`file://${ABS_RN_TARBALL_PATH}`)).toEqual({
     uri: `file://${ABS_RN_TARBALL_PATH}`,
     name: 'react-native',
   });

diff --git a/packages/cli/src/commands/init/template.ts b/packages/cli/src/commands/init/template.ts
@@ -1,9 +1,11 @@
 import execa from 'execa';
 import path from 'path';
-import {logger} from '@react-native-community/cli-tools';
+import {logger, CLIError} from '@react-native-community/cli-tools';
 import * as PackageManager from '../../tools/packageManager';
 import copyFiles from '../../tools/copyFiles';
 import replacePathSepForRegex from '../../tools/replacePathSepForRegex';
+import fs from 'fs';
+import chalk from 'chalk';
 
 export type TemplateConfig = {
   placeholderName: string;
@@ -33,11 +35,18 @@ export function getTemplateConfig(
     templateSourceDir,
     'node_modules',
     templateName,
-    'template.config',
+    'template.config.js',
   );
 
-  logger.debug(`Getting config from ${configFilePath}.js`);
-
+  logger.debug(`Getting config from ${configFilePath}`);
+  if (!fs.existsSync(configFilePath)) {
+    throw new CLIError(
+      `Couldn't find the "${configFilePath} file inside "${templateName}" template. Please make sure the template is valid.
+      Read more: ${chalk.underline.dim(
+        'https://github.com/react-native-community/cli/blob/master/docs/init.md#creating-custom-template',
+      )}`,
+    );
+  }
   return require(configFilePath);
 }
 

diff --git a/packages/cli/src/commands/init/templateName.ts b/packages/cli/src/commands/init/templateName.ts
@@ -1,5 +1,7 @@
 import path from 'path';
 import {URL} from 'url';
+import fs from 'fs';
+import {CLIError} from '@react-native-community/cli-tools';
 
 const FILE_PROTOCOL = /file:/;
 const TARBALL = /\.tgz$/;
@@ -9,20 +11,49 @@ const VERSIONED_PACKAGE = /(@?.+)(@)(.+)/;
 function handleFileProtocol(filePath: string) {
   let uri = new URL(filePath).pathname;
   if (process.platform === 'win32') {
-    // On Windows, the pathname has an extra leading / so remove that
+    // On Windows, the pathname has an extra / at the start, so remove that
     uri = uri.substring(1);
   }
+  if (!fs.existsSync(uri)) {
+    throw new CLIError(
+      `Failed to retrieve template name. The specified template directory path "${uri}" does not exist or is invalid.`,
+    );
+  }
+  const packageJsonPath = path.join(uri, 'package.json');
+  let packageJson;
+  try {
+    packageJson = JSON.parse(
+      fs.readFileSync(packageJsonPath, {encoding: 'utf8'}),
+    );
+  } catch {
+    throw new CLIError(
+      'Failed to retrieve template name. We expect the template directory to include "package.json" file, but it was not found.',
+    );
+  }
+
+  if (!packageJson || !packageJson.name) {
+    throw new CLIError(
+      `Failed to retrieve template name. We expect the "package.json" of the template to include the "name" property, but we found "${
+        packageJson ? packageJson.name : 'undefined'
+      }" which is invalid.`,
+    );
+  }
   return {
     uri,
-    name: require(path.join(uri, 'package.json')).name,
+    name: packageJson.name,
   };
 }
 
 function handleTarball(filePath: string) {
+  if (!fs.existsSync(filePath)) {
+    throw new CLIError(
+      `Failed to retrieve tarball name. The specified tarball path "${filePath}" does not exist or is invalid.`,
+    );
+  }
   const nameWithVersion = path.parse(path.basename(filePath)).name;
   const tarballVersionMatch = nameWithVersion.match(VERSION_POSTFIX);
   if (!tarballVersionMatch) {
-    throw new Error(
+    throw new CLIError(
       `Failed to retrieve tarball name. We expect the tarball to include package name and version, e.g.: "template-name-1.2.3-rc.0.tgz", but received: "${nameWithVersion}".`,
     );
   }
@@ -36,7 +67,7 @@ function handleTarball(filePath: string) {
 function handleVersionedPackage(versionedPackage: string) {
   const versionedPackageMatch = versionedPackage.match(VERSIONED_PACKAGE);
   if (!versionedPackageMatch) {
-    throw new Error(
+    throw new CLIError(
       `Failed to retrieve package name. We expect the package to include name and version, e.g.: "template-name@1.2.3-rc.0", but received: "${versionedPackage}".`,
     );
   }
@@ -46,7 +77,7 @@ function handleVersionedPackage(versionedPackage: string) {
   };
 }
 
-export async function processTemplateName(templateName: string) {
+export function processTemplateName(templateName: string) {
   if (templateName.match(TARBALL)) {
     return handleTarball(templateName);
   }

diff --git a/packages/cli/src/commands/link/link.ts b/packages/cli/src/commands/link/link.ts
@@ -49,7 +49,7 @@ async function link(
   );
 
   if (rawPackageName === undefined) {
-    logger.debug('No package name provided, will linking all possible assets.');
+    logger.debug('No package name provided, will link all possible assets.');
     return linkAll(ctx, {linkDeps: opts.all, linkAssets: true});
   }
 

diff --git a/packages/cli/src/commands/link/linkAll.ts b/packages/cli/src/commands/link/linkAll.ts
@@ -21,7 +21,7 @@ async function linkAll(config: Config, options: Options) {
     logger.info(
       `Linking dependencies using "${chalk.bold(
         'link',
-      )}" command is now legacy and likely unnecessary. We encourage you to try ${chalk.bold(
+      )}" command is now legacy and most likely unnecessary. We encourage you to try ${chalk.bold(
         'autolinking',
       )} that comes with React Native v0.60 default template. Autolinking happens at build time – during CocoaPods install or Gradle install phase. More information: ${chalk.dim.underline(
         'https://github.com/react-native-community/cli/blob/master/docs/autolinking.md',

diff --git a/packages/cli/src/commands/upgrade/upgrade.ts b/packages/cli/src/commands/upgrade/upgrade.ts
@@ -13,22 +13,52 @@ const webDiffUrl = 'https://react-native-community.github.io/upgrade-helper';
 const rawDiffUrl =
   'https://raw.githubusercontent.com/react-native-community/rn-diff-purge/diffs/diffs';
 
+const isConnected = (output: string): boolean => {
+  // there is no reliable way of checking for internet connectivity, so we should just
+  // read the output from npm (to check for connectivity errors) which is faster and relatively more reliable.
+  return !output.includes('the host is inaccessible');
+};
+
+const checkForErrors = (output: string): void => {
+  if (!output) {
+    return;
+  }
+  if (!isConnected(output)) {
+    throw new CLIError(
+      'Upgrade failed. You do not seem to have an internet connection.',
+    );
+  }
+
+  if (output.includes('npm ERR')) {
+    throw new CLIError(`Upgrade failed with the following errors:\n${output}`);
+  }
+
+  if (output.includes('npm WARN')) {
+    logger.warn(output);
+  }
+};
+
 const getLatestRNVersion = async (): Promise<string> => {
   logger.info('No version passed. Fetching latest...');
-  const {stdout} = await execa('npm', ['info', 'react-native', 'version']);
+  const {stdout, stderr} = await execa('npm', [
+    'info',
+    'react-native',
+    'version',
+  ]);
+  checkForErrors(stderr);
   return stdout;
 };
 
 const getRNPeerDeps = async (
   version: string,
 ): Promise<{[key: string]: string}> => {
-  const {stdout} = await execa('npm', [
+  const {stdout, stderr} = await execa('npm', [
     'info',
     `react-native@${version}`,
     'peerDependencies',
     '--json',
   ]);
-
+  checkForErrors(stderr);
   return JSON.parse(stdout);
 };