diff --git a/lib/packExternalModules.js b/lib/packExternalModules.js index a06045812..35487c42c 100644 --- a/lib/packExternalModules.js +++ b/lib/packExternalModules.js @@ -58,10 +58,10 @@ function removeExcludedModules(modules, packageForceExcludes, log) { } /** - * Resolve the needed versions of production depenencies for external modules. + * Resolve the needed versions of production dependencies for external modules. * @this - The active plugin instance */ -function getProdModules(externalModules, packagePath, dependencyGraph) { +function getProdModules(externalModules, packagePath, dependencyGraph, forceExcludes) { const packageJsonPath = path.join(process.cwd(), packagePath); const packageJson = require(packageJsonPath); const prodModules = []; @@ -89,20 +89,34 @@ function getProdModules(externalModules, packagePath, dependencyGraph) { const peerDependencies = require(modulePackagePath).peerDependencies; if (!_.isEmpty(peerDependencies)) { this.options.verbose && this.serverless.cli.log(`Adding explicit peers for dependency ${module.external}`); - const peerModules = getProdModules.call(this, _.map(peerDependencies, (value, key) => ({ external: key })), packagePath, dependencyGraph); + const peerModules = getProdModules.call(this, _.map(peerDependencies, (value, key) => ({ external: key })), packagePath, dependencyGraph, forceExcludes); Array.prototype.push.apply(prodModules, peerModules); } } catch (e) { this.serverless.cli.log(`WARNING: Could not check for peer dependencies of ${module.external}`); } - } else if (!packageJson.devDependencies || !packageJson.devDependencies[module.external]) { - // Add transient dependencies if they appear not in the service's dev dependencies - const originInfo = _.get(dependencyGraph, 'dependencies', {})[module.origin] || {}; - moduleVersion = _.get(_.get(originInfo, 'dependencies', {})[module.external], 'version'); - if (!moduleVersion) { - this.serverless.cli.log(`WARNING: Could not determine version of module ${module.external}`); + } else { + if (!packageJson.devDependencies || !packageJson.devDependencies[module.external]) { + // Add transient dependencies if they appear not in the service's dev dependencies + const originInfo = _.get(dependencyGraph, 'dependencies', {})[module.origin] || {}; + moduleVersion = _.get(_.get(originInfo, 'dependencies', {})[module.external], 'version'); + if (!moduleVersion) { + this.serverless.cli.log(`WARNING: Could not determine version of module ${module.external}`); + } + prodModules.push(moduleVersion ? `${module.external}@${moduleVersion}` : module.external); + } else if (packageJson.devDependencies && packageJson.devDependencies[module.external] && !_.includes(forceExcludes, module.external)) { + // To minimize the chance of breaking setups we whitelist packages available on AWS here. These are due to the previously missing check + // most likely set in devDependencies and should not lead to an error now. + const ignoredDevDependencies = ['aws-sdk']; + + if (!_.includes(ignoredDevDependencies, module.external)) { + // Runtime dependency found in devDependencies but not forcefully excluded + this.serverless.cli.log(`ERROR: Runtime dependency '${module.external}' found in devDependencies. Move it to dependencies or use forceExclude to explicitly exclude it.`); + throw new this.serverless.classes.Error(`Serverless-webpack dependency error: ${module.external}.`); + } + + this.serverless.cli.log(`WARNING: Runtime dependency '${module.external}' found in devDependencies. You should use forceExclude to explicitly exclude it.`); } - prodModules.push(moduleVersion ? `${module.external}@${moduleVersion}` : module.external); } }); @@ -220,7 +234,7 @@ module.exports = { getExternalModules.call(this, compileStats), _.map(packageForceIncludes, whitelistedPackage => ({ external: whitelistedPackage })) ); - return getProdModules.call(this, externalModules, packagePath, dependencyGraph); + return getProdModules.call(this, externalModules, packagePath, dependencyGraph, packageForceExcludes); })); removeExcludedModules.call(this, compositeModules, packageForceExcludes, true); @@ -292,7 +306,7 @@ module.exports = { _.concat( getExternalModules.call(this, compileStats), _.map(packageForceIncludes, whitelistedPackage => ({ external: whitelistedPackage })) - ), packagePath, dependencyGraph); + ), packagePath, dependencyGraph, packageForceExcludes); removeExcludedModules.call(this, prodModules, packageForceExcludes); const relPath = path.relative(modulePath, path.dirname(packageJsonPath)); addModulesToPackageJson(prodModules, modulePackage, relPath); diff --git a/tests/mocks/packageIgnoredDevDeps.mock.json b/tests/mocks/packageIgnoredDevDeps.mock.json new file mode 100644 index 000000000..fa6ef536a --- /dev/null +++ b/tests/mocks/packageIgnoredDevDeps.mock.json @@ -0,0 +1,33 @@ +{ + "dependencies": { + "archiver": "^2.0.0", + "bluebird": "^3.4.0", + "fs-extra": "^0.26.7", + "glob": "^7.1.2", + "lodash": "^4.17.4", + "npm-programmatic": "0.0.5", + "uuid": "^5.4.1", + "ts-node": "^3.2.0", + "@scoped/vendor": "1.0.0", + "pg": "^4.3.5" + }, + "devDependencies": { + "aws-sdk": "^2.10.1", + "babel-eslint": "^7.2.3", + "chai": "^4.1.0", + "chai-as-promised": "^7.1.1", + "eslint": "^4.3.0", + "eslint-plugin-import": "^2.7.0", + "eslint-plugin-lodash": "^2.4.4", + "eslint-plugin-promise": "^3.5.0", + "istanbul": "^0.4.5", + "mocha": "^3.4.2", + "mockery": "^2.1.0", + "serverless": "^1.17.0", + "sinon": "^2.3.8", + "sinon-chai": "^2.12.0" + }, + "peerDependencies": { + "webpack": "*" + } +} diff --git a/tests/packExternalModules.test.js b/tests/packExternalModules.test.js index af62394b3..c872514ae 100644 --- a/tests/packExternalModules.test.js +++ b/tests/packExternalModules.test.js @@ -13,6 +13,7 @@ const Configuration = require('../lib/Configuration'); const fsExtraMockFactory = require('./mocks/fs-extra.mock'); const packageMock = require('./mocks/package.mock.json'); const packageLocalRefMock = require('./mocks/packageLocalRef.mock.json'); +const packageIgnoredDevDepsMock = require('./mocks/packageIgnoredDevDeps.mock.json'); chai.use(require('chai-as-promised')); chai.use(require('sinon-chai')); @@ -136,9 +137,6 @@ describe('packExternalModules', () => { { identifier: _.constant('"uuid/v4"') }, - { - identifier: _.constant('external "eslint"') - }, { identifier: _.constant('"mockery"') }, @@ -191,6 +189,45 @@ describe('packExternalModules', () => { ] }; const statsWithFileRef = { + stats: [ + { + compilation: { + chunks: [ + new ChunkMock([ + { + identifier: _.constant('"crypto"') + }, + { + identifier: _.constant('"uuid/v4"') + }, + { + identifier: _.constant('"mockery"') + }, + { + identifier: _.constant('"@scoped/vendor/module1"') + }, + { + identifier: _.constant('external "@scoped/vendor/module2"') + }, + { + identifier: _.constant('external "uuid/v4"') + }, + { + identifier: _.constant('external "localmodule"') + }, + { + identifier: _.constant('external "bluebird"') + }, + ]) + ], + compiler: { + outputPath: '/my/Service/Path/.webpack/service' + } + } + } + ] + }; + const statsWithDevDependency = { stats: [ { compilation: { @@ -232,6 +269,48 @@ describe('packExternalModules', () => { } ] }; + const statsWithIgnoredDevDependency = { + stats: [ + { + compilation: { + chunks: [ + new ChunkMock([ + { + identifier: _.constant('"crypto"') + }, + { + identifier: _.constant('"uuid/v4"') + }, + { + identifier: _.constant('"mockery"') + }, + { + identifier: _.constant('"@scoped/vendor/module1"') + }, + { + identifier: _.constant('external "@scoped/vendor/module2"') + }, + { + identifier: _.constant('external "uuid/v4"') + }, + { + identifier: _.constant('external "localmodule"') + }, + { + identifier: _.constant('external "bluebird"') + }, + { + identifier: _.constant('external "aws-sdk"') + }, + ]) + ], + compiler: { + outputPath: '/my/Service/Path/.webpack/service' + } + } + } + ] + }; it('should do nothing if webpackIncludeModules is not set', () => { module.configuration = new Configuration(); @@ -760,6 +839,80 @@ describe('packExternalModules', () => { ])); }); + it('should reject if devDependency is required at runtime', () => { + module.webpackOutputPath = 'outputPath'; + fsExtraMock.pathExists.yields(null, false); + fsExtraMock.copy.yields(); + packagerMock.getProdDependencies.returns(BbPromise.resolve({})); + packagerMock.install.returns(BbPromise.resolve()); + packagerMock.prune.returns(BbPromise.resolve()); + packagerMock.runScripts.returns(BbPromise.resolve()); + module.compileStats = statsWithDevDependency; + return expect(module.packExternalModules()).to.be.rejectedWith('Serverless-webpack dependency error: eslint.') + .then(() => BbPromise.all([ + expect(module.serverless.cli.log).to.have.been.calledWith(sinon.match(/ERROR: Runtime dependency 'eslint' found in devDependencies/)), + // npm ls and npm install should have been called + expect(packagerMock.getProdDependencies).to.have.been.calledOnce, + expect(packagerMock.install).to.not.have.been.called, + expect(packagerMock.prune).to.not.have.been.called, + expect(packagerMock.runScripts).to.not.have.been.called, + ])); + }); + + it('should ignore aws-sdk if set only in devDependencies', () => { + module.configuration = new Configuration({ + webpack: { + includeModules: { + packagePath: path.join('ignoreDevDeps', 'package.json') + } + } + }); + module.webpackOutputPath = 'outputPath'; + fsExtraMock.pathExists.yields(null, false); + fsExtraMock.copy.yields(); + packagerMock.getProdDependencies.returns(BbPromise.resolve({})); + packagerMock.install.returns(BbPromise.resolve()); + packagerMock.prune.returns(BbPromise.resolve()); + packagerMock.runScripts.returns(BbPromise.resolve()); + module.compileStats = statsWithIgnoredDevDependency; + mockery.registerMock(path.join(process.cwd(), 'ignoreDevDeps', 'package.json'), packageIgnoredDevDepsMock); + return expect(module.packExternalModules()).to.be.fulfilled + .then(() => BbPromise.all([ + expect(module.serverless.cli.log).to.have.been.calledWith(sinon.match(/WARNING: Runtime dependency 'aws-sdk' found in devDependencies/)), + // npm ls and npm install should have been called + expect(packagerMock.getProdDependencies).to.have.been.calledOnce, + expect(packagerMock.install).to.have.been.calledOnce, + expect(packagerMock.prune).to.have.been.calledOnce, + expect(packagerMock.runScripts).to.have.been.calledOnce, + ])); + }); + + it('should succeed if devDependency is required at runtime but forcefully excluded', () => { + module.configuration = new Configuration({ + webpack: { + includeModules: { + forceExclude: ['eslint'] + } + } + }); + module.webpackOutputPath = 'outputPath'; + fsExtraMock.pathExists.yields(null, false); + fsExtraMock.copy.yields(); + packagerMock.getProdDependencies.returns(BbPromise.resolve({})); + packagerMock.install.returns(BbPromise.resolve()); + packagerMock.prune.returns(BbPromise.resolve()); + packagerMock.runScripts.returns(BbPromise.resolve()); + module.compileStats = statsWithDevDependency; + return expect(module.packExternalModules()).to.be.fulfilled + .then(() => BbPromise.all([ + // npm ls and npm install should have been called + expect(packagerMock.getProdDependencies).to.have.been.calledOnce, + expect(packagerMock.install).to.have.been.calledOnce, + expect(packagerMock.prune).to.have.been.calledOnce, + expect(packagerMock.runScripts).to.have.been.calledOnce, + ])); + }); + it('should read package-lock if found', () => { const expectedCompositePackageJSON = { name: 'test-service',