ctrlplusb / react-universally

A starter kit for universal react applications.
MIT License
1.7k stars 244 forks source link

Move webpack manifest out from index chunk(Improve long term caching of index chunk) #472

Open dhruvparmar372 opened 7 years ago

dhruvparmar372 commented 7 years ago

Issue

index chunk hash does not change if any of the children chunks content changes.

Steps to reproduce

  1. yarn run build on master branch.
  2. Save the build/client/index-{}.js file for later reference
  3. Change content of shared/components/DemoApp/AsyncCounterRoute/CounterRoute.js
  4. yarn run build.
  5. Save the build/client/index-{}.js file.

Both versions of index-{}.js file have same file names but different hash name for "counter" chunk inside them.

This is currently an issue with webpack https://github.com/webpack/webpack/issues/1856

A possible fix for same is to use https://github.com/roscoe054/webpack-hashed-module-id-plugin but it tends to generate new chunk names on every build even for unchanged chunks. This hampers long term caching of those chunks

Another side effect of the issue is that since webpack manifest is currently part of the index chunk on every change to any child chunk the index chunk's hash will change.

Fix

The manifest is inlined with every server rendered route in production mode. This PR enables the same in production mode

dhruvparmar372 commented 7 years ago

@ctrlplusb any thoughts on this? I ran into the issue while using react-universally in production for an application.

slipo commented 6 years ago

I would strongly recommend this approach is taken.

This is a huge issue if you have a CDN in front of your files and it can be very tricky to debug. Users in one part of the world can get a cached manifest pointing to files that no longer exist (completely breaking the site), while other users in a different region will get the new manifest and everything will work.

I believe the issue is with the md5 hasher: https://github.com/erm0l0v/webpack-md5-hash/issues/9

The solution is either dhruvparmar372's which I think is ideal because it increases the likelihood of serving a cached index or removing webpack-md5-hash like this (not making a PR because in the end I think we should break out the manifest):

diff --git a/internal/webpack/configFactory.js b/internal/webpack/configFactory.js
index a7060c5..71da446 100644
--- a/internal/webpack/configFactory.js
+++ b/internal/webpack/configFactory.js
@@ -4,7 +4,6 @@ import ExtractTextPlugin from 'extract-text-webpack-plugin';
 import nodeExternals from 'webpack-node-externals';
 import path from 'path';
 import webpack from 'webpack';
-import WebpackMd5Hash from 'webpack-md5-hash';

 import { happyPackPlugin, log } from '../utils';
 import { ifElse } from '../../shared/utils/logic';
@@ -226,13 +225,6 @@ export default function webpackConfigFactory(buildOptions) {
       // the significant improvement will be how fast the JavaScript loads in the browser.
       ifProdClient(new webpack.optimize.ModuleConcatenationPlugin()),

-      // We use this so that our generated [chunkhash]'s are only different if
-      // the content for our respective chunks have changed.  This optimises
-      // our long term browser caching strategy for our client bundle, avoiding
-      // cases where browsers end up having to download all the client chunks
-      // even though 1 or 2 may have only changed.
-      ifClient(() => new WebpackMd5Hash()),
-
       // These are process.env flags that you can use in your code in order to
       // have advanced control over what is included/excluded in your bundles.
       // For example you may only want certain parts of your code to be
sinwailam193 commented 6 years ago

I actually tried out this resolution by @dhruvparmar372 , however it will fail for service worker offline, as I get MIME Type error for application/javascript for the manifest.json.

oyeanuj commented 6 years ago

@dhruvparmar372 @sinwailam193 @slipo what solution did you folks end up with here?

dhruvparmar372 commented 6 years ago

@oyeanuj i feel this approach should be taken due to the issues pointed above. @sinwailam193 i've put in the fix for the offline mode error, the inlined manifest needed to have correct nonce value inorder to execute in offline mode.