Repository: serverless-heaven/serverless-webpack
Pull request #541 (2019-09-30 11:44:42)

Fix external modules version for transitive dependencies
<!--
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
-->

**This pull request is the same change as #507** plus tests added. I also published a repo with a minimal reproduction case: https://github.com/salomvary/serverless-webpack-bug/tree/master/comp-a (run `yarn && node_modules/.bin/serverless package`).

## What did you implement:

Same as in #507.

<!--
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.
-->
Same as in #507.

## 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
-->

Same as in #507.

## Todos:

- [x] Write tests
- [ ] Write documentation
- [x] Fix linting errors
- [x] Make sure code coverage hasn't dropped
- [ ] 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


salomvary: Sadly, this only solves problem of the missing versions in the generated package.json and eliminates "WARNING: Could not determine version of module XXX" but does not fully solve the problem of fully supporting yarn + multiple modules. The next failure I am getting is:

```
  Error --------------------------------------------------
 
  yarn install --frozen-lockfile --non-interactive failed with code 1
  error Your lockfile needs to be updated, but yarn was run with `--frozen-lockfile`.
```

The reason for the frozen lockfile problem is the following. The generated `.webpack/dependencies/package.json` looks like this (using the exampel repo linked above):

```
{
  "name": "comp-a-service",
  "version": "1.0.0",
  "description": "Packaged externals for comp-a-service",
  "private": true,
  "scripts": {},
  "dependencies": {
    "classnames": "2.2.6"
  }
}
```

And `.webpack/dependencies/yarn.lock` contains this (among a long list of irrelevant devDependencies):

```
classnames@^2.2.6:
  version "2.2.6"
  resolved "https://registry.yarnpkg.com/classnames/-/classnames-2.2.6.tgz#43935bffdd291f326dad0a205309b38d00f650ce"
  integrity sha512-JR/iSQOSt+LQIWwrwEzJ9uk0xfN3mTVYMwt1Ir5mUcSN6pU+V4zQFFaJsclJbPuAUQH+yfWef6tm7l1quW3C8Q==
```

Now if I change `"classnames": "2.2.6"` in `package.json` to `"classnames": "^2.2.6"` it will make `yarn install --frozen-lockfile --non-interactive` no longer fail. Kind of makes sense as this exact version specifier is what we see in `yarn.lock` and also how it was declared in the original [`package.lock` of the dependent module](https://github.com/salomvary/serverless-webpack-bug/blob/master/comp-c/package.json#L7). Not sure how to proceed from here though, my relationship with yarn/npm/webpack/serverless-webpack is not intimate enough to see any obvious solution...
salomvary: @HyperBrain What do you think? Is it worth merging a partial fix? Do you have any ideas for a proper solution? I'm kind of stuck here and falling back to ugly workarounds.
salomvary: I investigated a little further where the `dependency@^version` vs `dependency@version` difference comes from. The [`yarn list` command invoked](https://github.com/serverless-heaven/serverless-webpack/blob/master/lib/packagers/yarn.js#L31) results in a shallow tree not including the original version specifier at all:

```
$ yarn list --depth=1 --production --json
{
  "type": "tree",
  "data": {
    "type": "list",
    "trees": [
      {
        "name": "comp-b@1.0.0",
        "children": [],
        "hint": null,
        "color": "bold",
        "depth": 0
      },
      {
        "name": "comp-c@1.0.0",
        "children": [],
        "hint": null,
        "color": "bold",
        "depth": 0
      },
      {
        "name": "classnames@2.2.6",
        "children": [],
        "hint": null,
        "color": null,
        "depth": 0
      }
    ]
  }
}
```

Whereas not limiting depth gives us the original version further deep at the tree:

```
$ yarn list  --production --json
{
  "type": "tree",
  "data": {
    "type": "list",
    "trees": [
      {
        "name": "comp-b@1.0.0",
        "children": [
          {
            "name": "comp-c@file:../comp-c",
            "color": "dim",
            "shadow": true
          }
        ],
        "hint": null,
        "color": "bold",
        "depth": 0
      },
      {
        "name": "comp-c@1.0.0",
        "children": [
          {
            "name": "classnames@^2.2.6",
            "color": "dim",
            "shadow": true
          }
        ],
        "hint": null,
        "color": "bold",
        "depth": 0
      },
      {
        "name": "classnames@2.2.6",
        "children": [],
        "hint": null,
        "color": null,
        "depth": 0
      }
    ]
  }
}
```

Could the solution be removing the depth limitation? What was the original purpose of `depth=1`?
MrLoh: any chance to move this along, I'm running into the same issue. 

@salomvary what is your current workaround?
salomvary: @MrLoh I'm using a forked version of `serverless-webpack` with this pull request merged into it.
j0k3r: > Could the solution be removing the depth limitation? What was the original purpose of `depth=1`?

As far as I can understand, it might be to avoid having to retrieve a huge object because of all the subdeps.
Also, I've just ran each command on a real project and got twice thinner JSON object when using `depth=1`
roni-frantchi: Using this PR plus bundling the Yarn Workspaces packages has **resolved** the both the ``WARNING: Could not determine version of module`` and `Your lockfile needs to be updated, but yarn was run with `--frozen-lockfile` for us.  

So for instance, if all your monorepo packages are scoped with `@acme`:

```json5
{ // webpack.config.js
  externals: [nodeExternals({ allowlist: [/^@acme/] })],
}
```

Any chance to move this PR along?...

Patch:
diff --git a/lib/packExternalModules.js b/lib/packExternalModules.js
index 8ae24e6..96b4a4c 100644
--- a/lib/packExternalModules.js
+++ b/lib/packExternalModules.js
@@ -129,6 +129,10 @@ function getProdModules(externalModules, packagePath, dependencyGraph, forceExcl
         // 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) {
+          // eslint-disable-next-line lodash/path-style 
+          moduleVersion = _.get(dependencyGraph, [ 'dependencies', module.external, 'version' ]);
+        }
         if (!moduleVersion) {
           this.serverless.cli.log(`WARNING: Could not determine version of module ${module.external}`);
         }


Test patch:
diff --git a/tests/data/package-transitive.json b/tests/data/package-transitive.json
new file mode 100644
index 0000000..a211136
--- /dev/null
+++ b/tests/data/package-transitive.json
@@ -0,0 +1,22 @@
+{
+  "name": "comp-a",
+  "version": "1.0.0",
+  "description": "",
+  "main": "index.js",
+  "dependencies": {
+    "comp-b": "file:../comp-b",
+    "comp-c": "file:../comp-c"
+  },
+  "scripts": {
+    "test": "echo \"Error: no test specified\" && exit 1"
+  },
+  "keywords": [],
+  "author": "",
+  "license": "ISC",
+  "devDependencies": {
+    "serverless": "^1.53.0",
+    "serverless-webpack": "^5.3.1",
+    "webpack": "^4.41.0",
+    "webpack-node-externals": "^1.7.2"
+  }
+}
diff --git a/tests/packExternalModules.test.js b/tests/packExternalModules.test.js
index 236ea59..6999994 100644
--- a/tests/packExternalModules.test.js
+++ b/tests/packExternalModules.test.js
@@ -1350,5 +1350,93 @@ describe('packExternalModules', () => {
         });
       });
     });
+
+    describe('transitive dependencies', () => {
+      before(() => {
+        const transitivePackageJson = require('./data/package-transitive.json');
+        mockery.deregisterMock(path.join(process.cwd(), 'package.json'));
+        mockery.registerMock(path.join(process.cwd(), 'package.json'), transitivePackageJson);
+      });
+
+      after(() => {
+        mockery.deregisterMock(path.join(process.cwd(), 'package.json'));
+        mockery.registerMock(path.join(process.cwd(), 'package.json'), packageMock);
+      });
+
+      it('should install transitive dependencies', () => {
+        const expectedCompositePackageJSON = {
+          name: 'test-service',
+          version: '1.0.0',
+          description: 'Packaged externals for test-service',
+          private: true,
+          scripts: {},
+          dependencies: {
+            classnames: '2.2.6'
+          }
+        };
+        const expectedPackageJSON = {
+          name: 'test-service',
+          version: '1.0.0',
+          description: 'Packaged externals for test-service',
+          private: true,
+          scripts: {},
+          dependencies: {
+            classnames: '2.2.6'
+          }
+        };
+
+        const dependencyGraph = {
+          problems: [],
+          dependencies: {
+            'comp-b': { version: '1.0.0', dependencies: {} },
+            'comp-c': { version: '1.0.0', dependencies: {} },
+            classnames: { version: '2.2.6', dependencies: {} }
+          }
+        };
+
+        const transitiveDepStats = {
+          stats: [
+            {
+              compilation: {
+                chunks: [
+                  new ChunkMock([
+                    {
+                      identifier: _.constant('external "classnames"')
+                    }
+                  ])
+                ],
+                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 = transitiveDepStats;
+        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
+          ])
+        );
+      });
+    });
   });
 });