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

[WIP] Split vendor bundle #2310

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions packages/react-scripts/config/paths.js
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ module.exports = {
appPublic: resolveApp('public'),
appHtml: resolveApp('public/index.html'),
appIndexJs: resolveApp('src/index.js'),
appVendorJs: resolveApp('src/vendor.js'),
appPackageJson: resolveApp('package.json'),
appSrc: resolveApp('src'),
yarnLockFile: resolveApp('yarn.lock'),
Expand All @@ -75,6 +76,7 @@ module.exports = {
appPublic: resolveApp('public'),
appHtml: resolveApp('public/index.html'),
appIndexJs: resolveApp('src/index.js'),
appVendorJs: resolveApp('src/vendor.js'),
appPackageJson: resolveApp('package.json'),
appSrc: resolveApp('src'),
yarnLockFile: resolveApp('yarn.lock'),
Expand Down Expand Up @@ -104,6 +106,7 @@ if (
appPublic: resolveOwn('template/public'),
appHtml: resolveOwn('template/public/index.html'),
appIndexJs: resolveOwn('template/src/index.js'),
appVendorJs: resolveOwn('template/src/vendor.js'),
appPackageJson: resolveOwn('package.json'),
appSrc: resolveOwn('template/src'),
yarnLockFile: resolveOwn('template/yarn.lock'),
Expand Down
59 changes: 57 additions & 2 deletions packages/react-scripts/config/webpack.config.prod.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,11 @@

const autoprefixer = require('autoprefixer');
const path = require('path');
const fs = require('fs');
const webpack = require('webpack');
const NameAllModulesPlugin = require('name-all-modules-plugin');
const HtmlWebpackPlugin = require('html-webpack-plugin');
const ScriptExtHtmlWebpackPlugin = require('script-ext-html-webpack-plugin');
const ExtractTextPlugin = require('extract-text-webpack-plugin');
const ManifestPlugin = require('webpack-manifest-plugin');
const InterpolateHtmlPlugin = require('react-dev-utils/InterpolateHtmlPlugin');
Expand Down Expand Up @@ -53,6 +56,18 @@ const extractTextPluginOptions = shouldUseRelativeAssetPaths
? // Making sure that the publicPath goes back to to build folder.
{ publicPath: Array(cssFilename.split('/').length).join('../') }
: {};
// Check if vendor file exists
const checkIfVendorFileExists = fs.existsSync(paths.appVendorJs);
// If the vendor file exists, add an entry point for vendor,
// and a seperate entry for polyfills and app index file,
// otherwise keep only polyfills and app index.
const appEntryFiles = [require.resolve('./polyfills'), paths.appIndexJs];
Copy link
Contributor

@viankakrisna viankakrisna May 23, 2017

Choose a reason for hiding this comment

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

This way vendor bundle does not get the polyfills. Polyfills should the first thing that run from our bundles, else we could get issue similar to this facebook/react#8379 (comment) for promises and fetch. I think we need another check here / move this to a resolver function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@viankakrisna yup makes sense. external libs that depends on promise might get this issue. Plus polyfills are vendor libs.
can't we simply do

const entryFiles = checkIfVendorFileExists
  ? {
      vendor: [require.resolve('./polyfills'), paths.appVendorJs],
      main: paths.appIndexJs,
    }
  : [require.resolve('./polyfills'), paths.appIndexJs];

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm thinking about something like this https://github.com/viankakrisna/create-react-app/blob/cb34a1baf21497ad1de7eb314d9ba97a89f1c0be/packages/react-dev-utils/webpackAutoDllCompiler.js#L82-L111 I feel that ejected users should not be bothered by an optional feature checks when they ejected.

const entryFiles = checkIfVendorFileExists
? {
vendor: paths.appVendorJs,
main: appEntryFiles,
}
: appEntryFiles;

// This is the production configuration.
// It compiles slowly and is focused on producing a fast and minimal bundle.
Expand All @@ -63,8 +78,8 @@ module.exports = {
// We generate sourcemaps in production. This is slow but gives good results.
// You can exclude the *.map files from the build during deployment.
devtool: 'source-map',
// In production, we only want to load the polyfills and the app code.
entry: [require.resolve('./polyfills'), paths.appIndexJs],
// Add the entry point based on whether vendor file exists.
entry: entryFiles,
output: {
// The build folder.
path: paths.appBuild,
Expand Down Expand Up @@ -250,6 +265,41 @@ module.exports = {
],
},
plugins: [
// configuration for vendor splitting and long term caching
// more info: https://medium.com/webpack/predictable-long-term-caching-with-webpack-d3eee1d3fa31
new webpack.NamedModulesPlugin(),
Copy link

Choose a reason for hiding this comment

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

just in case - webpack docs recommend adding also HashedModuleIdsPlugin for prod build. https://webpack.js.org/guides/caching/#deterministic-hashes

new webpack.NamedChunksPlugin(chunk => {
if (chunk.name) {
return chunk.name;
}
return chunk.modules
.map(m => path.relative(m.context, m.request))
.join('_');
Copy link

Choose a reason for hiding this comment

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

the only thing to keep in mind here is, that this could potentially become a length string, so it might make sense to cut the string at a certain point or use a hash digest or something similar.
Would probably need real life testing to see if it really becomes an issue.

the most brutal way would be to digest the paths, but that would mean one looses the ability to see what actually got required. maybe a healthy mix of both?

new webpack.NamedChunksPlugin(chunk => {
    if (chunk.name) {
        return chunk.name;
    }
    // const crypto = require('crypto');
    const pathDigest = crypto.createHash('md5');
    return chunk.modules
        .map(m => path.relative(m.context, m.request))
        .reduce((d, m )=> d.update(m), pathDigest)
        .digest('base64');

}),
new webpack.optimize.CommonsChunkPlugin(
// Check if vendor file exists, if it does,
// generate a seperate chucks for vendor
// else don't generate any common chunck
checkIfVendorFileExists
? {
name: 'vendor',
minChunks: Infinity,
}
: { names: [] }
),
// We need to extract out the runtime into a separate manifest file.
// more info: https://webpack.js.org/guides/code-splitting-libraries/#manifest-file
new webpack.optimize.CommonsChunkPlugin(
// Check if vendor file exists, if it does,
// generate a seperate chucks for manifest file
// else don't generate any common chunck
checkIfVendorFileExists
? {
name: 'manifest',
}
: { names: [] }
),
new NameAllModulesPlugin(),
Copy link

@mshustov mshustov Jun 6, 2017

Choose a reason for hiding this comment

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

there is an issue with the plugin that could hurt hashing. didn't have time to elaborate approach though :( timse/name-all-modules-plugin#1

// Makes some environment variables available in index.html.
// The public URL is available as %PUBLIC_URL% in index.html, e.g.:
// <link rel="shortcut icon" href="%PUBLIC_URL%/favicon.ico">
Expand All @@ -273,6 +323,11 @@ module.exports = {
minifyURLs: true,
},
}),
// This ensures that the browser will load the scripts in parallel,
// but execute them in the order they appear in the document.
new ScriptExtHtmlWebpackPlugin({
defaultAttribute: 'defer',
}),
// Makes some environment variables available to the JS code, for example:
// if (process.env.NODE_ENV === 'production') { ... }. See `./env.js`.
// It is absolutely essential that NODE_ENV was set to production here.
Expand Down
2 changes: 2 additions & 0 deletions packages/react-scripts/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -44,12 +44,14 @@
"fs-extra": "3.0.1",
"html-webpack-plugin": "2.28.0",
"jest": "20.0.3",
"name-all-modules-plugin": "^1.0.1",
"object-assign": "4.1.1",
"postcss-flexbugs-fixes": "3.0.0",
"postcss-loader": "2.0.5",
"promise": "7.1.1",
"react-dev-utils": "^2.0.0",
"react-error-overlay": "^1.0.5",
"script-ext-html-webpack-plugin": "^1.8.0",
"style-loader": "0.17.0",
"sw-precache-webpack-plugin": "0.9.1",
"url-loader": "0.5.8",
Expand Down