-
Notifications
You must be signed in to change notification settings - Fork 319
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
Update to Webpack 4 #1052
Update to Webpack 4 #1052
Conversation
Deploy preview for nusmods ready! Built with commit 0d33fd7 |
Hmmm, the file size is slightly bigger. Will investigate. |
Does |
That's odd. Cache loader should be removed from all configs. Check if I forgot to remove them from somewhere? |
Found it. I'll remove it and push |
Anyway I forgot to post an update here, but the reason why I've left this branch for a while is because this upgrade isn't giving us better filesizes or build times (better in dev, worse in prod), so all in all it looks like a regression. I'm sure we're doing something wrong, but I can't figure out where - probably because we're not using OptimizeCSSAssetsPlugin, but like I said before it's not playing well with sourcemaps |
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.
LGTM 🗳
www/.postcssrc.js
Outdated
'default', | ||
{ | ||
// mergeLonghand produces incorrect transformations with border | ||
// https://github.com/cssnano/cssnano/issues/557 |
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 was fixed!
@@ -10,3 +10,21 @@ module.exports = { | |||
require('./scripts/postcss-single-root'), | |||
], | |||
}; | |||
|
|||
// Use CSSNano in production | |||
if (process.env.NODE_ENV === 'production') { |
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.
Should we run this in dev too? It's easier to identify errors that 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.
May slow it down though. I don't know what's the overhead but I assume it's not nothing
www/webpack/webpack.parts.js
Outdated
@@ -45,8 +41,6 @@ const VENDOR = [ | |||
'history', // History module used by router | |||
'fbjs', // facebook deps | |||
'prop-types', // gone but not forgotten | |||
'loader', // style loader fallbacks | |||
'equal', // various comparison libs used by deps | |||
]; | |||
|
|||
const DLL = { |
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 drop this now we don't have dll.
www/webpack/webpack.config.prod.js
Outdated
const extractTextPlugin = new ExtractTextPlugin('[name].[chunkhash].css', { | ||
allChunks: true, | ||
const cssExtractPlugin = new MiniCssExtractPlugin({ | ||
filename: '[name].[chunkhash:8].css', |
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 should use content hash instead: https://medium.com/@sahilkkrazy/hash-vs-chunkhash-vs-contenthash-e94d38a32208
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 that's right? Article says content hash is from ExtractTextPlugin which doesn't exist anymore
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's replaced by MiniCssExtractPlugin, which should work the same way
I'm still hesitant about merging this in. Currently
|
f51c3d8
to
3786ca9
Compare
I think the branch is ready to merge now. Just need to run through the usual checks to make sure upgrading the build toolchain didn't break anything |
Remaining issues (non-blockers)
|
Closes #874
Upgrade notes