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

Upgrading Webpack to v4.5, enable caching via the uglifyJS, and modifying output #3970

Merged
merged 4 commits into from
Apr 6, 2018

Conversation

sebworks
Copy link
Contributor

@sebworks sebworks commented Apr 6, 2018

Upgrading Webpack to v4.5, enable caching via the uglifyJS Plugin, and modifying Webpack output

Changes

  • Upgraded Webpack to version 4.5
  • Modified various Webpack configs to enable caching UglifyJS files and modify console output

Testing

  1. Run gulp build and verify that the output no longer contains entrypoints.
    Example: Entrypoint Expandable.js = Expandable.js
  2. Run open node_modules/.cache/uglifyjs-webpack-plugin and verify that the folder exists.

Notes

  • We have a couple layers of caching / process avoidance going on, so we will need to pay attention to ensure the caches are being expired properly.

Checklist

  • PR has an informative and human-readable title
  • Changes are limited to a single goal (no scope creep)
  • Code can be automatically merged (no conflicts)
  • Code follows the standards laid out in the development playbook
  • Passes all existing automated tests
  • Any change in functionality is tested
  • New functions are documented (with a description, list of inputs, and expected output)
  • Placeholder code is flagged / future todos are captured in comments
  • Visually tested in supported browsers and devices (see checklist below 👇)
  • Project documentation has been updated
  • Reviewers requested with the Reviewers tool ➡️

Testing checklist

Browsers

  • Chrome
  • Firefox
  • Safari
  • Internet Explorer 8, 9, 10, and 11
  • Edge
  • iOS Safari
  • Chrome for Android

Accessibility

  • Keyboard friendly
  • Screen reader friendly

Other

  • Is useable without CSS
  • Is useable without JS
  • Flexible from small to large screens
  • No linting errors or warnings
  • JavaScript tests are passing

@sebworks sebworks changed the title Upgrading Webpack to v4.5, enable caching via the uglifyOptions, and modifying output Upgrading Webpack to v4.5, enable caching via the uglifyJS, and modifying output Apr 6, 2018
@@ -60,27 +60,36 @@ const COMMON_CHUNK_CONFIG = new webpack.optimize.SplitChunksPlugin( {
name: COMMON_BUNDLE_NAME
} );

const STATS_CONFIG = {
stats: {
entrypoints: false
Copy link
Member

Choose a reason for hiding this comment

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

This is such a short config, what do you think about putting the false value in a constant and then putting the rest in each webpack object? This would make the object spread unnecessary and simplify the webpack configs that aren't using multiple copies of STATS_CONFIG.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like it as an separate object because it can be extended when debugging and you don't have to update things in multiple places. This is the entire list of stat options: https://webpack.js.org/configuration/stats/#stats

Copy link
Member

Choose a reason for hiding this comment

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

Ah gotcha, I didn't realize there were so many settings in there. One thing I'm confused on is https://webpack.js.org/configuration/stats/#stats shows entrypoints: false like that's the default value. What's the default state of the stats object in the config? Is the default that it's absent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, I'm not sure what the default values are and the string value doesn't work with Webpack stream. shama/webpack-stream#163.

@@ -9,7 +9,7 @@ parserOptions:
globalReturn: false
impliedStrict: true
jsx: true
experimentalObjectRestSpread: false
experimentalObjectRestSpread: true
Copy link
Member

Choose a reason for hiding this comment

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

Does this need an update in https://github.com/cfpb/development/blob/master/.eslintrc#L12 (and CF and generator-cf)? This is to avoid usage of Object.assign?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's my understanding that object rest spread has reached stage 4 per: https://github.com/tc39/proposal-object-rest-spread. I think its easier to read and less code.

Copy link
Member

Choose a reason for hiding this comment

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

@@ -37,14 +38,19 @@ const COMMON_MODULE_CONFIG = {
presets: [ [ 'babel-preset-env', {
targets: {
browsers: BROWSER_LIST.LAST_2_IE_9_UP
},
debug: true
Copy link
Member

Choose a reason for hiding this comment

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

I'd be inclined to leave this as debug: false so it's easier to find where to turn it on, like is done with the uglify config. Or do you think that's too much clutter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in cc2efa0.

module: COMMON_MODULE_CONFIG,
mode: 'production',
output: {
filename: 'spanish.js'
},
plugins: [
COMMON_UGLIFY_CONFIG
]
],
...STATS_CONFIG
Copy link
Member

Choose a reason for hiding this comment

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

Since this is the only webpack config where the STATS_CONFIG is used multiple times, you could modify the loop at the end of the file:

let currConfig;
for ( const key in configExports ) {
  currConfig = configExports[key];
  currConfig.stats = STATS_CONFIG.stats; // Or use Object.assign(…)
  if ( NODE_ENV === 'development' ) {
    // eslint-disable-next-line guard-for-in
    Object.assign( currConfig, devConf );
  }
}

That would render the need for turning on the experimental flag unnecessary.

Or alternatively, the object spread could be used for mixing in the dev config as well.

Copy link
Contributor Author

@sebworks sebworks Apr 6, 2018

Choose a reason for hiding this comment

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

I don't think we have to use object spread or Object.assign uniformly in all cases. I think the code reads wells as it is. There might be a case for using the object spread for COMMON_MODULE_CONFIG though.

Copy link
Member

Choose a reason for hiding this comment

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

👍

@anselmbradford
Copy link
Member

anselmbradford commented Apr 6, 2018

We have a couple layers of caching / process avoidance going on, so we will need to pay attention to ensure the caches are being expired properly.

How is the cache expired with it turned on here? Do we need to do anything in our gulp clean task?

@sebworks
Copy link
Contributor Author

sebworks commented Apr 6, 2018

How is the cache expired with it turned on here? Do we need to do anything in our gulp clean task?

It's my understanding that the cache will automatically be invalidated upon files changing. This the default config for webpack per: webpack-contrib/uglifyjs-webpack-plugin#224 (comment).

https://github.com/webpack-contrib/uglifyjs-webpack-plugin/blob/b31b5744d7e1500b179e854ca8902e738c348e80/src/index.js#L158 and

webpack-contrib/uglifyjs-webpack-plugin#224

Copy link
Member

@anselmbradford anselmbradford left a comment

Choose a reason for hiding this comment

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

Works as described!

@sebworks sebworks merged commit 48599ec into master Apr 6, 2018
@sebworks sebworks deleted the upgrading_webpack branch April 6, 2018 23:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants