Skip to content

Commit

Permalink
Merge pull request #373 from serverless-heaven/error-on-stdout-empty-…
Browse files Browse the repository at this point in the history
…with-npm

Do not swallow errors in case npm ls fails without an empty stdout and use spawn for child processes
HyperBrain authored Apr 25, 2018

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
2 parents 5cf2479 + 4ee2dfe commit 81f6696
Showing 13 changed files with 512 additions and 254 deletions.
1 change: 0 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
@@ -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
9 changes: 0 additions & 9 deletions lib/Configuration.js
Original file line number Diff line number Diff line change
@@ -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;
}
10 changes: 0 additions & 10 deletions lib/Configuration.test.js
Original file line number Diff line number Diff line change
@@ -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
});
});
9 changes: 4 additions & 5 deletions lib/packExternalModules.js
Original file line number Diff line number Diff line change
@@ -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]`));
});
})
6 changes: 3 additions & 3 deletions lib/packagers/index.js
Original file line number Diff line number Diff line change
@@ -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>;
*
* }
*/
93 changes: 47 additions & 46 deletions lib/packagers/npm.js
Original file line number Diff line number Diff line change
@@ -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();
}
}
Loading

0 comments on commit 81f6696

Please sign in to comment.