Repository: serverless-heaven/serverless-webpack
Pull request #384 (2018-05-02 10:43:16)

Added error if needed runtime dependency is a devDependency
<!--
1. Do not remove any section of the template. If something is not applicable leave it empty but leave it in the PR
2. Please follow the template, otherwise we'll have to ask you to update it and it will take longer until your PR is merged
-->

## What did you implement:

Relates to #383, https://github.com/serverless-heaven/serverless-webpack/issues/369#issuecomment-385783247, #230

<!--
Briefly describe the feature if no issue exists for this PR. If possible only
submit PRs for existing issues. If the PR is trivial (like doc changes or simple
code fixes) it can be submitted without a related issue, but as soon as it adds
or changes functionality, a related issue should be present.
-->

This PR introduces better error handling for the case that a runtime dependency detected by Webpack is only found in `devDependencies` and would lead to an `Unable to import module` error when deployed.
If the detected module is forcefully excluded it will, however, succeed.

## How did you implement it:

<!--
If this is a nontrivial change please briefly describe your implementation so its easy for us to understand and review your code.
-->

Added a check for devDependencies in the production module loop.

## How can we verify it:

<!--
Add any applicable config, commands, screenshots or other resources
to make it easy for us to verify this works. The easier you make it for us
to review a PR, the faster we can review and merge it.

Examples:
* Step by step description, how to verify
* Screenshots - Showing the difference between your output and the master
* Other - Anything else that comes to mind to help us evaluate
-->

Move a needed dependency to devDependencies. It should error out as long as the dependency is not forcefully excluded.

## Todos:

- [x] Write tests
- ~~Write documentation~~
- [x] Fix linting errors
- [x] Make sure code coverage hasn't dropped
- [x] Provide verification config / commands / resources
- [x] Enable "Allow edits from maintainers" for this PR
- [x] Update the messages below

***Is this ready for review?:*** YES
***Is it a breaking change?:*** NO



Patch:
diff --git a/lib/packExternalModules.js b/lib/packExternalModules.js
index a060458..35487c4 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);


Test patch:
diff --git a/tests/mocks/packageIgnoredDevDeps.mock.json b/tests/mocks/packageIgnoredDevDeps.mock.json
new file mode 100644
index 0000000..fa6ef53
--- /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 af62394..c872514 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',