From 29da13b30549667d66a9e348b01f4301ca3825b3 Mon Sep 17 00:00:00 2001 From: Frank Schmid Date: Thu, 7 Sep 2017 13:42:50 +0200 Subject: [PATCH 1/2] Ignore irrelevant NPM problems when building the dependency graph --- lib/packExternalModules.js | 172 ++++++++++++++++++++++--------------- 1 file changed, 103 insertions(+), 69 deletions(-) diff --git a/lib/packExternalModules.js b/lib/packExternalModules.js index bd00e0dc9..da24d4cd8 100644 --- a/lib/packExternalModules.js +++ b/lib/packExternalModules.js @@ -116,81 +116,115 @@ module.exports = { this.options.verbose && this.serverless.cli.log(`Fetch dependency graph from ${packageJsonPath}`); // Get first level dependency graph const command = 'npm ls -prod -json -depth=1'; // Only prod dependencies - let dependencyGraph = {}; - try { - const depJson = childProcess.execSync(command, { + + const ignoredNpmErrors = [ + { npmError: 'extraneous', log: false }, + { npmError: 'missing', log: false }, + { npmError: 'peer dep missing', log: true }, + ]; + + return BbPromise.fromCallback(cb => { + childProcess.exec(command, { cwd: path.dirname(packageJsonPath), maxBuffer: this.serverless.service.custom.packExternalModulesMaxBuffer || 200 * 1024, 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); + + if (failed) { + return cb(err); + } + } + return cb(null, stdout); }); - dependencyGraph = JSON.parse(depJson); - } catch (e) { - // We rethrow here. It's not recoverable. - throw e; - } - - // (1) Generate dependency composition - const compositeModules = _.uniq(_.flatMap(stats.stats, compileStats => { - const externalModules = getExternalModules.call(this, compileStats); - return getProdModules.call(this, externalModules, packagePath, dependencyGraph); - })); + }) + .then(depJson => BbPromise.try(() => JSON.parse(depJson))) + .then(dependencyGraph => { + const problems = _.get(dependencyGraph, 'problems', []); + if (this.options.verbose && !_.isEmpty(problems)) { + this.serverless.cli.log(`Ignoring ${_.size(problems)} NPM errors:`); + _.forEach(problems, problem => { + this.serverless.cli.log(`=> ${problem}`); + }); + } - // (1.a) Install all needed modules - const compositeModulePath = path.join(this.webpackOutputPath, 'dependencies'); - const compositePackageJson = path.join(compositeModulePath, 'package.json'); - this.serverless.utils.writeFileSync(compositePackageJson, '{}'); + // (1) Generate dependency composition + const compositeModules = _.uniq(_.flatMap(stats.stats, compileStats => { + const externalModules = getExternalModules.call(this, compileStats); + return getProdModules.call(this, externalModules, packagePath, dependencyGraph); + })); - this.serverless.cli.log('Packing external modules: ' + compositeModules.join(', ')); + if (_.isEmpty(compositeModules)) { + // The compiled code does not reference any external modules at all + this.serverless.cli.log('No external modules needed'); + return BbPromise.resolve(stats); + } - return new BbPromise((resolve, reject) => { - const start = _.now(); - npm.install(compositeModules, { - cwd: compositeModulePath, - maxBuffer: this.serverless.service.custom.packExternalModulesMaxBuffer || 200 * 1024, - save: true - }).then(() => { - this.options.verbose && this.serverless.cli.log(`Package took [${_.now() - start} ms]`); // eslint-disable-line promise/always-return - resolve(stats.stats); - }).catch(e => { - reject(e); - }); - }) - .mapSeries(compileStats => { - const modulePath = compileStats.compilation.compiler.outputPath; - - // Create package.json - const modulePackageJson = path.join(modulePath, 'package.json'); - const modulePackage = { - dependencies: {} - }; - const prodModules = getProdModules.call(this, getExternalModules.call(this, compileStats), packagePath, dependencyGraph); - _.forEach(prodModules, prodModule => { - const splitModule = _.split(prodModule, '@'); - // If we have a scoped module we have to re-add the @ - if (_.startsWith(prodModule, '@')) { - splitModule.splice(0, 1); - splitModule[0] = '@' + splitModule[0]; - } - const moduleVersion = _.join(_.tail(splitModule), '@'); - modulePackage.dependencies[_.first(splitModule)] = moduleVersion; - }); - this.serverless.utils.writeFileSync(modulePackageJson, JSON.stringify(modulePackage, null, 2)); - - // Copy modules - const startCopy = _.now(); - return BbPromise.fromCallback(callback => fse.copy(path.join(compositeModulePath, 'node_modules'), path.join(modulePath, 'node_modules'), callback)) - .tap(() => this.options.verbose && this.serverless.cli.log(`Copy modules: ${modulePath} [${_.now() - startCopy} ms]`)) - .then(() => { - // Prune extraneous packages - removes not needed ones - const startPrune = _.now(); - return BbPromise.fromCallback(callback => { - childProcess.exec('npm prune', { - cwd: modulePath - }, callback); - }) - .tap(() => this.options.verbose && this.serverless.cli.log(`Prune: ${modulePath} [${_.now() - startPrune} ms]`)); - }); - }) - .return(stats); + // (1.a) Install all needed modules + const compositeModulePath = path.join(this.webpackOutputPath, 'dependencies'); + const compositePackageJson = path.join(compositeModulePath, 'package.json'); + this.serverless.utils.writeFileSync(compositePackageJson, '{}'); + + this.serverless.cli.log('Packing external modules: ' + compositeModules.join(', ')); + + return new BbPromise((resolve, reject) => { + const start = _.now(); + npm.install(compositeModules, { + cwd: compositeModulePath, + maxBuffer: this.serverless.service.custom.packExternalModulesMaxBuffer || 200 * 1024, + save: true + }).then(() => { + this.options.verbose && this.serverless.cli.log(`Package took [${_.now() - start} ms]`); // eslint-disable-line promise/always-return + resolve(stats.stats); + }).catch(e => { + reject(e); + }); + }) + .mapSeries(compileStats => { + const modulePath = compileStats.compilation.compiler.outputPath; + + // Create package.json + const modulePackageJson = path.join(modulePath, 'package.json'); + const modulePackage = { + dependencies: {} + }; + const prodModules = getProdModules.call(this, getExternalModules.call(this, compileStats), packagePath, dependencyGraph); + _.forEach(prodModules, prodModule => { + const splitModule = _.split(prodModule, '@'); + // If we have a scoped module we have to re-add the @ + if (_.startsWith(prodModule, '@')) { + splitModule.splice(0, 1); + splitModule[0] = '@' + splitModule[0]; + } + const moduleVersion = _.join(_.tail(splitModule), '@'); + modulePackage.dependencies[_.first(splitModule)] = moduleVersion; + }); + this.serverless.utils.writeFileSync(modulePackageJson, JSON.stringify(modulePackage, null, 2)); + + // Copy modules + const startCopy = _.now(); + return BbPromise.fromCallback(callback => fse.copy(path.join(compositeModulePath, 'node_modules'), path.join(modulePath, 'node_modules'), callback)) + .tap(() => this.options.verbose && this.serverless.cli.log(`Copy modules: ${modulePath} [${_.now() - startCopy} ms]`)) + .then(() => { + // Prune extraneous packages - removes not needed ones + const startPrune = _.now(); + return BbPromise.fromCallback(callback => { + childProcess.exec('npm prune', { + cwd: modulePath + }, callback); + }) + .tap(() => this.options.verbose && this.serverless.cli.log(`Prune: ${modulePath} [${_.now() - startPrune} ms]`)); + }); + }) + .return(stats); + }); } }; From 51ad4b14dadbd1c0968d7f8387560dfc0fb02500 Mon Sep 17 00:00:00 2001 From: Frank Schmid Date: Thu, 7 Sep 2017 15:00:49 +0200 Subject: [PATCH 2/2] Added unit tests --- tests/packExternalModules.test.js | 269 +++++++++++++++++++++++++----- 1 file changed, 225 insertions(+), 44 deletions(-) diff --git a/tests/packExternalModules.test.js b/tests/packExternalModules.test.js index 6cd8d37da..02badc2f5 100644 --- a/tests/packExternalModules.test.js +++ b/tests/packExternalModules.test.js @@ -72,11 +72,87 @@ describe('packExternalModules', () => { afterEach(() => { // Reset all counters and restore all stubbed functions + writeFileSyncStub.reset(); + childProcessMock.exec.reset(); + fsExtraMock.copy.reset(); + npmMock.install.reset(); sandbox.reset(); sandbox.restore(); }); describe('packExternalModules()', () => { + // Test data + const stats = { + stats: [ + { + compilation: { + chunks: [ + { + modules: [ + { + identifier: _.constant('"crypto"') + }, + { + identifier: _.constant('"uuid/v4"') + }, + { + identifier: _.constant('external "eslint"') + }, + { + identifier: _.constant('"mockery"') + }, + { + identifier: _.constant('"@scoped/vendor/module1"') + }, + { + identifier: _.constant('external "@scoped/vendor/module2"') + }, + { + identifier: _.constant('external "uuid/v4"') + }, + { + identifier: _.constant('external "bluebird"') + }, + ] + } + ], + compiler: { + outputPath: '/my/Service/Path/.webpack/service' + } + } + } + ] + }; + const noExtStats = { + stats: [ + { + compilation: { + chunks: [ + { + modules: [ + { + identifier: _.constant('"crypto"') + }, + { + identifier: _.constant('"uuid/v4"') + }, + { + identifier: _.constant('"mockery"') + }, + { + identifier: _.constant('"@scoped/vendor/module1"') + }, + ] + } + ], + compiler: { + outputPath: '/my/Service/Path/.webpack/service' + } + } + } + ] + }; + it('should do nothing if webpackIncludeModules is not set', () => { _.unset(serverless, 'service.custom.webpackIncludeModules'); return expect(module.packExternalModules({ stats: [] })).to.eventually.deep.equal({ stats: [] }) @@ -89,48 +165,108 @@ describe('packExternalModules', () => { }); it('should install external modules', () => { - // Test data - const stats = { - stats: [ - { - compilation: { - chunks: [ - { - modules: [ - { - identifier: _.constant('"crypto"') - }, - { - identifier: _.constant('"uuid/v4"') - }, - { - identifier: _.constant('external "eslint"') - }, - { - identifier: _.constant('"mockery"') - }, - { - identifier: _.constant('"@scoped/vendor/module1"') - }, - { - identifier: _.constant('external "@scoped/vendor/module2"') - }, - { - identifier: _.constant('external "uuid/v4"') - }, - { - identifier: _.constant('external "bluebird"') - }, - ] - } - ], - compiler: { - outputPath: '/my/Service/Path/.webpack/service' - } - } - } - ] + const expectedPackageJSON = { + dependencies: { + '@scoped/vendor': '1.0.0', + uuid: '^5.4.1', + bluebird: '^3.4.0' + } }; + + module.webpackOutputPath = 'outputPath'; + npmMock.install.returns(BbPromise.resolve()); + fsExtraMock.copy.yields(); + childProcessMock.exec.onFirstCall().yields(null, '{}', ''); + childProcessMock.exec.onSecondCall().yields(); + return expect(module.packExternalModules(stats)).to.be.fulfilled + .then(() => BbPromise.all([ + // npm install should have been called with all externals from the package mock + expect(npmMock.install).to.have.been.calledOnce, + expect(npmMock.install).to.have.been.calledWithExactly([ + '@scoped/vendor@1.0.0', + 'uuid@^5.4.1', + 'bluebird@^3.4.0' + ], + { + cwd: path.join('outputPath', 'dependencies'), + maxBuffer: 204800, + save: true + }), + // The module package JSON and the composite one should have been stored + expect(writeFileSyncStub).to.have.been.calledTwice, + expect(writeFileSyncStub.firstCall.args[1]).to.equal('{}'), + expect(writeFileSyncStub.secondCall.args[1]).to.equal(JSON.stringify(expectedPackageJSON, null, 2)), + // The modules should have been copied + expect(fsExtraMock.copy).to.have.been.calledOnce, + // npm ls and npm prune should have been called + expect(childProcessMock.exec).to.have.been.calledTwice, + expect(childProcessMock.exec.firstCall).to.have.been.calledWith( + 'npm ls -prod -json -depth=1' + ), + expect(childProcessMock.exec.secondCall).to.have.been.calledWith( + 'npm prune' + ) + ])); + }); + + it('should reject if npm install fails', () => { + module.webpackOutputPath = 'outputPath'; + npmMock.install.returns(BbPromise.reject(new Error('npm install failed'))); + fsExtraMock.copy.yields(); + childProcessMock.exec.onFirstCall().yields(null, '{}', ''); + childProcessMock.exec.onSecondCall().yields(); + return expect(module.packExternalModules(stats)).to.be.rejectedWith('npm install failed') + .then(() => BbPromise.all([ + // npm install should have been called with all externals from the package mock + expect(npmMock.install).to.have.been.calledOnce, + // npm ls and npm prune should have been called + expect(childProcessMock.exec).to.have.been.calledOnce, + ])); + }); + + it('should reject if npm returns a critical error', () => { + const stderr = 'ENOENT: No such file'; + module.webpackOutputPath = 'outputPath'; + npmMock.install.returns(BbPromise.resolve()); + fsExtraMock.copy.yields(); + childProcessMock.exec.yields(new Error('something went wrong'), '{}', stderr); + return expect(module.packExternalModules(stats)).to.be.rejectedWith('something went wrong') + .then(() => BbPromise.all([ + expect(npmMock.install).to.not.have.been.called, + // The module package JSON and the composite one should have been stored + expect(writeFileSyncStub).to.not.have.been.called, + // The modules should have been copied + expect(fsExtraMock.copy).to.not.have.been.called, + // npm ls and npm prune should have been called + expect(childProcessMock.exec).to.have.been.calledOnce, + expect(childProcessMock.exec.firstCall).to.have.been.calledWith( + 'npm ls -prod -json -depth=1' + ), + ])); + }); + + it('should reject if npm returns critical and minior errors', () => { + const stderr = 'ENOENT: No such file\nnpm ERR! extraneous: sinon@2.3.8 ./babel-dynamically-entries/node_modules/serverless-webpack/node_modules/sinon\n\n'; + module.webpackOutputPath = 'outputPath'; + npmMock.install.returns(BbPromise.resolve()); + fsExtraMock.copy.yields(); + childProcessMock.exec.yields(new Error('something went wrong'), '{}', stderr); + return expect(module.packExternalModules(stats)).to.be.rejectedWith('something went wrong') + .then(() => BbPromise.all([ + expect(npmMock.install).to.not.have.been.called, + // The module package JSON and the composite one should have been stored + expect(writeFileSyncStub).to.not.have.been.called, + // The modules should have been copied + expect(fsExtraMock.copy).to.not.have.been.called, + // npm ls and npm prune should have been called + expect(childProcessMock.exec).to.have.been.calledOnce, + expect(childProcessMock.exec.firstCall).to.have.been.calledWith( + 'npm ls -prod -json -depth=1' + ), + ])); + }); + + it('should ignore minor local NPM errors and log them', () => { const expectedPackageJSON = { dependencies: { '@scoped/vendor': '1.0.0', @@ -138,12 +274,33 @@ describe('packExternalModules', () => { bluebird: '^3.4.0' } }; + const stderr = _.join( + [ + 'npm ERR! extraneous: sinon@2.3.8 ./babel-dynamically-entries/node_modules/serverless-webpack/node_modules/sinon', + 'npm ERR! missing: internalpackage-1@1.0.0, required by internalpackage-2@1.0.0', + 'npm ERR! peer dep missing: sinon@2.3.8', + ], + '\n' + ); + const lsResult = { + version: '1.0.0', + problems: [ + 'npm ERR! extraneous: sinon@2.3.8 ./babel-dynamically-entries/node_modules/serverless-webpack/node_modules/sinon', + 'npm ERR! missing: internalpackage-1@1.0.0, required by internalpackage-2@1.0.0', + 'npm ERR! peer dep missing: sinon@2.3.8', + ], + dependencies: { + '@scoped/vendor': '1.0.0', + uuid: '^5.4.1', + bluebird: '^3.4.0' + } + }; module.webpackOutputPath = 'outputPath'; npmMock.install.returns(BbPromise.resolve()); fsExtraMock.copy.yields(); - childProcessMock.exec.yields(); - childProcessMock.execSync.returns('{}'); + childProcessMock.exec.onFirstCall().yields(new Error('NPM error'), JSON.stringify(lsResult), stderr); + childProcessMock.exec.onSecondCall().yields(); return expect(module.packExternalModules(stats)).to.be.fulfilled .then(() => BbPromise.all([ // npm install should have been called with all externals from the package mock @@ -164,7 +321,31 @@ describe('packExternalModules', () => { expect(writeFileSyncStub.secondCall.args[1]).to.equal(JSON.stringify(expectedPackageJSON, null, 2)), // The modules should have been copied expect(fsExtraMock.copy).to.have.been.calledOnce, - // npm prune should have been called + // npm ls and npm prune should have been called + expect(childProcessMock.exec).to.have.been.calledTwice, + expect(childProcessMock.exec.firstCall).to.have.been.calledWith( + 'npm ls -prod -json -depth=1' + ), + expect(childProcessMock.exec.secondCall).to.have.been.calledWith( + 'npm prune' + ) + ])); + }); + + it('should not install modules if no external modules are reported', () => { + module.webpackOutputPath = 'outputPath'; + npmMock.install.returns(BbPromise.resolve()); + fsExtraMock.copy.yields(); + childProcessMock.exec.yields(null, '{}', ''); + return expect(module.packExternalModules(noExtStats)).to.be.fulfilled + .then(stats => BbPromise.all([ + expect(stats).to.deep.equal(noExtStats), + expect(npmMock.install).to.not.have.been.called, + // The module package JSON and the composite one should have been stored + expect(writeFileSyncStub).to.not.have.been.called, + // The modules should have been copied + expect(fsExtraMock.copy).to.not.have.been.called, + // npm ls and npm prune should have been called expect(childProcessMock.exec).to.have.been.calledOnce, ])); });