Merge pull request serverless-heaven#373 from serverless-heaven/error…

…-on-stdout-empty-with-npm Do not swallow errors in case npm ls fails without an empty stdout and use spawn for child processes
jamesmbourne · Apr 25, 2018 · 9ccb15e · 9ccb15e
2 parents c99329f + 080ae6f
commit 9ccb15e
Show file tree

Hide file tree

Showing 13 changed files with 512 additions and 254 deletions.
diff --git a/README.md b/README.md
@@ -61,7 +61,6 @@ custom:
     webpackConfig: 'webpack.config.js'   # Name of webpack configuration file
     includeModules: false   # Node modules configuration for packaging
     packager: 'npm'   # Packager that will be used to package your external modules
-    packExternalModulesMaxBuffer: 200 * 1024   # Size of stdio buffers for spawned child processes
 ```
 
 ### Webpack configuration file

diff --git a/lib/Configuration.js b/lib/Configuration.js
@@ -13,7 +13,6 @@ const DefaultConfig = {
   includeModules: false,
   packager: 'npm',
   packagerOptions: {},
-  packExternalModulesMaxBuffer: 200 * 1024,
   config: null
 };
 
@@ -31,10 +30,6 @@ class Configuration {
         this._config.includeModules = custom.webpackIncludeModules;
         this._hasLegacyConfig = true;
       }
-      if (custom.packExternalModulesMaxBuffer) {
-        this._config.packExternalModulesMaxBuffer = custom.packExternalModulesMaxBuffer;
-        this._hasLegacyConfig = true;
-      }
       if (_.isString(custom.webpack)) {
         this._config.webpackConfig = custom.webpack;
         this._hasLegacyConfig = true;
@@ -55,10 +50,6 @@ class Configuration {
     return this._config.includeModules;
   }
 
-  get packExternalModulesMaxBuffer() {
-    return this._config.packExternalModulesMaxBuffer;
-  }
-
   get packager() {
     return this._config.packager;
   }

diff --git a/lib/Configuration.test.js b/lib/Configuration.test.js
@@ -18,7 +18,6 @@ describe('Configuration', () => {
         includeModules: false,
         packager: 'npm',
         packagerOptions: {},
-        packExternalModulesMaxBuffer: 200 * 1024,
         config: null
       };
     });
@@ -43,12 +42,6 @@ describe('Configuration', () => {
       expect(config).to.have.a.property('includeModules').that.deep.equals(testCustom.webpackIncludeModules);
     });
 
-    it('should use custom.packExternalModulesMaxBuffer', () => {
-      const testCustom = { packExternalModulesMaxBuffer: 4711 };
-      const config = new Configuration(testCustom);
-      expect(config).to.have.a.property('packExternalModulesMaxBuffer').that.equals(4711);
-    });
-
     it('should use custom.webpack as string', () => {
       const testCustom = { webpack: 'myWebpackFile.js' };
       const config = new Configuration(testCustom);
@@ -73,7 +66,6 @@ describe('Configuration', () => {
         includeModules: { forceInclude: ['mod1'] },
         packager: 'npm',
         packagerOptions: {},
-        packExternalModulesMaxBuffer: 200 * 1024,
         config: null
       });
     });
@@ -93,7 +85,6 @@ describe('Configuration', () => {
         includeModules: { forceInclude: ['mod1'] },
         packager: 'npm',
         packagerOptions: {},
-        packExternalModulesMaxBuffer: 200 * 1024,
         config: null
       });
     });
@@ -112,7 +103,6 @@ describe('Configuration', () => {
         includeModules: { forceInclude: ['mod1'] },
         packager: 'npm',
         packagerOptions: {},
-        packExternalModulesMaxBuffer: 200 * 1024,
         config: null
       });
     });

diff --git a/lib/packExternalModules.js b/lib/packExternalModules.js
@@ -195,9 +195,8 @@ module.exports = {
     .then(packager => {
       // Get first level dependency graph
       this.options.verbose && this.serverless.cli.log(`Fetch dependency graph from ${packageJsonPath}`);
-      const maxExecBufferSize = this.configuration.packExternalModulesMaxBuffer;
 
-      return packager.getProdDependencies(path.dirname(packageJsonPath), 1, maxExecBufferSize)
+      return packager.getProdDependencies(path.dirname(packageJsonPath), 1)
       .then(dependencyGraph => {
         const problems = _.get(dependencyGraph, 'problems', []);
         if (this.options.verbose && !_.isEmpty(problems)) {
@@ -271,7 +270,7 @@ module.exports = {
         .then(() => {
           const start = _.now();
           this.serverless.cli.log('Packing external modules: ' + compositeModules.join(', '));
-          return packager.install(compositeModulePath, maxExecBufferSize, this.configuration.packagerOptions)
+          return packager.install(compositeModulePath, this.configuration.packagerOptions)
           .then(() => this.options.verbose && this.serverless.cli.log(`Package took [${_.now() - start} ms]`))
           .return(stats.stats);
         })
@@ -320,13 +319,13 @@ module.exports = {
           .then(() => {
             // Prune extraneous packages - removes not needed ones
             const startPrune = _.now();
-            return packager.prune(modulePath, maxExecBufferSize, this.configuration.packagerOptions)
+            return packager.prune(modulePath, this.configuration.packagerOptions)
             .tap(() => this.options.verbose && this.serverless.cli.log(`Prune: ${modulePath} [${_.now() - startPrune} ms]`));
           })
           .then(() => {
             // Prune extraneous packages - removes not needed ones
             const startRunScripts = _.now();
-            return packager.runScripts(modulePath, maxExecBufferSize, _.keys(packageScripts))
+            return packager.runScripts(modulePath, _.keys(packageScripts))
             .tap(() => this.options.verbose && this.serverless.cli.log(`Run scripts: ${modulePath} [${_.now() - startRunScripts} ms]`));
           });
         })

diff --git a/lib/packagers/index.js b/lib/packagers/index.js
@@ -7,11 +7,11 @@
  * interface Packager {
  * 
  * static get lockfileName(): string;
- * static getProdDependencies(cwd: string, depth: number = 1, maxExecBufferSize = undefined): BbPromise<Object>;
+ * static getProdDependencies(cwd: string, depth: number = 1): BbPromise<Object>;
  * static rebaseLockfile(pathToPackageRoot: string, lockfile: Object): void;
- * static install(cwd: string, maxExecBufferSize = undefined): BbPromise<void>;
+ * static install(cwd: string): BbPromise<void>;
  * static prune(cwd: string): BbPromise<void>;
- * static runScripts(cwd: string, maxExecBufferSize, scriptNames): BbPromise<void>;
+ * static runScripts(cwd: string, scriptNames): BbPromise<void>;
  * 
  * }
  */

diff --git a/lib/packagers/npm.js b/lib/packagers/npm.js
@@ -5,7 +5,7 @@
 
 const _ = require('lodash');
 const BbPromise = require('bluebird');
-const childProcess = require('child_process');
+const Utils = require('../utils');
 
 class NPM {
   static get lockfileName() {  // eslint-disable-line lodash/prefer-constant
@@ -16,39 +16,44 @@ class NPM {
     return true;
   }
 
-  static getProdDependencies(cwd, depth, maxExecBufferSize) {
+  static getProdDependencies(cwd, depth) {
     // Get first level dependency graph
-    const command = `npm ls -prod -json -depth=${depth || 1}`;  // Only prod dependencies
+    const command = /^win/.test(process.platform) ? 'npm.cmd' : 'npm';
+    const args = [
+      'ls',
+      '-prod',  // Only prod dependencies
+      '-json',
+      `-depth=${depth || 1}`
+    ];
 
     const ignoredNpmErrors = [
       { npmError: 'extraneous', log: false },
       { npmError: 'missing', log: false },
       { npmError: 'peer dep missing', log: true },
     ];
 
-    return BbPromise.fromCallback(cb => {
-      childProcess.exec(command, {
-        cwd: cwd,
-        maxBuffer: maxExecBufferSize,
-        encoding: 'utf8'
-      }, (err, stdout, stderr) => {
-        if (err) {
-          // Only exit with an error if we have critical npm errors for 2nd level inside
-          const errors = _.split(stderr, '\n');
-          const failed = _.reduce(errors, (failed, error) => {
-            if (failed) {
-              return true;
-            }
-            return !_.isEmpty(error) && !_.some(ignoredNpmErrors, ignoredError => _.startsWith(error, `npm ERR! ${ignoredError.npmError}`));
-          }, false);
-
+    return Utils.spawnProcess(command, args, {
+      cwd: cwd
+    })
+    .catch(err => {
+      if (err instanceof Utils.SpawnError) {
+        // Only exit with an error if we have critical npm errors for 2nd level inside
+        const errors = _.split(err.stderr, '\n');
+        const failed = _.reduce(errors, (failed, error) => {
           if (failed) {
-            return cb(err);
+            return true;
           }
+          return !_.isEmpty(error) && !_.some(ignoredNpmErrors, ignoredError => _.startsWith(error, `npm ERR! ${ignoredError.npmError}`));
+        }, false);
+
+        if (!failed && !_.isEmpty(err.stdout)) {
+          return BbPromise.resolve({ stdout: err.stdout });
         }
-        return cb(null, stdout);
-      });
+      }
+
+      return BbPromise.reject(err);
     })
+    .then(processOutput => processOutput.stdout)
     .then(depJson => BbPromise.try(() => JSON.parse(depJson)));
   }
 
@@ -73,36 +78,32 @@ class NPM {
     }
   }
 
-  static install(cwd, maxExecBufferSize) {
-    return BbPromise.fromCallback(cb => {
-      childProcess.exec('npm install', {
-        cwd: cwd,
-        maxBuffer: maxExecBufferSize,
-        encoding: 'utf8'
-      }, cb);
-    })
+  static install(cwd) {
+    const command = /^win/.test(process.platform) ? 'npm.cmd' : 'npm';
+    const args = ['install'];
+
+    return Utils.spawnProcess(command, args, { cwd })
     .return();
   }
 
-  static prune(cwd, maxExecBufferSize) {
-    return BbPromise.fromCallback(cb => {
-      childProcess.exec('npm prune', {
-        cwd: cwd,
-        maxBuffer: maxExecBufferSize,
-        encoding: 'utf8'
-      }, cb);
-    })
+  static prune(cwd) {
+    const command = /^win/.test(process.platform) ? 'npm.cmd' : 'npm';
+    const args = ['prune'];
+
+    return Utils.spawnProcess(command, args, { cwd })
     .return();
   }
 
-  static runScripts(cwd, maxExecBufferSize, scriptNames) {
-    return BbPromise.mapSeries(scriptNames, scriptName => BbPromise.fromCallback(cb => {
-      childProcess.exec(`npm run ${scriptName}`, {
-        cwd: cwd,
-        maxBuffer: maxExecBufferSize,
-        encoding: 'utf8'
-      }, cb);
-    }))
+  static runScripts(cwd, scriptNames) {
+    const command = /^win/.test(process.platform) ? 'npm.cmd' : 'npm';
+    return BbPromise.mapSeries(scriptNames, scriptName => {
+      const args = [
+        'run',
+        scriptName
+      ];
+
+      return Utils.spawnProcess(command, args, { cwd });
+    })
     .return();
   }
 }