-
-
Notifications
You must be signed in to change notification settings - Fork 26.8k
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
Long term caching and split app into main.js and vendors.js #3145
Conversation
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed. If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks! |
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
@@ -64,7 +64,7 @@ module.exports = { | |||
// 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], | |||
entry: [require.resolve('./polyfills'), paths.appIndexJs, paths.appVendors], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain this to me? I thought it needed to be like this:
{
entry: {
vendors: require(paths.appVendors),
app: [require.resolve('./polyfills'), paths.appIndexJs]
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've done this to remain coherent with the current code
I think that
{
entry: {
vendors: require(paths.appVendors),
app: paths.appIndexJs,
polyfills: require.resolve('./polyfills'),
}
}
is easier to understand
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That seems fine as long as the polyfills get included before the app, and vendors before the app too.
I don't fully understand webpack entries 😄 .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can give either a string which indicates the location of a file (or a npm package), an array of those strings or an object.
If it's an array or a string, the name of the entry will be the name of the file, and if you give an object, the name is the key and the location is the value.
In this case, the vendors in an array so webpack's gonna concatenate the vendors into a single chunk called vendors.[hash].js and the app and the polyfills are linked to the location of the file
I haven't put the files in the right order, I'll change this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the order of the entries is handle by HtmlWebpackPlugin with its option chunksSortMode
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I understand, NameAllModulesPlugin
is required for when externals are added because they cause ids to shift.
Sure, it may not affect non-ejected users, but we'd like ejected users to have something that "just works".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the module NameAllModulesPlugin
is deprecated and now we have to use NamedModulesPlugin
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(In fact, it's not, I have to learn how to read...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haha you're alright! You're a total champ for your work on this PR; most people give up by now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dude, it's just a two day work and I already did the split vendor thing in one of my project haha 😀
If possible, I'd like to get as close as possible to this: https://medium.com/webpack/predictable-long-term-caching-with-webpack-d3eee1d3fa31 |
Hello! I'm a bot that helps facilitate testing pull requests. Your pull request (commit 9bfe59544077be82794bcbf5c34e0667259da36d) has been released on npm for testing purposes. npm i react-scripts-dangerous@1.0.11-9bfe595.0
# or
yarn add react-scripts-dangerous@1.0.11-9bfe595.0
# or
create-react-app --scripts-version=react-scripts-dangerous@1.0.11-9bfe595.0 folder/ Note that the package has not been reviewed or vetted by the maintainers. Only install it at your own risk! Thanks for your contribution! |
We need a story for when |
// Thanks to this change the vendor hash will now always stay the same | ||
new webpack.NamedModulesPlugin(), | ||
// Ensure that every chunks have an actual name and not an id | ||
// If the chunck has a name, this name is used |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: chunck
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops 😅
Hello! I'm a bot that helps facilitate testing pull requests. Your pull request (commit 700470871e04b689563fdd3bf98c9db013b825f8) has been released on npm for testing purposes. npm i react-scripts-dangerous@1.0.11-7004708.0
# or
yarn add react-scripts-dangerous@1.0.11-7004708.0
# or
create-react-app --scripts-version=react-scripts-dangerous@1.0.11-7004708.0 folder/ Note that the package has not been reviewed or vetted by the maintainers. Only install it at your own risk! Thanks for your contribution! |
I forced a release for you (by @react-scripts-dangerous) in case the bug in CI only manifests during a simulated release. Just trying to be of help! I'm going to sleep for now, so good luck! Thanks for what you're doing. 😄 |
Thanks Timer 👍 |
Timer, I've tried many things, the NamedChunksPlugin was a bit buggy when dynamic imports occured in the code. And sorting entries with the HTML plugging cause the compilation to fail. |
The issue with NamedChunksPlugin and dynamic imports is that the filename is |
Is
So does the |
For And the option |
} | ||
return (chunk.mapModules | ||
? chunk.mapModules(m => m.request) | ||
: chunk.modules.map(m => m.request) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need to have fallback behavior for old webpack versions, or is this here for another reason?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mapModules isn't supported in Webpack 2.6 and modules are deprecated in Webpack 3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need to worry about webpack 2.6 support 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was the current version on CRA when I fork the repo
But now the v3 is used 😁
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll pull the current version of CRA
Looks like you accidentally rebased in the wrong direction 😛 |
Fix via: $ git pull # just in case, I see you're on two different computers
$ git remote add upstream https://github.com/facebookincubator/create-react-app.git
$ git fetch upstream
$ git rebase upstream/master
$ git push -f Then, on your other computer, make sure you do (for the first time after you force push): $ git pull --rebase or, I didn't test this one but it should work too: $ git status # make sure your directory is clean; if it's not, commit and use the above command
$ git fetch origin
$ git reset --hard origin/master |
Oh yeah, i've rebased by reflex, sorry |
Hello! I'm a bot that helps facilitate testing pull requests. Your pull request (commit 11579fb9fb1848208be47edb53bba981b8e94ddc) has been released on npm for testing purposes. npm i react-scripts-dangerous@1.0.14-11579fb.0
# or
yarn add react-scripts-dangerous@1.0.14-11579fb.0
# or
create-react-app --scripts-version=react-scripts-dangerous@1.0.14-11579fb.0 folder/ Note that the package has not been reviewed or vetted by the maintainers. Only install it at your own risk! Thanks for your contribution! |
// List of all the node modules that should be excluded from the app | ||
vendors: fs.existsSync(paths.appVendors) | ||
? require(paths.appVendors) | ||
: ['react', 'react-dom'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gaearon would you agree these are decent defaults, or would you rather disable the vendor bundle all together?
I'm not sure how many people use react-scripts
without react
. 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you use react-scripts without react ?
If you want to use it with preact for instance, you have to eject so...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You wouldn't have to eject if you don't need to alias (external modules), right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've never used external modules so I have no ideas
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant a node_modules
package by external modules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can wait for @gaearon's input, but I think we'd just change:
entry: {
// Load the app and all its dependencies
main: paths.appIndexJs,
// Add the polyfills
polyfills: require.resolve('./polyfills'),
// List of all the node modules that should be excluded from the app
vendors: fs.existsSync(paths.appVendors)
? require(paths.appVendors)
: ['react', 'react-dom'],
},
to something like this:
entry: Object.assign(
{
// Load the app and all its dependencies
main: paths.appIndexJs,
// Add the polyfills
polyfills: require.resolve('./polyfills'),
},
fs.existsSync(paths.appVendors)
? // List of all the node modules that should be excluded from the app
{ vendors: require(paths.appVendors) }
: {}
),
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, it makes more sense to only load the vendors if the file exists, it's more understandable this way
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Timer, I managed to import the entries in the right order with HtmlWebpackPlugin
and the option chunksSortMode
What wasn't working before is that I've imported an entry before the entry called runtime. And if you place the entries here in a wrong order, it won't work. The right order is
['runtime', 'vendors', 'polyfills', 'main']`
But as webpack loads the entries without any bugs, I think it auto detects which entries must be imported before the others so I don't know if it's really usefull to hardcode the order of the entries
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well if it's not needed we can remove it, but only if we're sure!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure at all, it's just that if the order is wrong, it won't compile and this is the only arrangement that works, so it can't be by chance.
I've added the commit anyways, thus if we want to remove it, it's pretty easy and the work isn't lost
Hello! I'm a bot that helps facilitate testing pull requests. Your pull request (commit 64887f9f5d251256f6a381c3ec3ba992e148f976) has been released on npm for testing purposes. npm i react-scripts-dangerous@1.0.14-64887f9.0
# or
yarn add react-scripts-dangerous@1.0.14-64887f9.0
# or
create-react-app --scripts-version=react-scripts-dangerous@1.0.14-64887f9.0 folder/ Note that the package has not been reviewed or vetted by the maintainers. Only install it at your own risk! Thanks for your contribution! |
Please merge this 🙏 |
// it loads the rest, then the vendors | ||
// And then the polyfills before the main | ||
// The others chunks will be loaded after | ||
chunksSortMode: sortChunks(['runtime', 'vendors', 'polyfills', 'main']), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why don't we remove this since we said that it automatically detects the correct order?
Let's not add this until a problem arises (if one does).
Less complexity is always better. 😄
@@ -0,0 +1,45 @@ | |||
// @remove-on-eject-begin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per removing chunksSortMode
, we can probably trash this file now.
packages/react-scripts/package.json
Outdated
@@ -44,6 +44,7 @@ | |||
"fs-extra": "3.0.1", | |||
"html-webpack-plugin": "2.29.0", | |||
"jest": "20.0.4", | |||
"name-all-modules-plugin": "^1.0.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please pin this package version.
@@ -0,0 +1 @@ | |||
module.exports = ['react', 'react-dom']; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about we add a comment here explaining what adding an entry to this list does?
// This file controls which npm packages are extracted into a vendor bundle.
// A vendor bundle is very useful for reducing your transmitted bundle size, as
// dependencies which rarely change can be cached while you iterate on your application.
// (edit this comment as you see fit and make it run to 80 cols please)
module.exports = ['react', 'react-dom'];
// You can delete this file if you'd prefer to not make a vendor bundle.
// (edit this comment as you see fit and make it run to 80 cols please)
|
||
// Avoid having the vendors in the rest of the app | ||
new webpack.optimize.CommonsChunkPlugin({ | ||
name: 'vendors', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to exclude this commons chunk if we don't specify vendors
in entry
? Or is it simply ignored?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tried this without the vendor entry and it seems to be okay
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've rechecked, and if the file exists, the vendor entry weights 40KB, and the main 10KB (can't remember the exact values), and without, webpack compiles and there is no vendors entry and main weighs 50KB
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to exclude this for safety reasons; we're not sure how webpack might regress in the future.
// as dependencies which rarely change can be cached while you iterate on your | ||
// application. | ||
|
||
// If you want to add another vendor, just add in this array the name of the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sounds backwards -- how about "If you want to add another npm package to the vendor bundle, just add its name in this array".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not fluent in english so... 😁
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problem!
Hello! I'm a bot that helps facilitate testing pull requests. Your pull request (commit c40c666dcce74aad55f06e7e5640505d5b5e44ca) has been released on npm for testing purposes. npm i react-scripts-dangerous@1.0.14-c40c666.0
# or
yarn add react-scripts-dangerous@1.0.14-c40c666.0
# or
create-react-app --scripts-version=react-scripts-dangerous@1.0.14-c40c666.0 folder/ Note that the package has not been reviewed or vetted by the maintainers. Only install it at your own risk! Thanks for your contribution! |
@Timer do I need to change other things ? |
packages/react-scripts/package.json
Outdated
@@ -64,4 +65,4 @@ | |||
"optionalDependencies": { | |||
"fsevents": "1.1.2" | |||
} | |||
} | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you re-add the ending newline? Sorry for such a trivial thing.
This reverts commit 64887f9f5d251256f6a381c3ec3ba992e148f976.
This PR is now useless |
We're very thankful for all the work you've put into it ❤️ If you'd like to send a PR updating us to webpack 4 beta (so we can get closer to trying this change) it would be very welcome! |
I filed #3878 for migrating to webpack 4. |
Proposal for issue #2206