Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Added error if needed runtime dependency is a devDependency #384

Merged
merged 3 commits into from May 3, 2018
Merged

Added error if needed runtime dependency is a devDependency #384

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
58f0f57
Added error if needed runtime dependency is a devDependency
May 2, 2018
ddf1435
Ignore aws-sdk to reduce risk of breaking builds. Emit a warning inst…
May 3, 2018
a9e959b
Added missing file
May 3, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
38 changes: 26 additions & 12 deletions lib/packExternalModules.js
View file
Open in desktop
Expand Up @@ -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 = [];
Expand Down Expand Up @@ -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);
}
});

Expand Down Expand Up @@ -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);

Expand Down Expand Up @@ -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);
Expand Down
33 changes: 33 additions & 0 deletions tests/mocks/packageIgnoredDevDeps.mock.json
View file
Open in desktop
@@ -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": "*"
}
}
159 changes: 156 additions & 3 deletions tests/packExternalModules.test.js
View file
Open in desktop
Expand Up @@ -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'));
Expand Down Expand Up @@ -136,9 +137,6 @@ describe('packExternalModules', () => {
{
identifier: _.constant('"uuid/v4"')
},
{
identifier: _.constant('external "eslint"')
},
{
identifier: _.constant('"mockery"')
},
Expand Down Expand Up @@ -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: {
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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',
Expand Down