From fb8368e9f6edd25cf18b55d21c60391bd18d6e35 Mon Sep 17 00:00:00 2001
From: childish-sambino <sharrison@twilio.com>
Date: Tue, 19 May 2020 13:02:53 -0500
Subject: [PATCH] fix: delay module-loading error logs until all locations have
 been exhausted (#89)

If we're able to successfully load/install a runtime dependency, this change prevents any errors from being logged since we were successful in the end. This was confusing customers when they see things like keytar errors during the install process when everything's actually fine.
---
 src/services/require-install.js       | 32 ++++++++++++++++++++-------
 test/services/require-install.test.js |  3 +--
 2 files changed, 25 insertions(+), 10 deletions(-)

diff --git a/src/services/require-install.js b/src/services/require-install.js
index ba6e302f..4c3e8776 100644
--- a/src/services/require-install.js
+++ b/src/services/require-install.js
@@ -28,14 +28,18 @@ const getCommandPlugin = command => {
 /**
  * Retrieves the package version given a path.
  */
-const getPackageVersion = packagePath => {
+const getPackageVersion = (packagePath, errors = null) => {
   const pjsonPath = path.join(packagePath, 'package.json');
 
   try {
     return require(pjsonPath).version;
   } catch (error) {
     // Failure to read the version is non-fatal.
-    logger.debug(`Could not determine package version: ${error}`);
+    if (errors === null) {
+      logger.debug(`Could not determine package version: ${error}`);
+    } else {
+      errors.push(error);
+    }
   }
 };
 
@@ -68,11 +72,13 @@ const checkVersion = (currentVersion, targetVersion) => {
  * Loads the given package and installs it if missing or not the proper version.
  */
 const requireInstall = async (packageName, command) => {
+  const errors = [];
+
   // First, try to load the package the old-fashioned way.
   try {
     return require(packageName);
   } catch (error) {
-    logger.debug(`Error loading ${packageName}: ${error}`);
+    errors.push(error);
   }
 
   const plugin = getCommandPlugin(command);
@@ -81,7 +87,7 @@ const requireInstall = async (packageName, command) => {
   const pluginPath = path.join(command.config.dataDir, 'runtime_modules', plugin.name);
   const packagePath = path.join(pluginPath, 'node_modules', packageName);
 
-  const currentVersion = getPackageVersion(packagePath);
+  const currentVersion = getPackageVersion(packagePath, errors);
   const targetVersion = getDependencyVersion(packageName, plugin.pjson);
 
   // Then, try to load the package from the plugin's runtime modules path.
@@ -90,7 +96,7 @@ const requireInstall = async (packageName, command) => {
 
     return require(packagePath);
   } catch (error) {
-    logger.debug(`Error loading ${packageName}: ${error}`);
+    errors.push(error);
   }
 
   // If we're here, attempt to install the package in the plugin's runtime modules path.
@@ -106,11 +112,21 @@ const requireInstall = async (packageName, command) => {
     const packageTag = targetVersion ? `${packageName}@${targetVersion}` : packageName;
     await plugins.yarn.exec(['add', '--force', packageTag], { cwd: pluginPath, verbose: false });
   } catch (error) {
-    logger.debug(`Error installing ${packageName}: ${error}`);
+    errors.push(error);
   }
 
-  // Finally, re-attempt loading the package from the plugin's runtime modules path.
-  return require(packagePath);
+  try {
+    // Finally, re-attempt loading the package from the plugin's runtime modules path.
+    return require(packagePath);
+  } catch (error) {
+    // Debug log any lazy errors we swallowed earlier.
+    if (errors) {
+      logger.debug(`Error loading/installing ${packageName}:`);
+      errors.forEach(lazyError => logger.debug(lazyError));
+    }
+
+    throw error;
+  }
 };
 
 module.exports = {
diff --git a/test/services/require-install.test.js b/test/services/require-install.test.js
index c7eaae48..ae812187 100644
--- a/test/services/require-install.test.js
+++ b/test/services/require-install.test.js
@@ -111,9 +111,8 @@ describe('services', () => {
       test.stderr().it('will attempt to install packages', async ctx => {
         const command = { id: 'top-command', config };
         await expect(requireInstall('chai-dai', command)).to.be.rejected;
-        expect(ctx.stderr).to.contain('Error loading chai-dai');
         expect(ctx.stderr).to.contain('Installing chai-dai');
-        expect(ctx.stderr).to.contain('Error installing chai-dai');
+        expect(ctx.stderr).to.contain('Error loading/installing chai-dai');
       });
     });
   });