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

Split vendor and app into separate files #2206

Closed
shrynx opened this issue May 18, 2017 · 31 comments
Closed

Split vendor and app into separate files #2206

shrynx opened this issue May 18, 2017 · 31 comments

Comments

@shrynx
Copy link
Contributor

shrynx commented May 18, 2017

NOT AN ISSUE but a feature request.
imo, it would be beneficial for splitting the js bundle into app and vendor files.
My app code keeps changing but vendor dependencies rarely change. splitting into two files will be a lot beneficial for long term caching.

If this interests, i will be more than happy to make a pull request for this.

@gaearon
Copy link
Contributor

gaearon commented May 18, 2017

Yeah I agree it’s a good idea.

@viankakrisna did some work in #1651 but it’s pretty complicated. Maybe we can start with something simpler. For now I would be open to taking the following implementation:

  • src/index.js stays the main entry point
  • If src/vendor.js exists, then anything imported there goes into a separate vendor bundle

@gaearon gaearon added this to the 2.0.0 milestone May 18, 2017
@gaearon gaearon changed the title split vender and app into separate files. Split vendor and app into separate files May 18, 2017
@shrynx
Copy link
Contributor Author

shrynx commented May 18, 2017

Okay i think my way of splitting vendor is quite different.
i just had a look to the fore mentioned pull request. it deals with dll (which i still not too familiar with)

webpack is super awesome
the commons chunk plugin takes can take a function, thus we can automatically determine all the vendors by checking if the import belongs to node_modules folder or not.

// this assumes your vendor imports exist in the node_modules directory
new webpack.optimize.CommonsChunkPlugin({
  name: 'vendor',
  minChunks: module => module.context && module.context.indexOf('node_modules') !== -1
}),

@gaearon
Copy link
Contributor

gaearon commented May 18, 2017

I'm not sure this works well because some Node modules update much more often than others. I would like to keep user in control here.

@shrynx
Copy link
Contributor Author

shrynx commented May 18, 2017

ahh i see the point. got it.

@shrynx
Copy link
Contributor Author

shrynx commented May 18, 2017

@gaearon i'll try making a pull request by weekend for the way you mentioned src/index.js and src/vendor.js

@gaearon
Copy link
Contributor

gaearon commented May 18, 2017

Sounds good!

@viankakrisna
Copy link
Contributor

@gaearon actually i've moved the vendor entry point to ./src/vendor.js but no heuristics yet to enable the feature only if it exists (i could add another check in start.js and webpack config :p). I'll try to simplify it to a webpack plugin i guess.

@shrynx The common chunks plugin still runs on every build, so it does not solve the long build time for large project (can the build be cached? I actually haven't explore it ). I chose dll because then we can control when we want to build the vendor bundle.

As a side note, i haven't found a solution for this auto bundling the vendor yet. The closest one is this https://github.com/clinyong/dll-link-webpack-plugin but it requires user to use yarn.

My solution depends on the hash of vendor.js, yarn.lock (if exists), package.json, existence of the build in node_modules/.cache/vendor, and switches on prod / dev. I agree the ejected user will overwhelmed by the config right now, but i'll try to simplify it into a plugin (all help is welcome!).

@viankakrisna
Copy link
Contributor

@shrynx for prior discussion of long term caching, see #210

@shrynx
Copy link
Contributor Author

shrynx commented May 18, 2017

@viankakrisna i might be mistaken, but there are two separate issues.

  • long term asset caching by browsers for vendor file.
  • caching vendor libraries during compile time, so as to reduce build time.

of course they can be solved with a common approach (like your PR), but my feature request was of browser caching and not compile time caching (definitely a relevant issue !)

@gaearon just to make sure, should i be working on vendor splitting for browser cache or for both browser cache && build time ?

@viankakrisna
Copy link
Contributor

Added a check for ./src/vendor.js existence here https://github.com/facebookincubator/create-react-app/pull/1651/files#diff-c0e4b3a84334c2e5d316b33e2b0d5c9bR7 It's now opt-in :).

@viankakrisna
Copy link
Contributor

viankakrisna commented May 18, 2017

@shrynx yeah, but for a long term caching the bundle is still generated on compile time, is it not?

#1651 is solving those two issue that you have said, it's splitting the vendor and app code (solving long term caching) with it's own each build process (solving faster rebuilds). It's quite complex in the earlier iterations, because it's naively modifying webpack config without heuristics.

Right now that PR does not touch the webpack config if src/vendor.js not exists. so i think it's quite simple for ejected users (i hope).

@shrynx
Copy link
Contributor Author

shrynx commented May 21, 2017

Sorry for the silence. i had been little busy.
@viankakrisna yes, the bundle is again generated at compile time, but i can use deterministic hashes to keep it the same. Again my aim is not solving build time, but browser caching.
@gaearon a little update.
i made a demo app, with vendor and route splitting and ran build.
the first build contains two routes.
screen shot 2017-05-21 at 12 04 04
next i added one more route to the application and built it again.
screen shot 2017-05-21 at 12 04 29
the vendor file and old routes keep same hashes.

i'll send up a pull request regarding this in a while (cleaning things up). i have one concern this process always generates
one manifest file (to extract out the common runtime). As this file is usually small (< 1kb), i would prefer to inline it. any suggestions regarding this ?

@xwenliang
Copy link

That's very useful, but will old routes still keep same hashes when delete first route?

@JustFly1984
Copy link

JustFly1984 commented Jun 19, 2017

Guys, how about making /src/chunks/,

So /src/chunks/index.js has other imports:

import vendors from './vendors.js'
import utils from './utils.js'
import devtools from './devtools.js'
// e.t.c.

// vendors.js content will be separated to vendors.6fj34s78hjfg.js chunk

import React, { Component } from 'react'

import PropTypes from 'prop-types'

export {
  React,
  Component,
  PropTypes
}

//  utils.js content will be separated to utils.7645ydffj6645.js chunk
import ... from '../utils'

export {
  ...
}
//  devtools.js content will be separated to devtools.k65ffs45yyj6.js chunk
import DevTools from '../devtools'

export {
  DevTools
}
// Somwhere in app structure:

import {
  React,
  Component,
  PropTypes
} from '../../chunks/vendors'

class A extends Component {...

if /src/chunks/index.js is empty, there is no code splitting

This keeps control in user's hands and creates best practices pattern

I used this pattern in times of webpack 1.x.x

@JustFly1984
Copy link

BTW I tried to use /src/vendor.js and have no code splitting (

@gaearon
Copy link
Contributor

gaearon commented Jun 29, 2017

@JustFly1984 Nothing was merged, so there is no splitting.

@JustFly1984
Copy link

Can you update the status? Right now I feel like I getting need to use CRA for development and make production webpack config separate from CRA. I'm getting weird tensions in the team. One part against ejecting, another needs a build improvements. Please clarify your intentions about vendor splitting.

@miraage
Copy link

miraage commented Jul 24, 2017

Noteworthy: https://github.com/asfktz/autodll-webpack-plugin

@karlhorky
Copy link

Another project along the same lines as AutoDllPlugin: fliphub/d-l-l

@Ayc0
Copy link

Ayc0 commented Aug 27, 2017

Why not this in webpack.config.prod.js:

const vendors = require('../src/vendors');

...

entry: {
  polyfills: require.resolve('./polyfills'),
  app: paths.appIndexJs,
  vendors,
},

...

plugins: [
  new webpack.optimize.CommonsChunkPlugin(/* chunkName= */ 'vendors'),
...
],

And in src/vendors.js:

module.exports = ['react', 'react-dom', ...];

I kept every thing else (with react-scripts 1.0.10) and added this and the code splitting is working

@Stanley-Jovel
Copy link

Any news on this?

@Ayc0
Copy link

Ayc0 commented Sep 18, 2017

I'm working on a PR

@JustFly1984
Copy link

JustFly1984 commented Sep 18, 2017 via email

@Ayc0
Copy link

Ayc0 commented Sep 19, 2017

I think it's done

@JustFly1984
Copy link

JustFly1984 commented Sep 20, 2017 via email

@Ayc0
Copy link

Ayc0 commented Sep 20, 2017

If the PR is merged, there will be a src/vendors.js file and inside, only module.exports = ['react', 'react-dom'];

To add another vendors, you just have to fill the array with the npm module names, and if the file doesn't exist, there won't be any vendors

@Ayc0
Copy link

Ayc0 commented Sep 20, 2017

but if you need this feature as soon as possible, you can eject the conf, add manually this MR (and others), and if a new version of CRA includes everything you need, you'll revert the eject (just revert the commit)

@Timer
Copy link
Contributor

Timer commented Sep 20, 2017

They don't need to eject, they could use the release cut by @react-scripts-dangerous temporarily.

@ebhoren
Copy link

ebhoren commented Sep 29, 2017

@Timer how to use this feature right now?

@Timer
Copy link
Contributor

Timer commented Sep 29, 2017

@ebhoren if you're willing to help test the feature, follow these instructions: #3145 (comment)

@gaearon
Copy link
Contributor

gaearon commented Jan 20, 2018

This is superseded by #3878.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests