Add vendor code-splitting #1031

ajgreenb · 2017-09-17T01:30:07Z

This PR introduces code-splitting for vendor libraries. It also adds scaffolding for caching in production. When NODE_ENV === 'production' in webpack.config.js, the files will be tagged with a hash of their contents. As far as I can tell, our build process currently doesn't allow us to set NODE_ENV in webpack.config.js since it gets required before NODE_ENV gets set in the env gulp task. To address this, I could

Make webpack.config.js export a function that takes an environment parameter and use that to modify the webpack configuration.
Add a build:production script in package.json that looks something like NODE_ENV=production gulp build --production.

Thoughts?

ajgreenb · 2017-09-17T01:31:13Z

Longer-term thinking: would you ever consider dropping gulp altogether and just using webpack for all building?

ajgreenb · 2017-09-17T01:34:17Z

src/index.html

@@ -14,6 +14,5 @@
        <div class="pop-throbber__image"></div>
      </div>
    </div>
-    <script src="application.js"></script>
  </body>
 </html>


Note that I moved src/static/index.html to src/index.html since it's now used as a template for html-webpack-plugin.

OK—I would say let’s maybe move it to src/html/index.html (mirroring src/css/application.css)? Realistically all the JS should be in src/js but that’s a reorganization for another day…

outoftime

Cool!!

Left a few very minor inline comments. Also more thoughts:

As far as I can tell, our build process currently doesn't allow us to set NODE_ENV in webpack.config.js since it gets required before NODE_ENV gets set in the env gulp task.

Oh, the --production thing is kind of silly anyway—let’s just get rid of it. I can just set NODE_ENV as an actual environment variable on Travis (in fact I just did—how’s that for service?). I like the idea of having the script in package.json to make it easier to test production-mode builds in development.

Longer-term thinking: would you ever consider dropping gulp altogether and just using webpack for all building?

Absolutely! I think we the way we build the CSS bundle right now is obviously clunky, and it seems like for HTML this PR basically transfers the responsibility to Webpack already.

I’ve never worked with a fully Webpack-based build system; would we still use gulp for stuff like pushing Firebase permissions changes on deploy?

It also adds scaffolding for caching in production.

So we already do cache in production, with time-based expiries. We also purge the entire cache when releasing. I think moving to hashing filenames is a good call here, as it allows us to only worry about the expiry of a single resource—index.html—and everything else can basically be cached forever.

But we will need to stop blowing away the entire CloudFlare cache when we release. I’m betting CF allows us to just purge index.html?

Also, another thing to think about is the service worker—with the content hash in the filename, intuitively it seems like offline storage will accumulate every version of the assets it’s ever seen, which (especially given the size of our assets!) seems potentially bad…

outoftime · 2017-09-17T20:27:47Z

src/index.html

@@ -14,6 +14,5 @@
        <div class="pop-throbber__image"></div>
      </div>
    </div>
-    <script src="application.js"></script>
  </body>
 </html>


OK—I would say let’s maybe move it to src/html/index.html (mirroring src/css/application.css)? Realistically all the JS should be in src/js but that’s a reorganization for another day…

outoftime · 2017-09-17T20:30:58Z

webpack.config.js

@@ -253,6 +261,27 @@ module.exports = {
      ],
      ServiceWorker: {navigateFallbackURL: '/'},
    }),
+    new webpack.optimize.CommonsChunkPlugin({
+      name: 'vendor',
+      minChunks({ context }) {


Project style would be no space inside the curly braces

outoftime · 2017-09-17T20:31:27Z

webpack.config.js

+          const isBowerComponent = context.indexOf('bower_components') !== -1;
+          return isNodeModule || isBowerComponent;
+        }
+        return false;


Very minor / I don’t feel strongly about this at all, but I would invert this and return false early if context is not truthy, so that the “meat” of the function body describes the expected case.

outoftime · 2017-09-17T20:37:24Z

webpack.config.js

@@ -66,8 +70,12 @@ module.exports = {
  entry: './src/application.js',
  output: {
    path: path.resolve(__dirname, './dist'),
-    filename: 'application.js',
-    sourceMapFilename: 'application.js.map',
+    filename: process.env.NODE_ENV === 'production'


I think it’s worth declaring a local const isProduction since we’re asking this question several times.

outoftime

Also noticed this error when checking it out locally:

(node:31065) DeprecationWarning: Chunk.modules is deprecated. Use Chunk.getNumberOfModules/mapModules/forEachModule/containsModule instead.

ajgreenb · 2017-09-23T03:42:44Z

In regard to pushing Firebase permissions changes on deploy: keeping gulp around just for that seems like overkill. If webpack doesn't cut it, maybe a simple package.json script would suffice? What does that gulp task actually do? I'm only have a surface-level understanding of Firebase.

ajgreenb · 2017-09-23T03:45:59Z

In regard to caching, I'm sure there's a solution to the service worker holding onto every version of the asset files. I'll do a little research and to find out the right way.

ajgreenb · 2017-09-23T04:01:33Z

In regard to the DeprecationWarning, I'll dig around and see what I can find out about that. I suspect it's caused by an out-of-date plugin.

ajgreenb · 2017-09-24T02:14:50Z

Just pushed a few more changes. A couple of things I'd like to bring to your attention:

We are (and were) minifying in development. Is that the right call? It's causing some Chrome console warnings about minifying development versions of React and Redux.
- We could add the UglifyJsPlugin only when NODE_ENV === 'production'.
- If we ultimately decide to use just webpack for building the front-end, we could skip adding the UglifyJsPlugin altogether and just execute webpack -p, which automatically adds it and also "tree-shakes".
I added a prod task to the package.json scripts. Is that the right idea?
- Will that require changes to the Travis CI configuration?

outoftime · 2017-09-24T22:12:45Z

We are (and were) minifying in development. Is that the right call?

Whoa, really! Totally unintentional. That would explain why the build in development is so freaking slow…

outoftime · 2017-09-24T22:14:29Z

package.json

@@ -173,6 +173,7 @@
    "postinstall": "bower install",
    "pretest": "yarn run lint",
    "dev": "yarn install && gulp dev",
+    "prod": "yarn install && NODE_ENV=production gulp build",


I don’t think there’s a need for the yarn install here—that’s already a step in Travis, and best to keep separate things separate imo (the dev thing is to make it more convenient for humans running the dev server, but no humans to inconvenience in production builds)

outoftime · 2017-09-24T22:21:37Z

webpack.config.js

@@ -16,6 +20,8 @@ const babel = require('babel-core');
 const babelLoaderVersion =
  require('./node_modules/babel-loader/package.json').version;

+const isProduction = process.env.NODE_ENV === 'production';


So, I just had a thought—yargs presumably is just looking at the arguments passed to the current process when it started. That information should be no different in a webpack.config module that’s required into the gulpfile than it is in the gulpfile itself, so could we just make this line const isProduction = yargs.argv.production and roll back some of the other related changes?

outoftime · 2017-09-24T22:22:18Z

webpack.config.js

@@ -253,6 +259,38 @@ module.exports = {
      ],
      ServiceWorker: {navigateFallbackURL: '/'},
    }),
+    new webpack.DefinePlugin({
+      'process.env': {
+        NODE_ENV: JSON.stringify(process.env.NODE_ENV || 'development'),


Do we need the stringify here?

Yup, I think we do:

Note that because the plugin does a direct text replacement, the value given to it must include actual quotes inside of the string itself. Typically, this is done either with either alternate quotes, such as '"production"', or by using JSON.stringify('production').

From https://webpack.js.org/plugins/define-plugin/

well then, in my face.

ajgreenb · 2017-09-30T22:34:26Z

Seems like offline-plugin adds its own content-based hash for cache invalidation. See

NekR/offline-plugin#127 (comment)

It's a matter of adding the updateStrategy: 'changed' option to the configuration. So could we avoid doing webpack content hashes altogether? The problem I see with this is browsers that don't support ServiceWorkers or AppCache. Does Popcode target browsers that don't support either of those?

outoftime · 2017-09-30T23:14:01Z

So could we avoid doing webpack content hashes altogether?

I don’t think so—as of now, at least, ServiceWorker / AppCache cached assets are only used if there is no network connection.

I think we probably want hashing at both levels for now, as long as they don’t interfere with each other?

ajgreenb · 2017-10-01T20:54:00Z

Okay cool, so I'll leave the content-hashing and possibly add updateStrategy: 'changed' to the offline-plugin configuration options.

That DeprecationWarning is caused by the webpack-chunk-hash plugin. I can remove it, but I've noticed that sometimes webpack will change a module's hash even if its contents haven't changed.

ajgreenb · 2017-10-07T15:55:01Z

webpack-chunk-hash just published 0.5.0 which fixes that DeprecationWarning we were seeing.

ajgreenb · 2017-10-07T16:00:43Z

@outoftime okay, looks like the default updateStrategy for offline-plugin is changed, so I don't need to do anything to enable that.

I'm not too familiar with ServiceWorkers or offline-plugin—do you know a way to view which resources have been cached? I'd like to be able to test that the cache behavior is correct (i.e., not accumulating copies of every bundle version.)

ajgreenb · 2017-10-07T16:26:13Z

Aaand, one more thing—you can see from the Travis output that the tests fail with Uncaught ReferenceError: webpackJsonp is not defined. This happens locally, too. It seems like karma-webpack doesn't support the CommonsChunkPlugin. See also: codymikol/karma-webpack#24.

Any preference on how to work around this issue? I could

Use lodash to pluck out the CommonsChunkPlugins in karma.conf.js, or
Use some kind of flag in webpack.config.js to include or omit the CommonsChunkPlugins based on whether we're testing.

outoftime · 2017-10-08T21:32:21Z

I'm not too familiar with ServiceWorkers or offline-plugin—do you know a way to view which resources have been cached? I'd like to be able to test that the cache behavior is correct (i.e., not accumulating copies of every bundle version.)

You can go into the Application tab of the Chrome Dev Tools, and then look in Service Workers and Cache Storage

Any preference on how to work around this issue?

The second option sounds better but what did you have in mind for the flag? I think my ideal solution would basically have webpack.config.js have a named export along the lines of generateWebpackConfig() that takes options and then generates a config based on those options, as well as a default export that is the result of calling that function with default options. However given we’re in CommonJS-land, we can’t actually do that…

ajgreenb · 2017-10-08T23:42:09Z

Nice, thanks! That's a great tip. I'll test this branch when I get home and make sure that the cache behaves as expected.

webpack supports exporting a function that returns a configuration object instead of just a plain object. Check out this link. How about having development, production, and test modes that enable/disable various plugins for each environment? In webpack.config.js, we could do something like

module.exports = (env = 'development') => {
    // ...
}

And then we could require it in different files and manually create the configuration object by passing the flag. If that sounds good, I'll open a small merge request that just changes webpack.config.js into a function but leaves everything else the same. Then I'll add the mode differentiation in this branch.

outoftime · 2017-10-09T01:28:43Z

@ajgreenb just merged the webpack function PR!

CommonsChunkPlugin is incompatible with karma-webpack.

ajgreenb · 2017-10-09T14:08:53Z

After doing some more research, it looks like offline-plugin doesn't actually care what you call your files. From what I understand, it will use its own content-based hashes as the sole source of truth for cache updates (regardless of the name of the file.)

Updatestrategy's effect on assets in the 'main' section of caches NekR/offline-plugin#292
Pending PR to update offline-plugin's documentation for updateStrategy

So I think the caching behavior is identical across this branch and master!

ajgreenb · 2017-10-09T14:22:17Z

I think the only thing left to do is update the purgeCache gulp task to stop purging the JavaScript files. The Cloudflare API does allow you to granularly specify what to purge, but you have to give it a list of filenames. Should that list include anything other than index.html? application.css comes to mind. Anything else?

outoftime · 2017-10-09T14:50:53Z

I think that probably covers it!

What about caching headers on S3? Right now the assets are set to expire fairly quickly—ideally everything that’s content-hashed should have ultra long lived headers right?

ajgreenb · 2017-10-09T15:15:03Z

Yeah, ultra-long-lived sounds good or maybe even Cache-Control: immutable? Where do you set those headers?

ajgreenb · 2017-10-09T15:55:34Z

This may be naive/silly (not too experienced here), but what about just not sending caching headers at all from S3 since we already cache in Cloudflare (also browser caching and maybe ServiceWorker caching)? Doesn't having lots of different caching layers make it easier to end up with the wrong caching behavior?

outoftime · 2017-10-10T01:37:30Z

This may be naive/silly (not too experienced here), but what about just not sending caching headers at all from S3 since we already cache in Cloudflare (also browser caching and maybe ServiceWorker caching)? Doesn't having lots of different caching layers make it easier to end up with the wrong caching behavior?

Sorry, I’m a bozo—caching behavior is already configured at the CloudFlare, not the S3, level. We can just update them to be more aggressive with the JavaScript after this is released.

ajgreenb · 2017-10-10T03:10:31Z

Sweet, well, unless I'm forgetting something, I think I've addressed all your feedback thus far. Let me know if you have further comments!

outoftime

Lookin great @ajgreenb just a couple final items!

outoftime · 2017-10-11T00:22:27Z

webpack.config.js

+      'process.env': {
+        NODE_ENV: JSON.stringify(isProduction ? 'production' : 'development'),
+      },
+    }),


I don’t think this is needed—we already set NODE_ENV using the EnvironmentPlugin (note that the values given there are just defaults in case the key isn’t defined in process.env at compile time).

outoftime · 2017-10-11T00:24:46Z

gulpfile.js

+    {
+      files: [
+        'https://popcode.org/index.html',
+        'https://popcode.org/application.css',


Let’s grab the hostname from process.env? It can just be added to .travis.yml for that to work properly.

It's not needed because we use the EnvironmentPlugin!

outoftime

Good to go!!

ajgreenb · 2017-10-11T01:16:41Z

Aw yeahhh, thanks @outoftime! I'll continue picking away at #348.

outoftime · 2017-10-11T01:36:17Z

Sweet! I feel like it was broken at some point but the Webpack Visualizer is working great now, ton of low-hanging fruit in here. I’d be inclined to try pulling out some of the big libraries that we don’t need for minimal bootstrap (firebase/database and github are two obvious ones) into async loads, but feel free to follow your nose/heart as well!

ajgreenb commented Sep 17, 2017

View reviewed changes

outoftime reviewed Sep 17, 2017

View reviewed changes

outoftime added the in progress label Sep 20, 2017

outoftime changed the title ~~[WIP] Add vendor code-splitting~~ Add vendor code-splitting Sep 20, 2017

ajgreenb force-pushed the code-splitting branch from 15f6134 to e3a62a9 Compare September 23, 2017 03:59

outoftime reviewed Sep 24, 2017

View reviewed changes

ajgreenb force-pushed the code-splitting branch from 12e8c0e to 54065ee Compare September 29, 2017 23:35

ajgreenb force-pushed the code-splitting branch from 54065ee to 33ae910 Compare September 30, 2017 22:35

ajgreenb force-pushed the code-splitting branch from 33ae910 to 20f72b2 Compare October 7, 2017 15:53

ajgreenb mentioned this pull request Oct 9, 2017

Convert webpack.config.js to export a function #1085

Merged

ajgreenb force-pushed the code-splitting branch from 59fd886 to cdc9343 Compare October 9, 2017 01:57

greenberga added 3 commits October 8, 2017 22:53

Autogenerate index.html with html-webpack-plugin

f5a8551

Code-split vendor libraries

3ae64a5

Lint changed JS files

636f433

greenberga added 7 commits October 8, 2017 22:53

Move index.html into src/html

ebde4b6

Prioritize order of operations in webpack plugin

9df7f69

Use yargs for webpack environment mode

a241540

Upgrade webpack-chunk-hash to v0.5.0

db72582

Configure bundlesize to check all JS files

c113b86

Update webpack config after rebasing master

c3f79bc

Do not use CommonsChunkPlugin when testing

22ec435

CommonsChunkPlugin is incompatible with karma-webpack.

ajgreenb force-pushed the code-splitting branch from cdc9343 to 22ec435 Compare October 9, 2017 14:24

Only purge HTML and CSS from Cloudflare cache

1b29bb9

ajgreenb closed this Oct 9, 2017

ajgreenb reopened this Oct 9, 2017

outoftime requested changes Oct 11, 2017

View reviewed changes

greenberga added 2 commits October 10, 2017 20:55

Remove redundant DefinePlugin

07a715c

It's not needed because we use the EnvironmentPlugin!

Get hostname for Cloudflare-purging from env var

a0d12d1

outoftime approved these changes Oct 11, 2017

View reviewed changes

outoftime merged commit 6aa3a11 into popcodeorg:master Oct 11, 2017

outoftime mentioned this pull request Oct 11, 2017

Reduce load times #348

Closed

6 tasks

Add vendor code-splitting #1031

Add vendor code-splitting #1031

Conversation

ajgreenb commented Sep 17, 2017

ajgreenb commented Sep 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

outoftime left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

outoftime left a comment

Choose a reason for hiding this comment

ajgreenb commented Sep 23, 2017

ajgreenb commented Sep 23, 2017 • edited Loading

ajgreenb commented Sep 23, 2017 • edited Loading

ajgreenb commented Sep 24, 2017

outoftime commented Sep 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajgreenb commented Sep 30, 2017

outoftime commented Sep 30, 2017

ajgreenb commented Oct 1, 2017

ajgreenb commented Oct 7, 2017 • edited Loading

ajgreenb commented Oct 7, 2017

ajgreenb commented Oct 7, 2017 • edited Loading

outoftime commented Oct 8, 2017

ajgreenb commented Oct 8, 2017

outoftime commented Oct 9, 2017

ajgreenb commented Oct 9, 2017

ajgreenb commented Oct 9, 2017 • edited Loading

outoftime commented Oct 9, 2017

ajgreenb commented Oct 9, 2017 via email

ajgreenb commented Oct 9, 2017 via email

outoftime commented Oct 10, 2017

ajgreenb commented Oct 10, 2017

outoftime left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

outoftime left a comment

Choose a reason for hiding this comment

ajgreenb commented Oct 11, 2017

outoftime commented Oct 11, 2017

outoftime left a comment •

edited

Loading

ajgreenb commented Sep 23, 2017 •

edited

Loading

ajgreenb commented Sep 23, 2017 •

edited

Loading

ajgreenb commented Oct 7, 2017 •

edited

Loading

ajgreenb commented Oct 7, 2017 •

edited

Loading

ajgreenb commented Oct 9, 2017 •

edited

Loading