Support webpack dynamic requires (injected requires)

[ceilfors]

This is a Feature Request

Description

This was originally a bug entry, but it turned out that dynamic requires can easily be added as feature.
See #266 (comment)

For bug reports:

• What went wrong?
  Upgrading serverless-webpack from v2 from v3 throws an error from a perfectly valid webpack.config.js.

  This is happening because Instead of using the typical entry: slsw.lib.entries in webpack.config.js, I have changed the entry to entry: addDynamicRequire(slsw.lib.entries). In a nutshell, I'm adding 'babel-polyfill', 'source-map-support/register' in webpack.config.js to avoid the need of adding these two requires in all of my handlers.

• What did you expect should have happened?

  serverless-webpack should not restrict the contract of webpack.config.js. or make it explicit in the documentation

• What was the config you used?
  serverless.yml
  package individually: true

• What stacktrace or error message from your provider did you see?
  Not sure what this means

Similar or dependent issue(s):

• None

Additional Data

• More references:
  https://webpack.js.org/concepts/entry-points/
  https://stackoverflow.com/a/33396110/2464295

• Serverless-Webpack Version you're using:
  3.1.2

• Webpack version you're using:
  3.5.5

• Serverless Framework Version you're using:
  1.23.0

• Operating System:
  OS X 10.10.5

• Stack Trace (if available):

Type error:

  Type Error ---------------------------------------------

  Path must be a string. Received [ 'babel-polyfill',
  'source-map-support/register',
  './packages/core/src/alexa-skill.js' ]

     For debugging logs, run again after setting the "SLS_DEBUG=*" environment variable.


Stack trace:
  Stack Trace --------------------------------------------

TypeError: Path must be a string. Received [ 'babel-polyfill',
  'source-map-support/register',
  './packages/core/src/alexa-skill.js' ]
    at assertPath (path.js:28:11)
    at Object.relative (path.js:1248:5)
    at entryFunctions._.flatMap (/Users/ceilfors/github/realtime-ww2-alexa/node_modules/serverless-webpack/lib/validate.js:154:28)
    at /Users/ceilfors/github/realtime-ww2-alexa/node_modules/lodash/lodash.js:3554:27
    at /Users/ceilfors/github/realtime-ww2-alexa/node_modules/lodash/lodash.js:4944:15
    at baseForOwn (/Users/ceilfors/github/realtime-ww2-alexa/node_modules/lodash/lodash.js:3001:24)
    at /Users/ceilfors/github/realtime-ww2-alexa/node_modules/lodash/lodash.js:4913:18
    at baseMap (/Users/ceilfors/github/realtime-ww2-alexa/node_modules/lodash/lodash.js:3553:7)
    at map (/Users/ceilfors/github/realtime-ww2-alexa/node_modules/lodash/lodash.js:9571:14)
    at Function.flatMap (/Users/ceilfors/github/realtime-ww2-alexa/node_modules/lodash/lodash.js:9274:26)
    at ServerlessWebpack.validate (/Users/ceilfors/github/realtime-ww2-alexa/node_modules/serverless-webpack/lib/validate.js:153:31)
From previous event:
    at PluginManager.invoke (/Users/ceilfors/.nvm/versions/node/v8.1.2/lib/node_modules/serverless/lib/classes/PluginManager.js:360:22)
    at PluginManager.run (/Users/ceilfors/.nvm/versions/node/v8.1.2/lib/node_modules/serverless/lib/classes/PluginManager.js:391:17)
    at variables.populateService.then (/Users/ceilfors/.nvm/versions/node/v8.1.2/lib/node_modules/serverless/lib/Serverless.js:99:33)
    at runCallback (timers.js:800:20)
    at tryOnImmediate (timers.js:762:5)
    at processImmediate [as _immediateCallback] (timers.js:733:5)
From previous event:
    at Serverless.run (/Users/ceilfors/.nvm/versions/node/v8.1.2/lib/node_modules/serverless/lib/Serverless.js:86:74)
    at serverless.init.then (/Users/ceilfors/.nvm/versions/node/v8.1.2/lib/node_modules/serverless/bin/serverless:39:50)
    at <anonymous>

[HyperBrain]

Hi @ceilfors ,
unfortunately, individual packaging only works, if you define your entry as slsw.lib.entries because there has to be a reverse lookup to the functions and an exact mapping between the entries.
Any method that changes the entries will lead to an error here.

With dynamic require you see, that entry is no longer a map (which is needed for the lookup to work) but an array. SLS webpack cannot associate them anymore to a function. Entry has to be a map since V3.

I think it might be possible to add something, that would allow to add dynamic requires to functions by adding a new property to the serverless function definitions. Then the plugin could use this information when building the slsw.lib.entries map.

functions:
  testfunc:
    handler: src/alexa-skill.handle
    webpack:
      dynamicRequire:
        - babel-polyfill
        - source-map-support/register

Then the configuration will stay consistent and the plugin can handle the configuration properly - even with individual packaging.

Additionally the dynamic require could be specified as global option, so that all functions automatically would require the given modules:

custom:
  webpack:
    dynamicRequire:
        - babel-polyfill
        - source-map-support/register

What do you think?

BTW: You should switch to 4.0.0 that was released yesterday. The breaking change is only, that it needs a higher Serverless version.

[ceilfors]

Hi @HyperBrain,

I'm currently using a workaround by enabling these babel plugins:

• transform-runtime: instead of requiring babel-polyfill
• source-map-support: instead of requiring source-map-support/register

That solution looks good! I guess the only confusing part is that it is still reading the standard webpack config with a more restrictive format, which is not ideal but not a big deal. At the same time, maybe the workaround is good enough in most use cases.

On the side note, I'm new to serverless APIs and might miss something as my code now is very small, but isn't the serverless lookup looking like this?

this.entryFunctions = _.flatMap(this.webpackConfig.entry, (value, key) => {
  if value is array
    entry = value.find(v => v.includes(key))
}

[HyperBrain]

Thanks for the feedback. I'll change the title of the issue so that it states to add dynamic requires via the configuration I mentioned, so that it can be used as feature request.

Regarding the restrictions: The plugin is hosted by the Serverless framework - that implies that some mechanisms available in vanilla webpack have to be adapted. In general, configurations of this kind that are per function should be done in the serverless configuration as long as they can be different per function. A user does not need to know the exact functions that are defined in the service, and adding a single function with or without a different dynamic require configuration should not lead to changes in the webpack config (that's a big benefit of the approach, so you don't have to think about how to change the webpack config).

The general problem with a manual mapping is, that the output array of the webpack compiler has to be mapped to the serverless functions again (for individual packaging). But that is only possible with an initial deterministic and well-known creation algorithm of the entries. Of course there might be improvements possible, but I thaink this is not trivial and needs a lot of investigation and analysis.
Taking into account that manual configuration has become an edge case over time (from sls-webpack V1 to V4) changes in that area are likely to be very low prio.

[ceilfors]

I understand your perspective. I agree that the abstracting webpack.config.js in serverless.yml will bring some benefits to the user too. I'm just having a difficulty to draw a line of when an abstraction is needed in serverless.yml.

For example, before serverless framework supports SSM, I used slscrypt and I needed to include the .serverless-secret.json it creates in the package. Having the flexibility of configuring webpack.config.js would allow me to do the following, which is great. I suppose this wouldn't be a case that'll be promoted to serverless.yml.

  plugins: [
    new CopyWebpackPlugin([
          { from: '.serverless-secret.json' }
    ])
  ]

A wild suggestion, if entry is such a unique config that shouldn't be touched, it might be a good idea to hardcode it to slsw.lib.entries internally and throw an error from serverless-webpack plugin if one tries to configure it? At least the plugin users would know that they shouldn't touch this. (Guess I'm failing to see why would anyone not use the nice slsw.lib.entries!)

[HyperBrain]

@ceilfors Thanks for the discussion. The reason why there is no hard validation for the entry is, that the current slsw.lib.entries has evolved over time. It was introduced in V2, but without individual packaging at that time. So all V1 users switching to V2 had a chance to use it or keep their manual configuration (the only way available in V1) untouched. V3 added individual packaging, and that was the point, where this new feature could only be supported with the dynamic entry resolution. However, without individual packaging, the old manual way still worked with V3. I can only guess that many of the users switched their configs to use the dynamic way because individual packaging offers a much better way for optimization and made a real single function deploy possible.
So, at the end of the day, not using it can only be, because someone uses a very old legacy project, or explicitly renounces individual packaging in favor of a fully manual configuration (if that makes sense anymore is a different issue).

As I already released V4, which would have been a good place to enforce using it (by emitting an error), I'd prefer a non-breaking solution right now. Maybe a warning could be emitted in verbose mode, if you set the entry property to anything else, and an error can be thrown, if you define a manual entry, but enable individual packaging - because that combination is known to not work.

Regarding the use of webpack plugins for other solutions (like SSM as you mentioned), this is completely unrestricted and there are many users who use e.g. the copy file plugin, the define plugin and many more. There are even projects like the ELM project which use a specific language transpiler and combine that with other plugins. The only really restricted thing is the entry property.

[ceilfors]

@HyperBrain Thank you for the kind explanation, and for maintaining this great plugin! I'll try to contribute back and file a PR for that last part warning and error throwing (and probably explore the actual feature if I can make the time!).

[HyperBrain]

@ceilfors That would be great. Maybe you can create a separate feature request for the entry warnings and errors, so that we can handle the implementation tasks separately too (they are not really related).