Repository: serverless-heaven/serverless-webpack (js)

Issue:
yarn resolutions is not respected
# This is a Bug Report

## Description
I use yarn as my packager for serverless-webpack. In my `package.json` I have defined `resolutions` to force yarn to use a specific version of a dependency. This works quite well in my project, but it won't get applied when I run `sls webpack`.

In my output folder of `serverless-webpack` (.webpack/<fn>) is a package.json without the resolutions field. If I run `yarn list <package>` I get multiple entries of this dependency where I expected exactly one. If I run `yarn install --frozen-lockfile` I get `Lockfile has incorrect entry for "graphql@^0.11.7". Ignoring it.` which leads to this missbehavior.

This is my serverless-webpack related config:
```
custom: 
  webpack:
    includeModules: true
    packager: yarn
```

If I add `resolutions` to package.json it works as it should.

This may be a bug within yarn itself, because I expect when using `--frozen-lockfile` it should take whats in the lockfile. 

Versions:
serverless-webpack@5.1.3
webpack@4.6.0
serverless@1.26.1
yarn@1.6.0

Thanks!

Pull request description:
Use package json sections defined by packagers
<!--
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:

Closes #379 

<!--
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.
-->

## 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.
-->

The packager implementations now can specify a list of sections in package.json, that will be copied from the original package.json to the ones created for deployment.
For now, only Yarn/resolutions will be added.

## 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
-->

Use a project setup like the one mentioned in #379. The generated package.json files in `.webpack/*` should contain a resolution section if one is defined in the project package.json.

## 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 66db7ad..a060458 100644
--- a/lib/packExternalModules.js
+++ b/lib/packExternalModules.js
@@ -193,6 +193,14 @@ module.exports = {
     // Determine and create packager
     return BbPromise.try(() => Packagers.get.call(this, this.configuration.packager))
     .then(packager => {
+      // Fetch needed original package.json sections
+      const sectionNames = packager.copyPackageSectionNames;
+      const packageJson = this.serverless.utils.readFileSync(packageJsonPath);
+      const packageSections = _.pick(packageJson, sectionNames);
+      if (!_.isEmpty(packageSections)) {
+        this.options.verbose && this.serverless.cli.log(`Using package.json sections ${_.join(_.keys(packageSections), ', ')}`);
+      }
+
       // Get first level dependency graph
       this.options.verbose && this.serverless.cli.log(`Fetch dependency graph from ${packageJsonPath}`);
 
@@ -227,13 +235,13 @@ module.exports = {
         const compositePackageJson = path.join(compositeModulePath, 'package.json');
   
         // (1.a.1) Create a package.json
-        const compositePackage = {
+        const compositePackage = _.defaults({
           name: this.serverless.service.service,
           version: '1.0.0',
           description: `Packaged externals for ${this.serverless.service.service}`,
           private: true,
           scripts: packageScripts
-        };
+        }, packageSections);
         const relPath = path.relative(compositeModulePath, path.dirname(packageJsonPath));
         addModulesToPackageJson(compositeModules, compositePackage, relPath);
         this.serverless.utils.writeFileSync(compositePackageJson, JSON.stringify(compositePackage, null, 2));
@@ -247,13 +255,6 @@ module.exports = {
             this.serverless.cli.log('Package lock found - Using locked versions');
             try {
               let packageLockFile = this.serverless.utils.readFileSync(packageLockPath);
-              /**
-               * We should not be modifying 'package-lock.json'
-               * because this file should be treat as internal to npm.
-               * 
-               * Rebase package-lock is a temporary workaround and must be
-               * removed as soon as https://github.com/npm/npm/issues/19183 gets fixed.
-               */
               packageLockFile = packager.rebaseLockfile(relPath, packageLockFile);
               if (_.isObject(packageLockFile)) {
                 packageLockFile = JSON.stringify(packageLockFile, null, 2);
@@ -279,14 +280,14 @@ module.exports = {
   
           // Create package.json
           const modulePackageJson = path.join(modulePath, 'package.json');
-          const modulePackage = {
+          const modulePackage = _.defaults({
             name: this.serverless.service.service,
             version: '1.0.0',
             description: `Packaged externals for ${this.serverless.service.service}`,
             private: true,
             scripts: packageScripts,
             dependencies: {}
-          };
+          }, packageSections);
           const prodModules = getProdModules.call(this,
             _.concat(
               getExternalModules.call(this, compileStats),
diff --git a/lib/packagers/index.js b/lib/packagers/index.js
index 7f0ebd1..ff02125 100644
--- a/lib/packagers/index.js
+++ b/lib/packagers/index.js
@@ -7,6 +7,8 @@
  * interface Packager {
  * 
  * static get lockfileName(): string;
+ * static get copyPackageSectionNames(): Array<string>;
+ * static get mustCopyModules(): boolean;
  * static getProdDependencies(cwd: string, depth: number = 1): BbPromise<Object>;
  * static rebaseLockfile(pathToPackageRoot: string, lockfile: Object): void;
  * static install(cwd: string): BbPromise<void>;
diff --git a/lib/packagers/npm.js b/lib/packagers/npm.js
index 443f7b9..8cb0393 100644
--- a/lib/packagers/npm.js
+++ b/lib/packagers/npm.js
@@ -12,6 +12,10 @@ class NPM {
     return 'package-lock.json';
   }
 
+  static get copyPackageSectionNames() {
+    return [];
+  }
+
   static get mustCopyModules() {  // eslint-disable-line lodash/prefer-constant
     return true;
   }
@@ -66,6 +70,13 @@ class NPM {
     return moduleVersion;
   }
   
+  /**
+   * We should not be modifying 'package-lock.json'
+   * because this file should be treated as internal to npm.
+   * 
+   * Rebase package-lock is a temporary workaround and must be
+   * removed as soon as https://github.com/npm/npm/issues/19183 gets fixed.
+   */
   static rebaseLockfile(pathToPackageRoot, lockfile) {
     if (lockfile.version) {
       lockfile.version = NPM._rebaseFileReferences(pathToPackageRoot, lockfile.version);
diff --git a/lib/packagers/yarn.js b/lib/packagers/yarn.js
index bcd961a..2c2b45f 100644
--- a/lib/packagers/yarn.js
+++ b/lib/packagers/yarn.js
@@ -16,6 +16,10 @@ class Yarn {
     return 'yarn.lock';
   }
 
+  static get copyPackageSectionNames() {
+    return ['resolutions'];
+  }
+
   static get mustCopyModules() {  // eslint-disable-line lodash/prefer-constant
     return false;
   }


Test patch:
diff --git a/lib/packagers/npm.test.js b/lib/packagers/npm.test.js
index 107a513..8ecc811 100644
--- a/lib/packagers/npm.test.js
+++ b/lib/packagers/npm.test.js
@@ -38,6 +38,10 @@ describe('npm', () => {
     expect(npmModule.lockfileName).to.equal('package-lock.json');
   });
 
+  it('should return no packager sections', () => {
+    expect(npmModule.copyPackageSectionNames).to.be.an('array').that.is.empty;
+  });
+
   it('requires to copy modules', () => {
     expect(npmModule.mustCopyModules).to.be.true;
   });
diff --git a/lib/packagers/yarn.test.js b/lib/packagers/yarn.test.js
index a20edb7..0907207 100644
--- a/lib/packagers/yarn.test.js
+++ b/lib/packagers/yarn.test.js
@@ -37,6 +37,10 @@ describe('yarn', () => {
     expect(yarnModule.lockfileName).to.equal('yarn.lock');
   });
 
+  it('should return packager sections', () => {
+    expect(yarnModule.copyPackageSectionNames).to.deep.equal(['resolutions']);
+  });
+
   it('does not require to copy modules', () => {
     expect(yarnModule.mustCopyModules).to.be.false;
   });
diff --git a/tests/packExternalModules.test.js b/tests/packExternalModules.test.js
index bce1c99..af62394 100644
--- a/tests/packExternalModules.test.js
+++ b/tests/packExternalModules.test.js
@@ -33,6 +33,7 @@ const packagerMockFactory = {
   create(sandbox) {
     const packagerMock = {
       lockfileName: 'mocked-lock.json',
+      copyPackageSectionNames: [ 'section1', 'section2' ],
       mustCopyModules: true,
       rebaseLockfile: sandbox.stub(),
       getProdDependencies: sandbox.stub(),
@@ -243,6 +244,75 @@ describe('packExternalModules', () => {
       ]));
     });
 
+    it('should copy needed package sections if available', () => {
+      const originalPackageJSON = {
+        name: 'test-service',
+        version: '1.0.0',
+        description: 'Packaged externals for test-service',
+        private: true,
+        scripts: {},
+        section1: {
+          value: 'myValue'          
+        },
+        dependencies: {
+          '@scoped/vendor': '1.0.0',
+          uuid: '^5.4.1',
+          bluebird: '^3.4.0'
+        }
+      };
+      const expectedCompositePackageJSON = {
+        name: 'test-service',
+        version: '1.0.0',
+        description: 'Packaged externals for test-service',
+        private: true,
+        scripts: {},
+        section1: originalPackageJSON.section1,
+        dependencies: {
+          '@scoped/vendor': '1.0.0',
+          uuid: '^5.4.1',
+          bluebird: '^3.4.0'
+        }
+      };
+      const expectedPackageJSON = {
+        name: 'test-service',
+        version: '1.0.0',
+        description: 'Packaged externals for test-service',
+        private: true,
+        scripts: {},
+        dependencies: {
+          '@scoped/vendor': '1.0.0',
+          uuid: '^5.4.1',
+          bluebird: '^3.4.0'
+        },
+        section1: originalPackageJSON.section1
+      };
+
+      module.webpackOutputPath = 'outputPath';
+      readFileSyncStub.onFirstCall().returns(originalPackageJSON);
+      readFileSyncStub.throws(new Error('Unexpected call to readFileSync'));
+      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 = stats;
+      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,
+      ]));
+    });
+
     it('should install external modules', () => {
       const expectedCompositePackageJSON = {
         name: 'test-service',
@@ -349,6 +419,7 @@ describe('packExternalModules', () => {
         }
       });
       module.webpackOutputPath = 'outputPath';
+      readFileSyncStub.onFirstCall().returns(packageLocalRefMock);
       readFileSyncStub.returns(fakePackageLockJSON);
       fsExtraMock.pathExists.yields(null, true);
       fsExtraMock.copy.yields();
@@ -718,6 +789,7 @@ describe('packExternalModules', () => {
       module.webpackOutputPath = 'outputPath';
       fsExtraMock.pathExists.yields(null, true);
       fsExtraMock.copy.yields();
+      readFileSyncStub.onFirstCall().returns(packageMock);
       readFileSyncStub.returns({ info: 'lockfile' });
       packagerMock.rebaseLockfile.callsFake((pathToPackageRoot, lockfile) => lockfile);
       packagerMock.getProdDependencies.returns(BbPromise.resolve({}));
@@ -769,6 +841,7 @@ describe('packExternalModules', () => {
       };
 
       module.webpackOutputPath = 'outputPath';
+      readFileSyncStub.onFirstCall().returns(packageMock);
       readFileSyncStub.throws(new Error('Failed to read package-lock.json'));
       fsExtraMock.pathExists.yields(null, true);
       fsExtraMock.copy.onFirstCall().yields();