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

Conversation

shrynx
Copy link
Contributor

@shrynx shrynx commented May 21, 2017

PR for #2206
steps to create vendor bundle file

  • Create a file vendor.js in src folder.
  • import vendor libs in the vendor file.
  • run build

@siggirh
Copy link

siggirh commented May 22, 2017

Just a general wondering: Is it a good idea to add this vendor ability without controlling the parameters for the CommonsChunkPlugin? I haven't used it in a while but I remember it having issues with bundling the entire libraries specified in vendor and not just the required bits. Is that not a thing anymore?

@shrynx
Copy link
Contributor Author

shrynx commented May 22, 2017

@SR-H you can control what goes into vendor bundle.
eg: if you are using only a few functions from lodash, then just import only those in vendor.js file.

// vendor.js
import React from 'react';
import 'lodash/func1';
import 'lodash/func2';
import 'lodash/func3';
import 'lodash/func4';

@gaearon
Copy link
Contributor

gaearon commented May 22, 2017

cc #2328

}
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');

// 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.

@chemitaxis
Copy link

Hi @shrynx, how is going it to work? I add all the vendors in my project (for example, lodash, mobx, etc.) in vendor.js and I would use in other file without import? I was using that in my custom projects, but I want to migrate to CRA.

// optimizations
    new CommonsChunkPlugin({
      children: true,
      async: 'common',
      minChunks: 2,
    }),
    new CommonsChunkPlugin({
      children: true,
      async: 'vendor',
      minChunks(module) {
        // this assumes your vendor imports exist in the node_modules directory
        return isVendor(module);
      },
    }),

Where isVendor is like this:

function isVendor (module) {
  return module.context && module.context.indexOf('node_modules') !== -1;
}

Thanks!!

@@ -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

}
: { 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

@jstejada
Copy link

jstejada commented Jun 14, 2017

hi all! I was just wondering if there were updates re this PR? thanks! cc @shrynx

@Stanley-Jovel
Copy link

Hello, guys, I wonder if this PR is going to get merged soon

@chemitaxis
Copy link

Hi, any news? :) Thanks!

@shrynx
Copy link
Contributor Author

shrynx commented Jun 20, 2017

Sorry for the long silence.
There is still little work left in this PR, but i am waiting to reach a general consensus and have to figure out how this should work for ejected users.
Also, there is another PR #1651 by @viankakrisna , which achieves same and also provided build time caching, which would speed up development significantly.

@gaearon
Copy link
Contributor

gaearon commented Jan 20, 2018

This is superseded by #3878.

@gaearon gaearon closed this Jan 20, 2018
@lock lock bot locked and limited conversation to collaborators Jan 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants