Exclude Node.js dev dependencies in .zip file by pmuens · Pull Request #3737 · serverless/serverless

pmuens · 2017-06-06T11:26:11Z

What did you implement:

Exclude the dev node_modules when zipping to decrease the bundle size.

How did you implement it:

Get a list of the production node_modules. Add the node_modules/** glob to the exclude and re-include the production node_modules via corresponding <package-name>/** globs in the include glob.

How can we verify it:

Create a service and do a npm init.

Run npm install --save node-fetch and npm install --save-dev jest.

Run serverless package and inspect the zip file. It should only include the dependencies necessary for the node-fetch module (and not the ones for jest).

Note: please use nested directories with dependencies as well. Those should also only include the dev dependencies after packaging.

Todos:

Is this ready for review?: YES
Is it a breaking change?: NO

cjelger · 2017-06-12T13:15:53Z

lib/plugins/package/lib/zipDirectory.js

+const fileExistsSync = require('../../../utils/fs/fileExistsSync');

 module.exports = {
  zipDirectory(exclude, include, zipFileName) {


There is here a major issue if the serverless.yml file has multiple functions and each function is packaged individually. The problem is that this function is called for all serverless functions before anything is zipped, but each call to zipDirectory overrides the include, exclude, and zipFileName properties of this. This means that the last call to zipDirectory "wins" and all serverless functions end up being deployed with the code of the last data set by zipDirectory. This basically breaks the deployment if the serverless functions are packaged individually.

cjelger · 2017-06-12T13:32:06Z

lib/plugins/package/lib/zipDirectory.js

+  excludeDevDependencies() {
+    // take care of node runtime-services
+    try {
+      const packageJsonFilePath = path.join(this.serverless.config.servicePath, 'package.json');


I almost missed that one. If a serverless project has multiple functions packaged individually, it can well be that each function is developed in a nodejs sub-project. In such a project, this will refer to the package.json file of the root/parent project, which does not have the right dependencies.

We actually exactly do that: the serverless.yml is in a parent nodejs project that refers to nodejs "sub-projects", each having a separate package.json file. However there is no way to specify the root path of each subproject in the serverless.yml file, so maybe there is a need for a new property. Otherwise you will always filter the production dependencies of the parent project (usually just an empty reactor project used to build all the sub-projects). For example, we use https://github.com/lerna/lerna to do that.

pmuens · 2017-06-16T10:52:35Z

Thanks for the feedback on this one @cjelger 👍

I agree. We should support that. I'll rebase this PR and work on a fix for it the next couple of hrs.

cjelger · 2017-06-16T11:47:58Z

@pmuens in case you missed it, I also discussed that topic a bit in #2709

pmuens · 2017-06-16T14:19:30Z

Thanks for the link to the issue @cjelger 👍

I just pushed the changes. Now nested directories are supported as well out of the box.

pmuens · 2017-06-16T14:24:01Z

@cjelger Just looked into the code again and I think the issue with "the last" zip wins is still around.

Will look into it and resolve it ASAP.

pmuens · 2017-06-16T19:14:59Z

@cjelger I fixed the problem and packaging individually should be working with this PR. Would be nice if you could take another look at it. Thanks in advance! 👍

ccannell · 2017-06-20T16:15:53Z

Does this PR also remove README and package.json files? I was able to trim ~1MB from a 3.5MB zip just removing those files.

pmuens · 2017-06-20T17:41:37Z

Does this PR also remove README and package.json files? I was able to trim ~1MB from a 3.5MB zip just removing those files.

@ccannell thanks for the question. This PR will only exclude dev dependencies for Node.js files for the time being (by using npm ls behind the scenes). Good pointer with the README.md files though 👍

cjelger · 2017-06-21T12:12:06Z

@pmuens I tested your code again and I'm sorry but it still doesn't properly work for me. With the fix for Openwhisk sequences (see here, had to do that to get the latest version working with Openwhisk), each function is zipped properly but it contains the production dependencies of absolutely all node packages that were found under the service path.

pmuens · 2017-06-21T12:45:49Z

Thanks for getting back @cjelger 👍

🤔 that's strange do you have any repo with code we can look into to reproduce this? A serverless.yml with some information about the context of the service could be a good start.

@eahefnawy and I tried it with different setups and the exclusion of dev dependencies was always working on our machines. Or might this be something OpenWhisk specific?

cjelger · 2017-06-21T12:50:02Z

I cannot share the big repo I'm working on but I will create a dummy project to show you the issue. I hope I can do that by the end of the week.

ryanmurakami · 2017-06-21T23:00:25Z

@cjelger I opened an issue: #3827
Pretty sure we're experiencing the same thing.

pmuens · 2017-06-22T06:32:49Z

Thanks @cjelger and @ryanmurakami 👍

We'll look into it and work on a fix for that.

HyperBrain · 2017-06-29T21:56:45Z

lib/plugins/package/lib/zipService.js

+        .execSync('npm ls --prod=true --parseable=true --silent')
+        .toString().trim();
+
+      const prodDependencyPaths = prodDependencies.match(/(node_modules\/.*)/g);


@pmuens This does not work with Windows. \/ has to be replaced with path.sep.

The output of npm ls --prod=true --parseable=true --silent under Windows returns backslashes as path separators.

Good catch! Thanks for pointing that out @HyperBrain 👍 !

arabold · 2017-06-29T22:19:01Z

Okay, I spent about an hour digging into the root causes of why 1.16 doesn't work for us anymore. Turns out it's this piece of code:

      const prodDependencies = childProcess
        .execSync('npm ls --prod=true --parseable=true --silent')
        .toString().trim();

The resulting string is cut off after 8k of data. That's not enough to hold all dependencies with their full paths on the file system. That causes the excludeNodeDevDependencies() function to skip all other modules.

pmuens · 2017-06-30T07:00:41Z

Thanks for investigating @arabold 👍 🎉

Really helpful! Any idea how this could be solved?

arabold · 2017-06-30T15:27:53Z

I've found this (nodejs/node#3033) and this (nodejs/node#2972). Seems to be a Node issue. We're using 6.10 here.

I'm looking for a workaround now... :(

arabold · 2017-06-30T15:42:13Z

The only reliable and cross-platform way of solving this seems to be a temporary file. I don't have the time to implement a full pull request today, but here's some code that actually worked locally and returned the whole list of dependencies.

const childProcess = require('child_process');
const path = require('path');
const fs = require('fs');
const BbPromise = require('bluebird');

const outputFile = path.join('.serverless', 'deps.tmp') // FIXME Use the proper var for the temp path;
childProcess.execSync(`npm ls --prod=true --parseable=true --silent >${outputFile}`); // FIXME Error handling!
return BbPromise.fromCallback(cb => fs.readFile(outputFile, 'utf8', cb))
.then(prodDependencies => {
  console.log(prodDependencies);
  console.log(`${prodDependencies.length} chars`);
});

pmuens · 2017-06-30T15:47:23Z

Awesome! Thanks for looking into this @arabold! 👍

This really helps a lot! We'll try to implement this in the upcoming week 🎉
Thanks for posting the code snippet!

Serverless 0.16.0 introduced a feature which excludes Node.js dev dependencies from .zip files. The feature uses package.json to find which dependencies can be excluded. serverless/serverless#3737 This commit adds the package.json to the .build folder, so that Serverless can find the package.json file.

ttarhan · 2017-07-24T00:46:46Z

Hi All,

I ran into an issue with this patch: if there are no prod dependencies, then the entire node_modules directory is included. This appears to be intentional, based on theif (includePatterns.length) check inside excludeNodeDevDependencies.

I'm happy to provide a patch that works through this, but didn't want to do that if this was the desired behavior.

pmuens · 2017-07-24T07:48:42Z

Hey @ttarhan thanks for commenting and thanks for offering help here! 👍

We've updated the exclusion of dev dependencies recently (see: #3889).

Which Serverless version are you running? Is this still the case when using the updated dev dependency exclusion?

ttarhan · 2017-07-24T17:02:52Z

I tested with 1.17; looks like #3889 is newer, and I haven't tried that yet.

pmuens · 2017-07-25T06:19:21Z

I tested with 1.17; looks like #3889 is newer, and I haven't tried that yet.

Thanks for getting back @ttarhan 👍

It would be nice if you could test it with v1.18 to see if this was resolved in the meantime. Could you please open up a new issue if this is still around after v1.18 so that we can discuss it separately?

Thanks in advance!

pmuens added the stage/in-progress label Jun 6, 2017

pmuens added this to the 1.16 milestone Jun 6, 2017

pmuens mentioned this pull request Jun 6, 2017

DX: Zip only production relevant node_modules to decrease bundle size #2709

Closed

pmuens force-pushed the exclude-dev-dependencies-in-zip branch 4 times, most recently from 98e4c1f to 7c631be Compare June 7, 2017 11:34

pmuens added stage/in-review and removed stage/in-progress labels Jun 7, 2017

pmuens force-pushed the exclude-dev-dependencies-in-zip branch 3 times, most recently from 65d9087 to 5ce023a Compare June 12, 2017 08:47

cjelger reviewed Jun 12, 2017

View reviewed changes

pmuens added stage/in-progress and removed stage/in-review labels Jun 16, 2017

pmuens force-pushed the exclude-dev-dependencies-in-zip branch from 5ce023a to 26e8881 Compare June 16, 2017 14:16

pmuens added stage/in-review and removed stage/in-progress labels Jun 16, 2017

pmuens added stage/in-progress and removed stage/in-review labels Jun 16, 2017

pmuens force-pushed the exclude-dev-dependencies-in-zip branch 2 times, most recently from 430d180 to 40674e8 Compare June 16, 2017 16:07

Exclude dev dependencies in .zip file

9f67a39

pmuens force-pushed the exclude-dev-dependencies-in-zip branch from 40674e8 to 9f67a39 Compare June 16, 2017 18:30

eahefnawy approved these changes Jun 21, 2017

View reviewed changes

eahefnawy merged commit 7a8e1a9 into master Jun 21, 2017

pmuens deleted the exclude-dev-dependencies-in-zip branch June 21, 2017 08:13

This was referenced Jun 27, 2017

How to deploy and stay under AWS Lambda size limits? adieuadieu/serverless-chrome#28

Closed

Refactor into an npm package, serverless-plugin, and provide example handlers as individual Serverless functions adieuadieu/serverless-chrome#4

Closed

HyperBrain mentioned this pull request Jun 27, 2017

Bundling of node modules works oddly serverless-heaven/serverless-webpack#117

Closed

pmuens mentioned this pull request Jun 28, 2017

Change in package/exclude usage? #3869

Closed

HyperBrain reviewed Jun 29, 2017

View reviewed changes

This was referenced Jun 30, 2017

Fix dev dependency exclusion bug when too many node modules are used #3874

Closed

Excluding dev dependencies throws Error #3827

Closed

Fix node_modules regex in excludeDevDependencies to be OS independent #3880

Merged

adieuadieu mentioned this pull request Jul 2, 2017

feat: add support for Serverless node devDeps exclusion feature serverless/serverless-plugin-typescript#25

Merged

pmuens mentioned this pull request Jul 3, 2017

Fix dev dependency exclusion bug #3889

Merged

5 tasks

Conversation

pmuens commented Jun 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What did you implement:

How did you implement it:

How can we verify it:

Todos:

Uh oh!

cjelger Jun 12, 2017

Choose a reason for hiding this comment

Uh oh!

cjelger Jun 12, 2017

Choose a reason for hiding this comment

Uh oh!

pmuens commented Jun 16, 2017

Uh oh!

cjelger commented Jun 16, 2017

Uh oh!

pmuens commented Jun 16, 2017

Uh oh!

pmuens commented Jun 16, 2017

Uh oh!

pmuens commented Jun 16, 2017

Uh oh!

ccannell commented Jun 20, 2017

Uh oh!

pmuens commented Jun 20, 2017

Uh oh!

cjelger commented Jun 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pmuens commented Jun 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cjelger commented Jun 21, 2017

Uh oh!

ryanmurakami commented Jun 21, 2017

Uh oh!

pmuens commented Jun 22, 2017

Uh oh!

HyperBrain Jun 29, 2017

Choose a reason for hiding this comment

Uh oh!

pmuens Jun 30, 2017

Choose a reason for hiding this comment

Uh oh!

arabold commented Jun 29, 2017

Uh oh!

pmuens commented Jun 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arabold commented Jun 30, 2017

Uh oh!

arabold commented Jun 30, 2017

Uh oh!

pmuens commented Jun 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ttarhan commented Jul 24, 2017

Uh oh!

pmuens commented Jul 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ttarhan commented Jul 24, 2017

Uh oh!

pmuens commented Jul 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

pmuens commented Jun 6, 2017 •

edited

Loading

cjelger commented Jun 21, 2017 •

edited

Loading

pmuens commented Jun 21, 2017 •

edited

Loading

pmuens commented Jun 30, 2017 •

edited

Loading

pmuens commented Jun 30, 2017 •

edited

Loading

pmuens commented Jul 24, 2017 •

edited

Loading

pmuens commented Jul 25, 2017 •

edited

Loading