Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Expose service and options for dynamicity #168

Merged
merged 4 commits into from
Jul 27, 2017
Merged

Expose service and options for dynamicity #168

merged 4 commits into from
Jul 27, 2017

Conversation

kikar
Copy link
Contributor

@kikar kikar commented Jul 27, 2017

What did you implement:

Closes #158

How did you implement it:

How can we verify it:

Todos:

  • Write tests
  • Write documentation
  • Fix linting errors (not yet implemented)
  • Make sure code coverage hasn't dropped (not yet implemented)
  • Provide verification config / commands / resources
  • Enable "Allow edits from maintainers" for this PR
  • Update the messages below

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

@kikar
Copy link
Contributor Author

kikar commented Jul 27, 2017

@HyperBrain opened a PR, could you help me with the documentation and testing?

@HyperBrain
Copy link
Member

Will have a look later today and review the changes 😃

@kikar
Copy link
Contributor Author

kikar commented Jul 27, 2017

I realised now that if we expose the serverless and the option, we dont really need the entries anymore.
Does it make sense to remove it?

@HyperBrain
Copy link
Member

Not really. Entries contain the entry points needed for the actual Serverless run. It will also honor single function deployments. We should not move this functionality (that guarantees that it is set correctly) to the webpack configuration. Later on (when we fully support individual packaging, so that tree shaking is applied via multi-compile to let each function be optimized separately) entries must be completely under the control of the plugin.

With the new service and options export, it is in the developer's hand, if he wants to change the default behavior - with the risk of breaking things or over-engineering :) But the default behavior should be as simple as possible, so that any user can just enable the plugin, use lib.entries and is done.

@kikar
Copy link
Contributor Author

kikar commented Jul 27, 2017

Example of my webpack.config.js.
Thanks to the servicePath it can stay in the root folder and not in every service.

const slsw = require('serverless-webpack');
const { join } = require('path');
const UglifyPlugin = require('webpack').optimize.UglifyJsPlugin;

let entry = slsw.lib.entries;
for (let key in entry) {
  entry[key] = entry[key] + '.ts';
}

let servicePath = slsw.lib.serverless.config.servicePath;
let plugins = [];

if (slsw.lib.options.stage === 'prod') {
  plugins.push(new UglifyPlugin()); // Tree Shaking FTW!
}

module.exports = {
  entry,
  target: 'node',
  module: {
    loaders: [
      { test: /\.ts(x?)$/, loader: 'ts-loader' }
    ]
  },
  resolve: {
    extensions: ['.ts', '.js']
  },
  output: {
    path: join(slsw.lib.serverless.config.servicePath, '.webpack'),
    libraryTarget: 'commonjs',
    filename: '[name].js'
  },
  plugins
}

@HyperBrain
Copy link
Member

This looks good.

let entry = slsw.lib.entries;
for (let key in entry) {
entry[key] = entry[key] + '.ts';
}

This will be obsolete if we do a file search and determine the correct handler extension in #167 instead of forcing the user to add code to his webpack conf to get it run.

In general, I think that your example (after the extension changes in #167 have been done) can be added to the examples folder as (typescript-with-dynamic configuration).

I will add the missing unit tests here and merge this after the other PR.

@kikar
Copy link
Contributor Author

kikar commented Jul 27, 2017

This will be obsolete if we do a file search and determine the correct handler extension in #167 instead of forcing the user to add code to his webpack conf to get it run.

I think that it can get complicated, but also gives less space for customisation. Imagine a situation where the developer wants to write a function in .ts and one in .js

It's only 4 lines of code that we're talking about in the end.

@HyperBrain
Copy link
Member

HyperBrain commented Jul 27, 2017

But everyone has to add them - exactly in the same way. The framework should do that properly (and can do that because it is deterministic).

Regarding the ts / js per function. That's a special use case that the developer can do in the config.js - he knows which function he wants ts or js, so he can add a few lines of code to modify the entries in this case. But given a 80/20 rule, most of the users will not do that and just use lib.entries and it will work for them.

@HyperBrain
Copy link
Member

I added unit tests and will update the README too. Then this should be good to go 😃 .

@HyperBrain HyperBrain self-requested a review July 27, 2017 13:37
Copy link
Member

@HyperBrain HyperBrain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to go now

@HyperBrain HyperBrain merged commit ceaf43c into serverless-heaven:master Jul 27, 2017
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Is it possible to dynamically configure the WebPack configuration (e.g. to do Tree Shaking)?
2 participants