Repository: ember-cli/ember-cli
Pull request #9108 (2020-03-16 19:41:11)

Cleanup of async in `CLI` / `Builder` while digging into issues around progress clean up.
See individual commits for details.


Patch:
diff --git a/lib/cli/cli.js b/lib/cli/cli.js
index e12323a86..914caedfe 100644
--- a/lib/cli/cli.js
+++ b/lib/cli/cli.js
@@ -118,139 +118,145 @@ class CLI {
 
     return command;
   }
+
   /**
    * @private
    * @method run
-   * @param environment
+   * @param {Promise<object>} environmentPromiseHash
    * @return {Promise}
    */
-  run(environment) {
-    if (environment === undefined) {
+  async run(environmentPromiseHash) {
+    if (environmentPromiseHash === undefined) {
       return Promise.reject(new Error('Unable to execute "run" command without environment argument'));
     }
-    this._environment = environment;
     let shutdownOnExit = null;
-    return hash(environment)
-      .then(environment => {
-        let args = environment.cliArgs.slice();
-        let commandName = args.shift();
-        let commandArgs = args;
-        let helpOptions;
 
-        let commandLookupCreationToken = heimdall.start('lookup-command');
+    let environment = (this._environment = await hash(environmentPromiseHash));
 
-        let command = this.maybeMakeCommand(commandName, commandArgs);
+    try {
+      let args = environment.cliArgs.slice();
+      let commandName = args.shift();
+      let commandArgs = args;
+      let helpOptions;
 
-        commandLookupCreationToken.stop();
+      let commandLookupCreationToken = heimdall.start('lookup-command');
 
-        getOptionArgs('--verbose', commandArgs).forEach(arg => {
-          process.env[`EMBER_VERBOSE_${arg.toUpperCase()}`] = 'true';
-        });
+      let command = this.maybeMakeCommand(commandName, commandArgs);
 
-        let platformCheckerToken = heimdall.start('platform-checker');
+      commandLookupCreationToken.stop();
 
-        const PlatformChecker = require('../utilities/platform-checker');
-        let platform = new PlatformChecker(process.version);
-        let recommendation =
-          ' We recommend that you use the most-recent "Active LTS" version of Node.js.' +
-          ' See https://git.io/v7S5n for details.';
+      getOptionArgs('--verbose', commandArgs).forEach(arg => {
+        process.env[`EMBER_VERBOSE_${arg.toUpperCase()}`] = 'true';
+      });
 
-        if (!this.testing) {
-          if (platform.isDeprecated) {
-            this.ui.writeDeprecateLine(`Node ${process.version} is no longer supported by Ember CLI.${recommendation}`);
-          }
+      let platformCheckerToken = heimdall.start('platform-checker');
 
-          if (!platform.isTested) {
-            this.ui.writeWarnLine(
-              `Node ${process.version} is not tested against Ember CLI on your platform.${recommendation}`
-            );
-          }
+      const PlatformChecker = require('../utilities/platform-checker');
+      let platform = new PlatformChecker(process.version);
+      let recommendation =
+        ' We recommend that you use the most-recent "Active LTS" version of Node.js.' +
+        ' See https://git.io/v7S5n for details.';
+
+      if (!this.testing) {
+        if (platform.isDeprecated) {
+          this.ui.writeDeprecateLine(`Node ${process.version} is no longer supported by Ember CLI.${recommendation}`);
         }
 
-        platformCheckerToken.stop();
+        if (!platform.isTested) {
+          this.ui.writeWarnLine(
+            `Node ${process.version} is not tested against Ember CLI on your platform.${recommendation}`
+          );
+        }
+      }
 
-        logger.info('command: %s', commandName);
+      platformCheckerToken.stop();
 
-        if (!this.testing) {
-          process.chdir(environment.project.root);
-          let skipInstallationCheck = commandArgs.indexOf('--skip-installation-check') !== -1;
-          if (environment.project.isEmberCLIProject() && !skipInstallationCheck) {
-            const InstallationChecker = require('../models/installation-checker');
-            new InstallationChecker({ project: environment.project }).checkInstallations();
-          }
+      logger.info('command: %s', commandName);
+
+      if (!this.testing) {
+        process.chdir(environment.project.root);
+        let skipInstallationCheck = commandArgs.indexOf('--skip-installation-check') !== -1;
+        if (environment.project.isEmberCLIProject() && !skipInstallationCheck) {
+          const InstallationChecker = require('../models/installation-checker');
+          new InstallationChecker({ project: environment.project }).checkInstallations();
         }
+      }
 
-        let instrumentation = this.instrumentation;
-        let onCommandInterrupt;
+      let instrumentation = this.instrumentation;
+      let onCommandInterrupt;
 
-        let runPromise = Promise.resolve()
-          .then(() => {
-            instrumentation.stopAndReport('init');
+      let runPromise = Promise.resolve().then(async () => {
+        let resultOrExitCode;
+
+        try {
+          instrumentation.stopAndReport('init');
+
+          try {
             instrumentation.start('command');
 
             loggerTesting.info('cli: command.beforeRun');
             onProcessInterrupt.addHandler(onCommandInterrupt);
 
-            return command.beforeRun(commandArgs);
-          })
-          .then(() => {
+            await command.beforeRun(commandArgs);
+
             loggerTesting.info('cli: command.validateAndRun');
 
-            return command.validateAndRun(commandArgs);
-          })
-          .then(result => {
+            resultOrExitCode = await command.validateAndRun(commandArgs);
+          } finally {
             instrumentation.stopAndReport('command', commandName, commandArgs);
 
             onProcessInterrupt.removeHandler(onCommandInterrupt);
+          }
+        } finally {
+          instrumentation.start('shutdown');
+          shutdownOnExit = function() {
+            instrumentation.stopAndReport('shutdown');
+          };
+        }
 
-            return result;
-          })
-          .finally(() => {
-            instrumentation.start('shutdown');
-            shutdownOnExit = function() {
-              instrumentation.stopAndReport('shutdown');
-            };
-          })
-          .then(result => {
-            // if the help option was passed, call the help command
-            if (result === 'callHelp') {
-              helpOptions = {
-                environment,
-                commandName,
-                commandArgs,
-              };
-
-              return this.callHelp(helpOptions);
-            }
-
-            return result;
-          })
-          .then(exitCode => {
-            loggerTesting.info(`cli: command run complete. exitCode: ${exitCode}`);
-            // TODO: fix this
-            // Possibly this issue: https://github.com/joyent/node/issues/8329
-            // Wait to resolve promise when running on windows.
-            // This ensures that stdout is flushed so acceptance tests get full output
-
-            return new Promise(resolve => {
-              if (process.platform === 'win32') {
-                setTimeout(resolve, 250, exitCode);
-              } else {
-                resolve(exitCode);
-              }
-            });
-          });
+        // if the help option was passed, call the help command
+        if (resultOrExitCode === 'callHelp') {
+          helpOptions = {
+            environment,
+            commandName,
+            commandArgs,
+          };
 
-        onCommandInterrupt = () => Promise.resolve(command.onInterrupt()).then(() => runPromise);
+          resultOrExitCode = await this.callHelp(helpOptions);
+        }
 
-        return runPromise;
-      })
-      .finally(() => {
-        if (shutdownOnExit) {
-          shutdownOnExit();
+        let exitCode = resultOrExitCode;
+
+        loggerTesting.info(`cli: command run complete. exitCode: ${exitCode}`);
+        // TODO: fix this
+        // Possibly this issue: https://github.com/joyent/node/issues/8329
+        // Wait to resolve promise when running on windows.
+        // This ensures that stdout is flushed so acceptance tests get full output
+
+        if (process.platform === 'win32') {
+          return new Promise(resolve => {
+            setTimeout(resolve, 250, exitCode);
+          });
+        } else {
+          return exitCode;
         }
-      })
-      .catch(this.logError.bind(this));
+      });
+
+      onCommandInterrupt = async () => {
+        await command.onInterrupt();
+
+        return await runPromise;
+      };
+
+      return await runPromise;
+    } catch (error) {
+      this.logError(error);
+      return 1;
+    } finally {
+      if (shutdownOnExit) {
+        shutdownOnExit();
+      }
+    }
   }
 
   /**
diff --git a/lib/cli/index.js b/lib/cli/index.js
index a0c3030fe..d54517873 100644
--- a/lib/cli/index.js
+++ b/lib/cli/index.js
@@ -68,7 +68,7 @@ function configureLogger(env) {
 }
 
 // Options: Array cliArgs, Stream inputStream, Stream outputStream, EventEmitter process
-module.exports = function(options) {
+module.exports = async function(options) {
   // `process` should be captured before we require any libraries which
   // may use `process.exit` work arounds for async cleanup.
   willInterruptProcess.capture(options.process || process);
@@ -143,5 +143,9 @@ module.exports = function(options) {
     settings: merge(defaultUpdateCheckerOptions, config.getAll()),
   };
 
-  return cli.run(environment).finally(() => willInterruptProcess.release());
+  try {
+    return await cli.run(environment);
+  } finally {
+    willInterruptProcess.release();
+  }
 };
diff --git a/lib/models/builder.js b/lib/models/builder.js
index 8db1618a1..7d069dc7c 100644
--- a/lib/models/builder.js
+++ b/lib/models/builder.js
@@ -31,7 +31,7 @@ class Builder extends CoreObject {
     this._instantiationStack = new Error().stack.replace(/[^\n]*\n/, '');
     this._cleanup = this.cleanup.bind(this);
 
-    this._cleanupPromise = null;
+    this._cleanupStarted = false;
     this._onProcessInterrupt = options.onProcessInterrupt || onProcessInterrupt;
 
     this._onProcessInterrupt.addHandler(this._cleanup);
@@ -136,21 +136,6 @@ class Builder extends CoreObject {
     return changes.map(op => op[1]);
   }
 
-  /**
-   * @private
-   * @method processBuildResult
-   * @param results
-   * @return {Promise}
-   */
-  processBuildResult(results) {
-    return Promise.resolve()
-      .then(() => this.copyToOutputPath(results.directory))
-      .then(syncResult => {
-        results.outputChanges = syncResult;
-        return results;
-      });
-  }
-
   /**
    * @private
    * @method processAddonBuildSteps
@@ -181,47 +166,65 @@ class Builder extends CoreObject {
    * @method build
    * @return {Promise}
    */
-  build(addWatchDirCallback, resultAnnotation) {
-    this.project._instrumentation.start('build');
+  async build(addWatchDirCallback, resultAnnotation) {
+    let buildResults, uiProgressIntervalID;
 
-    if (addWatchDirCallback && !this.broccoliBuilderFallback) {
-      for (let path of this.builder.watchedPaths) {
-        addWatchDirCallback(path);
+    try {
+      this.project._instrumentation.start('build');
+
+      if (addWatchDirCallback && !this.broccoliBuilderFallback) {
+        for (let path of this.builder.watchedPaths) {
+          addWatchDirCallback(path);
+        }
       }
-    }
 
-    this.ui.startProgress(progress.format(progress()));
-
-    const intervalID = setInterval(() => {
-      this.ui.spinner.text = progress.format(progress());
-    }, this.ui.spinner.interval);
-
-    return this.processAddonBuildSteps('preBuild')
-      .then(() => this.builder.build(this.broccoliBuilderFallback ? addWatchDirCallback : null))
-      .then(this.compatNode.bind(this), this.compatBroccoliPayload.bind(this))
-      .then(this.processAddonBuildSteps.bind(this, 'postBuild'))
-      .then(this.processBuildResult.bind(this))
-      .then(this.processAddonBuildSteps.bind(this, 'outputReady'))
-      .then(
-        result => {
-          this.project._instrumentation.stopAndReport('build', result, resultAnnotation);
-          return result;
-        },
-        reason => {
-          this.project._instrumentation.stopAndReport('build', null, resultAnnotation);
-          throw reason;
+      this.ui.startProgress(progress.format(progress()));
+
+      uiProgressIntervalID = setInterval(() => {
+        this.ui.spinner.text = progress.format(progress());
+      }, this.ui.spinner.interval);
+
+      await this.processAddonBuildSteps('preBuild');
+
+      if (this.broccoliBuilderFallback) {
+        try {
+          buildResults = await this.builder.build(addWatchDirCallback);
+        } catch (error) {
+          this.throwFormattedBroccoliError(error);
+        }
+      } else {
+        try {
+          await this.builder.build();
+
+          // build legacy style results object (this is passed to various addon APIs)
+          buildResults = {
+            directory: this.builder.outputPath,
+            graph: this.builder.outputNodeWrapper,
+          };
+        } catch (error) {
+          this.throwFormattedBroccoliError(error);
         }
-      )
-      .then(this.checkForPostBuildEnvironmentIssues.bind(this))
-      .finally(() => {
-        clearInterval(intervalID);
-        this.ui.stopProgress();
-      })
-      .catch(error => {
-        this.processAddonBuildSteps('buildError', error);
-        throw error;
-      })
-      .finally(this.finalizeBuild.bind(this));
+      }
+
+      await this.processAddonBuildSteps('postBuild', buildResults);
+
+      let outputChanges = await this.copyToOutputPath(buildResults.directory);
+
+      await this.processAddonBuildSteps('outputReady', Object.assign({ outputChanges }, buildResults));
+
+      this.checkForPostBuildEnvironmentIssues();
+
+      return buildResults;
+    } catch (error) {
+      await this.processAddonBuildSteps('buildError', error);
+
+      throw error;
+    } finally {
+      clearInterval(uiProgressIntervalID);
+      this.ui.stopProgress();
+      this.project._instrumentation.stopAndReport('build', buildResults, resultAnnotation);
+      this.project.configCache.clear();
+    }
   }
 
   /**
@@ -231,8 +234,9 @@ class Builder extends CoreObject {
    * @method cleanup
    * @return {Promise}
    */
-  cleanup() {
-    if (!this._cleanupPromise) {
+  async cleanup() {
+    if (!this._cleanupStarted) {
+      this._cleanupStarted = true;
       let ui = this.project.ui;
       ui.startProgress('cleaning up');
       ui.writeLine('cleaning up...');
@@ -243,20 +247,16 @@ class Builder extends CoreObject {
       this._onProcessInterrupt.removeHandler(this._cleanup);
 
       let node = heimdall.start({ name: 'Builder Cleanup' });
-
-      this._cleanupPromise = Promise.resolve()
-        .then(() => this.builder.cleanup())
-        .finally(() => {
-          ui.stopProgress();
-          node.stop();
-        })
-        .catch(err => {
-          ui.writeLine(chalk.red('Cleanup error.'));
-          ui.writeError(err);
-        });
+      try {
+        await this.builder.cleanup();
+      } catch (error) {
+        ui.writeLine(chalk.red('Cleanup error.'));
+        ui.writeError(error);
+      } finally {
+        ui.stopProgress();
+        node.stop();
+      }
     }
-
-    return this._cleanupPromise;
   }
 
   /**
@@ -279,33 +279,7 @@ class Builder extends CoreObject {
     return value;
   }
 
-  /**
-   * @private
-   * @method finalizeBuild
-   */
-  finalizeBuild() {
-    this.project.configCache.clear();
-  }
-
-  /**
-   * broccoli-builder reformats the response into {directory, graph}, this method is a backwards
-   * compatible shim for broccoli 1.x
-   * @private
-   * @method compatNode
-   * @param node The node returned from Broccoli builder
-   */
-  compatNode(node) {
-    if (!this.broccoliBuilderFallback) {
-      return {
-        directory: this.builder.outputPath,
-        graph: this.builder.outputNodeWrapper,
-      };
-    }
-
-    return node;
-  }
-
-  compatBroccoliPayload(err) {
+  throwFormattedBroccoliError(err) {
     // TODO fix ember-cli/console-ui to handle current broccoli broccoliPayload
     let broccoliPayload = err && err.broccoliPayload;
     if (broccoliPayload) {
diff --git a/lib/tasks/build.js b/lib/tasks/build.js
index fd7996a5e..465e55700 100644
--- a/lib/tasks/build.js
+++ b/lib/tasks/build.js
@@ -6,12 +6,10 @@ const Builder = require('../models/builder');
 
 module.exports = class BuildTask extends Task {
   // Options: String outputPath
-  run(options) {
+  async run(options) {
     let ui = this.ui;
     let analytics = this.analytics;
 
-    ui.startProgress(chalk.green('Building'), chalk.green('.'));
-
     let builder = new Builder({
       ui,
       outputPath: options.outputPath,
@@ -19,42 +17,43 @@ module.exports = class BuildTask extends Task {
       project: this.project,
     });
 
-    ui.writeLine(`Environment: ${options.environment}`);
-
-    let annotation = {
-      type: 'initial',
-      reason: 'build',
-      primaryFile: null,
-      changedFiles: [],
-    };
-
-    return builder
-      .build(null, annotation)
-      .then(results => {
-        let totalTime = results.totalTime / 1e6;
-
-        analytics.track({
-          name: 'ember build',
-          message: `${totalTime}ms`,
-        });
-
-        /*
-         * We use the `rebuild` category in our analytics setup for both builds
-         * and rebuilds. This is a bit confusing, but the actual thing we
-         * delineate on in the reports is the `variable` value below. This is
-         * used both here and in `lib/models/watcher.js`.
-         */
-        analytics.trackTiming({
-          category: 'rebuild',
-          variable: 'build time',
-          label: 'broccoli build time',
-          value: parseInt(totalTime, 10),
-        });
-      })
-      .finally(() => {
-        ui.stopProgress();
-        return builder.cleanup();
-      })
-      .then(() => ui.writeLine(chalk.green(`Built project successfully. Stored in "${options.outputPath}".`)));
+    try {
+      ui.startProgress(chalk.green('Building'), chalk.green('.'));
+
+      ui.writeLine(`Environment: ${options.environment}`);
+
+      let annotation = {
+        type: 'initial',
+        reason: 'build',
+        primaryFile: null,
+        changedFiles: [],
+      };
+
+      let results = await builder.build(null, annotation);
+      let totalTime = results.totalTime / 1e6;
+
+      analytics.track({
+        name: 'ember build',
+        message: `${totalTime}ms`,
+      });
+
+      /*
+       * We use the `rebuild` category in our analytics setup for both builds
+       * and rebuilds. This is a bit confusing, but the actual thing we
+       * delineate on in the reports is the `variable` value below. This is
+       * used both here and in `lib/models/watcher.js`.
+       */
+      analytics.trackTiming({
+        category: 'rebuild',
+        variable: 'build time',
+        label: 'broccoli build time',
+        value: parseInt(totalTime, 10),
+      });
+    } finally {
+      ui.stopProgress();
+      await builder.cleanup();
+    }
+
+    ui.writeLine(chalk.green(`Built project successfully. Stored in "${options.outputPath}".`));
   }
 };


Test patch:
diff --git a/tests/unit/cli/cli-test.js b/tests/unit/cli/cli-test.js
index 5d111527b..25623dc2d 100644
--- a/tests/unit/cli/cli-test.js
+++ b/tests/unit/cli/cli-test.js
@@ -5,6 +5,7 @@ const MockUI = require('console-ui/mock');
 const MockAnalytics = require('../../helpers/mock-analytics');
 const td = require('testdouble');
 const Command = require('../../../lib/models/command');
+const CLI = require('../../../lib/cli/cli');
 
 let ui;
 let analytics;
@@ -13,8 +14,6 @@ let isWithinProject;
 let project;
 let willInterruptProcess;
 
-let CLI;
-
 // helper to similate running the CLI
 function ember(args) {
   let cli = new CLI({
@@ -71,7 +70,6 @@ describe('Unit: CLI', function() {
     td.replace(willInterruptProcess, 'addHandler', td.function());
     td.replace(willInterruptProcess, 'removeHandler', td.function());
 
-    CLI = require('../../../lib/cli/cli');
     ui = new MockUI();
     analytics = new MockAnalytics();
     commands = {};
@@ -99,10 +97,6 @@ describe('Unit: CLI', function() {
 
   this.timeout(10000);
 
-  it('exists', function() {
-    expect(CLI).to.be.ok;
-  });
-
   it('ember', function() {
     let help = stubValidateAndRun('help');
 
diff --git a/tests/unit/models/builder-test.js b/tests/unit/models/builder-test.js
index 67745d286..1c6ae945d 100644
--- a/tests/unit/models/builder-test.js
+++ b/tests/unit/models/builder-test.js
@@ -141,8 +141,8 @@ describe('models/builder.js', function() {
         project,
         ui: project.ui,
         setupBroccoliBuilder,
-        processBuildResult(buildResults) {
-          return Promise.resolve(buildResults);
+        copyToOutputPath() {
+          return [];
         },
       });
 
@@ -202,8 +202,8 @@ describe('models/builder.js', function() {
       builder = new Builder({
         project,
         ui: project.ui,
-        processBuildResult(buildResults) {
-          return Promise.resolve(buildResults);
+        copyToOutputPath() {
+          return [];
         },
       });
 
@@ -222,8 +222,8 @@ describe('models/builder.js', function() {
         new Builder({
           project,
           ui: project.ui,
-          processBuildResult(buildResults) {
-            return Promise.resolve(buildResults);
+          copyToOutputPath() {
+            return [];
           },
         });
 
@@ -239,8 +239,8 @@ describe('models/builder.js', function() {
       builder = new Builder({
         project,
         ui: project.ui,
-        processBuildResult(buildResults) {
-          return Promise.resolve(buildResults);
+        copyToOutputPath() {
+          return [];
         },
       });
 
@@ -259,17 +259,20 @@ describe('models/builder.js', function() {
         project,
         ui: project.ui,
         setupBroccoliBuilder,
-        processBuildResult(buildResults) {
-          return Promise.resolve(buildResults);
-        },
       });
     });
 
-    it('is idempotent', function() {
-      let firstCleanupPromise = builder.cleanup();
-      expect(builder.cleanup()).to.equal(firstCleanupPromise);
+    it('is idempotent', async function() {
+      let cleanupCount = 0;
+      builder.builder.cleanup = function() {
+        cleanupCount++;
+      };
+
+      let cleanupPromises = [builder.cleanup(), builder.cleanup(), builder.cleanup(), builder.cleanup()];
 
-      return firstCleanupPromise;
+      await Promise.all(cleanupPromises);
+
+      expect(cleanupCount).to.equal(1);
     });
   });
 
@@ -314,8 +317,8 @@ describe('models/builder.js', function() {
             return originalBuild.call(this);
           };
         },
-        processBuildResult(buildResults) {
-          return Promise.resolve(buildResults);
+        copyToOutputPath() {
+          return [];
         },
         project,
         ui: project.ui,
@@ -352,7 +355,9 @@ describe('models/builder.js', function() {
       let outputReady = td.replace(addon, 'outputReady', td.function());
 
       await builder.build();
-      td.verify(outputReady(buildResults), { times: 1 });
+
+      let expected = Object.assign({ outputChanges: [] }, buildResults);
+      td.verify(outputReady(expected), { times: 1 });
     });
 
     describe('instrumentation hooks', function() {
@@ -375,26 +380,26 @@ describe('models/builder.js', function() {
       expect(hooksCalled).to.deep.equal(['preBuild', 'build', 'postBuild', 'outputReady']);
     });
 
-    it('should call postBuild before processBuildResult', async function() {
+    it('should call postBuild before copying to dist', async function() {
       let called = [];
 
       addon.postBuild = function() {
         called.push('postBuild');
       };
 
-      builder.processBuildResult = function() {
-        called.push('processBuildResult');
+      builder.copyToOutputPath = function() {
+        called.push('copyToOutputPath');
       };
 
       await builder.build();
-      expect(called).to.deep.equal(['postBuild', 'processBuildResult']);
+      expect(called).to.deep.equal(['postBuild', 'copyToOutputPath']);
     });
 
-    it('should call outputReady after processBuildResult', async function() {
+    it('should call outputReady after copying to output path', async function() {
       let called = [];
 
-      builder.processBuildResult = function() {
-        called.push('processBuildResult');
+      builder.copyToOutputPath = function() {
+        called.push('copyToOutputPath');
       };
 
       addon.outputReady = function() {
@@ -402,7 +407,7 @@ describe('models/builder.js', function() {
       };
 
       await builder.build();
-      expect(called).to.deep.equal(['processBuildResult', 'outputReady']);
+      expect(called).to.deep.equal(['copyToOutputPath', 'outputReady']);
     });
 
     it('buildError receives the error object from the errored step', async function() {