better handling of external modules

[mikestaub]

I see that this issue has recently been resolved with this PR.

While I understand the need for fine-grained control over which modules are included in the deployed artifact, I am concerned that the current approach is error prone and sub-optimal for two reasons:

1. A user is forced to explicitly state which modules should be included in the deploy. I suspect many users will add a dependency to the package.json during local dev, then forget to add it to serverless.yml. There should be a single source of truth, the package.json. At the very least, it would have been better to use "webpackExcludeModules" instead of "webpackIncludeModules".

2. I think it is too naive to simply do another npm install in the artifact directory. Which code is included in the final bundle should be determined in the webpack.config.js using the externals property (single source of truth again). This will also let users take advantage of the tree-shaking feature in webpack v2, which can dramatically reduce the artifact size being uploaded.

I suggest simply setting an environment variable during deployment, that the user can check for in their webpack config file. For example, while testing this PR I added the following to my webpack config:

const externals = [];
if (process.env.SERVERLESS_WEBPACK_DEPLOY !== 'true') {
  externals.push(require('webpack-node-externals'));
}

This PR can coexist with the current approach, but I would suggest reverting the earlier merge as it might be confusing to have two different ways of achieving the same thing.

Please let me know if you find anything wrong with my reasoning. Thanks.

[thenikso]

I totally agree with your points.

I'd envision a different solution though. Something like the plugin can automatically use dependencies in package.json and externals in webpack.config.js to bundle the required modules automatically. I think this should be possible with Webpack output stats and whatnot.

This whole thing might be activated using the same webpackIncludeModules custom option and setting it to true rather than specifying module names.

Would this work in your opinion? Maybe also @cyyuen which submitted the other PR might also have opinions about this?

[mikestaub]

I like your idea better. Environment variables are hacky, I was just trying to make a point. Setting 'webpackIncludeModules: true' is much cleaner, and as you say, we have all the needed information in dependencies and externals.

[cyyuen]

@thenikso I agree. We might use webpack-node-externals to exclude all the deps from the bundled files and pack all required packages stated in dependencies.

@mikestaub cannot agree with your point 2.

Which code is included in the final bundle should be determined in the webpack.config.js using the externals property.

'aws-sdk' package should be added to externals property and not be included in final distribution because it is a Lambda builtin package. Also, the plugin will only install the packages stated in webpackIncludeModules field. However, I total agree with you that there should be only one source of trust and I think @thenikso 's approach looks promising.

[HyperBrain]

@mikestaub The whole Serverless execution context is now available in webpack.config.js, so I think there's no need to introduce separate environment variables as Serverless' context can give you any information pretty well.

Sample:

// webpack.conf.js
const slsw = require('serverless-webpack');
...
// slsw exposes the following objects:
// slsw.lib.entries -> Used for automatic entry detection
  entry: slsw.lib.entries,
...
// slsw.lib.serverless
// The serverless instance. All properties are available and can be used to create
// a config completely depending on the framework's state.
...
// slsw.lib.options
// The options given to the webpack plugin. Can be used to retrieve the stage
// and other properties.
const stage = slsw.lib.options.stage;
const region = slsw.lib.options.region;

I suggest to close this PR and use the new sources of information instead.

[mikestaub]

Great, thanks for the follow up!