Repository: serverless-heaven/serverless-webpack
Pull request #542 (2019-10-16 14:43:20)

Remove optional peer dependencies
## What did you implement:

When fetching peer dependencies we now remove those which are optional.

And optional dependencies can be defined using Yarn (only at the moment) when using the `peerDependenciesMeta` field:

```json
"peerDependenciesMeta": {
  "peer-deps": {
    "optional": true
  }
}
```

I got that issue because jsdom recently added a peer dependency as optional: https://github.com/jsdom/jsdom/pull/2605

The `peerDependenciesMeta` exist in Yarn since almost a year now: https://github.com/yarnpkg/yarn/pull/6671

## How did you implement it:

If the peer dependency is defined as optional, I removed it from the peer dependencies.

## How can we verify it:

Build a project which contains the `jsdom` deps as of `15.2.0`.
It'll try to fetch the `canvas` deps which is defined as peer dependency but optional.
You should get that error when build the package using serverless:

```
Serverless: Bundling with Webpack...
   XX modules
Serverless: Fetch dependency graph from /Users/jeremy/project/package.json
...
Serverless: Adding explicit peers for dependency jsdom
Serverless: WARNING: Could not determine version of module canvas
Serverless: Package lock found - Using locked versions
Serverless: Packing external modules: jsdom@^15.2.0, canvas

  Error --------------------------------------------------

  Error: yarn install --frozen-lockfile --non-interactive --ignore-scripts failed with code 1
      at ChildProcess.child.on.exitCode (/Users/jeremy/Sites/project/node_modules/serverless-webpack/lib/utils.js:92:16)
      at emitTwo (events.js:126:13)
      at ChildProcess.emit (events.js:214:7)
      at maybeClose (internal/child_process.js:925:16)
      at Process.ChildProcess._handle.onexit (internal/child_process.js:209:5)

     For debugging logs, run again after setting the "SLS_DEBUG=*" environment variable.
```

With that PR the optional peer dependency will be ignored:

```
Serverless: Bundling with Webpack...
   XX modules
Serverless: Fetch dependency graph from /Users/jeremy/project/package.json
...
Serverless: Adding explicit peers for dependency jsdom
Serverless: Skipping peers dependency canvas for dependency jsdom because it's optional
Serverless: Package lock found - Using locked versions
Serverless: Packing external modules: jsdom@^15.2.0
Serverless: Package took [3123 ms]
```

## 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


j0k3r: I’ll take a look!
j0k3r: I force pushed to remove the merge commit from master and rebased against the master so the branch is clean and up to date.

I've also added more test to cover each scenarii: with & without optional peer deps.

I've duplicated the whole _peer dependencies_ tests because I didn't know how to achieve same test with a different `package.json`.
j0k3r: Force pushed to make the PR up-to-date with the master branch.

Also coverage dropped because there is an issue on Travis. The Node 6 build generate a _good_ coverage while  the Node 8 build generate _less_ coverage:
- Node 8: https://travis-ci.org/github/serverless-heaven/serverless-webpack/jobs/683232428#L523
- Node 6 https://travis-ci.org/github/serverless-heaven/serverless-webpack/jobs/683232427#L507

Patch:
diff --git a/README.md b/README.md
index 1d8e26c..155b143 100644
--- a/README.md
+++ b/README.md
@@ -756,25 +756,6 @@ me to take it over and continue working on the project. That helped to revive it
 
 ## Release Notes
 
-* 5.3.2
-  * Fix eslint prettier #518
-  * Add TypeScript definition #520
-  * Project dependencies updated #524
-  * fix typescript example lockfile #526
-  * Typescript example babel loader #527
-  * Bump tar from 2.2.1 to 2.2.2 in /examples/babel #544
-  * Bump acorn from 6.3.0 to 6.4.1 in /examples/typescript #562
-  * Bump eslint-utils from 1.4.0 to 1.4.3 #567
-  * Bump handlebars from 4.1.2 to 4.7.6 #568
-  * Addressed npm security vulnerabilities #569
-  * Bump https-proxy-agent from 2.2.2 to 2.2.4 #572
-  * Bump https-proxy-agent from 2.2.2 to 2.2.4 in /examples/typescript #573
-  * Bump extend from 3.0.1 to 3.0.2 in /examples/babel #574
-  * Bump stringstream from 0.0.5 to 0.0.6 in /examples/babel #575
-  * Bump tough-cookie from 2.3.2 to 2.3.4 in /examples/babel #576
-  * Bump sshpk from 1.13.1 to 1.16.1 in /examples/babel #577
-  * Bump debug from 2.6.8 to 2.6.9 in /examples/babel #578
-
 * 5.3.1
   * Fixed bug that prevented to use handlers using import [#505][link-505]
   * Do not print empty lines in webpack stats [#499][link-499]
diff --git a/lib/packExternalModules.js b/lib/packExternalModules.js
index 3e787a8..ac7b091 100644
--- a/lib/packExternalModules.js
+++ b/lib/packExternalModules.js
@@ -94,14 +94,32 @@ function getProdModules(externalModules, packagePath, dependencyGraph, forceExcl
         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,
-            forceExcludes
-          );
-          Array.prototype.push.apply(prodModules, peerModules);
+
+          const peerDependenciesMeta = require(modulePackagePath).peerDependenciesMeta;
+
+          if (!_.isEmpty(peerDependenciesMeta)) {
+            _.forEach(peerDependencies, (value, key) => {
+              if (peerDependenciesMeta[key] && peerDependenciesMeta[key].optional === true) {
+                this.options.verbose &&
+                  this.serverless.cli.log(
+                    `Skipping peers dependency ${key} for dependency ${module.external} because it's optional`
+                  );
+
+                _.unset(peerDependencies, key);
+              }
+            });
+          }
+
+          if (!_.isEmpty(peerDependencies)) {
+            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}`);
@@ -424,3 +442,4 @@ module.exports = {
     });
   }
 };
+
diff --git a/lib/validate.js b/lib/validate.js
index 72d09b9..b26bce3 100644
--- a/lib/validate.js
+++ b/lib/validate.js
@@ -145,11 +145,6 @@ module.exports = {
           filename: '[name].js'
         };
       }
-      
-      // Default node
-      if (!this.webpackConfig.node || _.isEmpty(this.webpackConfig.node)) {
-        this.webpackConfig.node = false;
-      }
 
       // Custom output path
       if (this.options.out) {


Test patch:
diff --git a/tests/data/rp-package-optional.json b/tests/data/rp-package-optional.json
new file mode 100644
index 0000000..f193d5c
--- /dev/null
+++ b/tests/data/rp-package-optional.json
@@ -0,0 +1,94 @@
+{
+  "_from": "request-promise",
+  "_id": "request-promise@4.2.1",
+  "_inBundle": false,
+  "_integrity": "sha1-fuxWyJMXqCLL/qmbA5zlQ8LhX2c=",
+  "_location": "/request-promise",
+  "_phantomChildren": {},
+  "_requested": {
+    "type": "tag",
+    "registry": true,
+    "raw": "request-promise",
+    "name": "request-promise",
+    "escapedName": "request-promise",
+    "rawSpec": "",
+    "saveSpec": null,
+    "fetchSpec": "latest"
+  },
+  "_requiredBy": [
+    "#USER",
+    "/"
+  ],
+  "_resolved": "https://registry.npmjs.org/request-promise/-/request-promise-4.2.1.tgz",
+  "_shasum": "7eec56c89317a822cbfea99b039ce543c2e15f67",
+  "_spec": "request-promise",
+  "_where": "C:\\Projects\\serverless\\test\\babel-dynamically-entries",
+  "author": {
+    "name": "Nicolai Kamenzky",
+    "url": "https://github.com/analog-nico"
+  },
+  "bugs": {
+    "url": "https://github.com/request/request-promise/issues"
+  },
+  "bundleDependencies": false,
+  "dependencies": {
+    "bluebird": "^3.5.0",
+    "request-promise-core": "1.1.1",
+    "stealthy-require": "^1.1.0",
+    "tough-cookie": ">=2.3.0"
+  },
+  "deprecated": false,
+  "description": "The simplified HTTP request client 'request' with Promise support. Powered by Bluebird.",
+  "devDependencies": {
+    "body-parser": "~1.15.2",
+    "chai": "~3.5.0",
+    "chalk": "~1.1.3",
+    "gulp": "~3.9.1",
+    "gulp-coveralls": "~0.1.4",
+    "gulp-eslint": "~2.1.0",
+    "gulp-istanbul": "~1.0.0",
+    "gulp-mocha": "~2.2.0",
+    "lodash": "~4.13.1",
+    "publish-please": "~2.1.4",
+    "request": "^2.34.0",
+    "rimraf": "~2.5.3",
+    "run-sequence": "~1.2.2"
+  },
+  "engines": {
+    "node": ">=0.10.0"
+  },
+  "homepage": "https://github.com/request/request-promise#readme",
+  "keywords": [
+    "xhr",
+    "http",
+    "https",
+    "promise",
+    "request",
+    "then",
+    "thenable",
+    "bluebird"
+  ],
+  "license": "ISC",
+  "main": "./lib/rp.js",
+  "name": "request-promise",
+  "peerDependencies": {
+    "request": "^2.34",
+    "canvas": "^2.5.0"
+  },
+  "peerDependenciesMeta": {
+    "canvas": {
+      "optional": true
+    }
+  },
+  "repository": {
+    "type": "git",
+    "url": "git+https://github.com/request/request-promise.git"
+  },
+  "scripts": {
+    "prepublish": "publish-please guard",
+    "publish-please": "publish-please",
+    "test": "gulp ci",
+    "test-publish": "gulp ci-no-cov"
+  },
+  "version": "4.2.1"
+}
diff --git a/tests/packExternalModules.test.js b/tests/packExternalModules.test.js
index 2fb9a88..b1abd43 100644
--- a/tests/packExternalModules.test.js
+++ b/tests/packExternalModules.test.js
@@ -1114,107 +1114,222 @@ describe('packExternalModules', () => {
     });
 
     describe('peer dependencies', () => {
-      before(() => {
-        const peerDepPackageJson = require('./data/package-peerdeps.json');
-        mockery.deregisterMock(path.join(process.cwd(), 'package.json'));
-        mockery.registerMock(path.join(process.cwd(), 'package.json'), peerDepPackageJson);
-        // Mock request-promise package.json
-        const rpPackageJson = require('./data/rp-package.json');
-        const rpPackagePath = path.join(process.cwd(), 'node_modules', 'request-promise', 'package.json');
-        mockery.registerMock(rpPackagePath, rpPackageJson);
-      });
+      /**
+       * Both "default" & "optinal" behaviors are mostly equal.
+       * The only difference between each scenario is they don't use the same package.json as mock
+       */
+      describe('default behavior', () => {
+        before(() => {
+          const peerDepPackageJson = require('./data/package-peerdeps.json');
+          mockery.deregisterMock(path.join(process.cwd(), 'package.json'));
+          mockery.registerMock(path.join(process.cwd(), 'package.json'), peerDepPackageJson);
+          // Mock request-promise package.json
+          const rpPackageJson = require('./data/rp-package.json');
+          const rpPackagePath = path.join(process.cwd(), 'node_modules', 'request-promise', 'package.json');
+          mockery.registerMock(rpPackagePath, rpPackageJson);
+        });
 
-      after(() => {
-        mockery.deregisterMock(path.join(process.cwd(), 'package.json'));
-        mockery.registerMock(path.join(process.cwd(), 'package.json'), packageMock);
-        const rpPackagePath = path.join(process.cwd(), 'node_modules', 'request-promise', 'package.json');
-        mockery.deregisterMock(rpPackagePath);
-      });
+        after(() => {
+          mockery.deregisterMock(path.join(process.cwd(), 'package.json'));
+          mockery.registerMock(path.join(process.cwd(), 'package.json'), packageMock);
+          const rpPackagePath = path.join(process.cwd(), 'node_modules', 'request-promise', 'package.json');
+          mockery.deregisterMock(rpPackagePath);
+        });
 
-      it('should install external peer dependencies', () => {
-        const expectedCompositePackageJSON = {
-          name: 'test-service',
-          version: '1.0.0',
-          description: 'Packaged externals for test-service',
-          private: true,
-          scripts: {},
-          dependencies: {
-            bluebird: '^3.5.0',
-            'request-promise': '^4.2.1',
-            request: '^2.82.0'
-          }
-        };
-        const expectedPackageJSON = {
-          name: 'test-service',
-          version: '1.0.0',
-          description: 'Packaged externals for test-service',
-          private: true,
-          scripts: {},
-          dependencies: {
-            bluebird: '^3.5.0',
-            'request-promise': '^4.2.1',
-            request: '^2.82.0'
-          }
-        };
+        it('should install external peer dependencies', () => {
+          const expectedCompositePackageJSON = {
+            name: 'test-service',
+            version: '1.0.0',
+            description: 'Packaged externals for test-service',
+            private: true,
+            scripts: {},
+            dependencies: {
+              bluebird: '^3.5.0',
+              'request-promise': '^4.2.1',
+              request: '^2.82.0'
+            }
+          };
+          const expectedPackageJSON = {
+            name: 'test-service',
+            version: '1.0.0',
+            description: 'Packaged externals for test-service',
+            private: true,
+            scripts: {},
+            dependencies: {
+              bluebird: '^3.5.0',
+              'request-promise': '^4.2.1',
+              request: '^2.82.0'
+            }
+          };
 
-        const dependencyGraph = require('./data/npm-ls-peerdeps.json');
-        const peerDepStats = {
-          stats: [
-            {
-              compilation: {
-                chunks: [
-                  new ChunkMock([
-                    {
-                      identifier: _.constant('"crypto"')
-                    },
-                    {
-                      identifier: _.constant('"uuid/v4"')
-                    },
-                    {
-                      identifier: _.constant('"mockery"')
-                    },
-                    {
-                      identifier: _.constant('"@scoped/vendor/module1"')
-                    },
-                    {
-                      identifier: _.constant('external "bluebird"')
-                    },
-                    {
-                      identifier: _.constant('external "request-promise"')
-                    }
-                  ])
-                ],
-                compiler: {
-                  outputPath: '/my/Service/Path/.webpack/service'
+          const dependencyGraph = require('./data/npm-ls-peerdeps.json');
+          const peerDepStats = {
+            stats: [
+              {
+                compilation: {
+                  chunks: [
+                    new ChunkMock([
+                      {
+                        identifier: _.constant('"crypto"')
+                      },
+                      {
+                        identifier: _.constant('"uuid/v4"')
+                      },
+                      {
+                        identifier: _.constant('"mockery"')
+                      },
+                      {
+                        identifier: _.constant('"@scoped/vendor/module1"')
+                      },
+                      {
+                        identifier: _.constant('external "bluebird"')
+                      },
+                      {
+                        identifier: _.constant('external "request-promise"')
+                      }
+                    ])
+                  ],
+                  compiler: {
+                    outputPath: '/my/Service/Path/.webpack/service'
+                  }
                 }
               }
+            ]
+          };
+
+          module.webpackOutputPath = 'outputPath';
+          fsExtraMock.pathExists.yields(null, false);
+          fsExtraMock.copy.yields();
+          packagerMock.getProdDependencies.returns(BbPromise.resolve(dependencyGraph));
+          packagerMock.install.returns(BbPromise.resolve());
+          packagerMock.prune.returns(BbPromise.resolve());
+          packagerMock.runScripts.returns(BbPromise.resolve());
+          module.compileStats = peerDepStats;
+          return expect(module.packExternalModules()).to.be.fulfilled.then(() =>
+            BbPromise.all([
+              // 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(
+                JSON.stringify(expectedCompositePackageJSON, null, 2)
+              ),
+              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(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
+            ])
+          );
+        });
+      });
+
+      describe('optional behavior', () => {
+        before(() => {
+          const peerDepPackageJson = require('./data/package-peerdeps.json');
+          mockery.deregisterMock(path.join(process.cwd(), 'package.json'));
+          mockery.registerMock(path.join(process.cwd(), 'package.json'), peerDepPackageJson);
+          // Mock request-promise package.json
+          const rpPackageJson = require('./data/rp-package-optional.json');
+          const rpPackagePath = path.join(process.cwd(), 'node_modules', 'request-promise', 'package.json');
+          mockery.registerMock(rpPackagePath, rpPackageJson);
+        });
+
+        after(() => {
+          mockery.deregisterMock(path.join(process.cwd(), 'package.json'));
+          mockery.registerMock(path.join(process.cwd(), 'package.json'), packageMock);
+          const rpPackagePath = path.join(process.cwd(), 'node_modules', 'request-promise', 'package.json');
+          mockery.deregisterMock(rpPackagePath);
+        });
+
+        it('should skip optional peer dependencies', () => {
+          const expectedCompositePackageJSON = {
+            name: 'test-service',
+            version: '1.0.0',
+            description: 'Packaged externals for test-service',
+            private: true,
+            scripts: {},
+            dependencies: {
+              bluebird: '^3.5.0',
+              'request-promise': '^4.2.1',
+              request: '^2.82.0'
+            }
+          };
+          const expectedPackageJSON = {
+            name: 'test-service',
+            version: '1.0.0',
+            description: 'Packaged externals for test-service',
+            private: true,
+            scripts: {},
+            dependencies: {
+              bluebird: '^3.5.0',
+              'request-promise': '^4.2.1',
+              request: '^2.82.0'
             }
-          ]
-        };
+          };
 
-        module.webpackOutputPath = 'outputPath';
-        fsExtraMock.pathExists.yields(null, false);
-        fsExtraMock.copy.yields();
-        packagerMock.getProdDependencies.returns(BbPromise.resolve(dependencyGraph));
-        packagerMock.install.returns(BbPromise.resolve());
-        packagerMock.prune.returns(BbPromise.resolve());
-        packagerMock.runScripts.returns(BbPromise.resolve());
-        module.compileStats = peerDepStats;
-        return expect(module.packExternalModules()).to.be.fulfilled.then(() =>
-          BbPromise.all([
-            // 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(JSON.stringify(expectedCompositePackageJSON, null, 2)),
-            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(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
-          ])
-        );
+          const dependencyGraph = require('./data/npm-ls-peerdeps.json');
+          const peerDepStats = {
+            stats: [
+              {
+                compilation: {
+                  chunks: [
+                    new ChunkMock([
+                      {
+                        identifier: _.constant('"crypto"')
+                      },
+                      {
+                        identifier: _.constant('"uuid/v4"')
+                      },
+                      {
+                        identifier: _.constant('"mockery"')
+                      },
+                      {
+                        identifier: _.constant('"@scoped/vendor/module1"')
+                      },
+                      {
+                        identifier: _.constant('external "bluebird"')
+                      },
+                      {
+                        identifier: _.constant('external "request-promise"')
+                      }
+                    ])
+                  ],
+                  compiler: {
+                    outputPath: '/my/Service/Path/.webpack/service'
+                  }
+                }
+              }
+            ]
+          };
+
+          module.webpackOutputPath = 'outputPath';
+          fsExtraMock.pathExists.yields(null, false);
+          fsExtraMock.copy.yields();
+          packagerMock.getProdDependencies.returns(BbPromise.resolve(dependencyGraph));
+          packagerMock.install.returns(BbPromise.resolve());
+          packagerMock.prune.returns(BbPromise.resolve());
+          packagerMock.runScripts.returns(BbPromise.resolve());
+          module.compileStats = peerDepStats;
+          return expect(module.packExternalModules()).to.be.fulfilled.then(() =>
+            BbPromise.all([
+              // 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(
+                JSON.stringify(expectedCompositePackageJSON, null, 2)
+              ),
+              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(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
+            ])
+          );
+        });
       });
     });
   });
diff --git a/tests/validate.test.js b/tests/validate.test.js
index 96ae118..df62757 100644
--- a/tests/validate.test.js
+++ b/tests/validate.test.js
@@ -212,21 +212,6 @@ describe('validate', () => {
     });
   });
 
-  describe('default node', () => {
-    it('should turn NodeStuffPlugin and NodeSourcePlugin plugins off by default', () => {
-      const testEntry = 'testentry';
-      const testConfig = {
-        entry: testEntry,
-      };
-      const testServicePath = 'testpath';
-      module.serverless.config.servicePath = testServicePath;
-      _.set(module.serverless.service, 'custom.webpack.config', testConfig);
-      return module
-        .validate()
-        .then(() => expect(module.webpackConfig.node).to.eql(false));
-    });
-  });
-
   describe('config file load', () => {
     it('should load a webpack config from file if `custom.webpack` is a string', () => {
       const testConfig = 'testconfig';
@@ -894,4 +879,3 @@ describe('validate', () => {
     });
   });
 });
-