Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Refactor configuration #336

Merged
merged 3 commits into from Mar 7, 2018
Merged

Refactor configuration #336

merged 3 commits into from Mar 7, 2018

Conversation

@HyperBrain
Copy link
Member

@HyperBrain HyperBrain commented Mar 7, 2018

What did you implement:

This PR consolidates all webpack plugin configuration settings into the new custom.webpack object. It effectively reduces cluttering of the serverless configuration and keeps all settings in one place.

There is one breaking change with the configuration: If you have chosen to embed the webpack configuration into the serverless.yml (previously by defining custom.webpack as an object with the contents of a webpack config file) you have to move that to custom.webpack.config. In my opinion this way to configure webpack is very rare and mainly used as convenience method for the unit tests, as it does not allow for dynamic entry point resolution. This feature will be dropped in the future.

The new configuration object has the following layout. If a property is omitted, it will default to the shown values:

custom:
  webpack:
    webpackConfig: 'webpack.config.js'
    webpackIncludeModules: false
    packager: 'npm'
    packExternalModulesMaxBuffer: 200 * 1024
}

All settings have just been moved, but semantically are exactly as before.

However, the old configuration keys continue to work, but will lead to a warning message to let people know that there is a better alternative for configuration now.

How did you implement it:

Use a new class to keep the configuration centralized. The class is initialized with the serverless custom object and will populate the settings from there (using the old semantics as fallback) and it will set reasonable defaults for anything that has not been set.

How can we verify it:

(1) Use an existing project as is. It should build as usual but show a warning.
(2) Adapt the configuration to use the new custom.webpack object. It should build as before.

Todos:

  • Write tests
  • Write documentation
  • Fix linting errors
  • Make sure code coverage hasn't dropped N/A
    This PR introduced an expected drop in coverage, because it enables counting all files for coverage calculation. Previously only the tested ones were counted, but it was not obvious that there were untested files in the project.
  • 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?: YES (most likely not noticable)

@HyperBrain HyperBrain added this to the 5.0.0 milestone Mar 7, 2018
@HyperBrain HyperBrain force-pushed the refactor-configuration branch from d3b06fb to 2dd8de8 Mar 7, 2018
@HyperBrain HyperBrain merged commit 7f1062b into v5 Mar 7, 2018
5 checks passed
5 checks passed
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 81.058%
Details
@HyperBrain HyperBrain deleted the refactor-configuration branch Mar 7, 2018
@HyperBrain
Copy link
Member Author

@HyperBrain HyperBrain commented Mar 8, 2018

Released with 5.0.0

@renovate renovate bot mentioned this pull request Nov 9, 2018
0 of 1 task complete
jamesmbourne pushed a commit to jamesmbourne/serverless-webpack that referenced this pull request Oct 15, 2019
…tor-configuration

Refactor configuration
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

1 participant