-
Notifications
You must be signed in to change notification settings - Fork 12k
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
feat(build): add lazy styles/scripts #3402
Conversation
009213a
to
fddd2cf
Compare
bde0363
to
8bbf31e
Compare
] | ||
} | ||
plugins: [ | ||
new ExtractTextPlugin({filename: '[name].bundle.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.
I think the javascript bundle is needed for HMR 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.
There seems to be ways of using it with extracted css: webpack-contrib/extract-text-webpack-plugin#30 (comment)
I also want to note that CSS is only extracted for global styles.
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.
OK. Always extracting is definitely cleaner just didn't know about the HMR workaround.
@@ -73,7 +50,8 @@ export const getWebpackProdConfigPartial = function(projectRoot: string, | |||
new webpack.LoaderOptionsPlugin({ |
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 will replace the options from the common config. webpack-merge doesn't handle it.
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.
Thanks for catching that, will fix.
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.
Done. had to duplicate the config into prod though, didn't find a better way.
|
||
// load global scripts using script-loader | ||
extraRules.push({ | ||
include: globalScrips.map((script) => script.path), test: /\.js$/, loader: 'script-loader' |
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.
AOT removes the need for eval
but script-loader brings it back. Probably not something that should be addressed in this PR but I think it's important to note.
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 always had script-loader
for global scripts though, I merely added it conditionally instead of always having it.
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 agree; no current action is needed. I was just thinking maybe a future TODO or some additional future documentation (post-1.0) might be useful (probably on CSP in general).
Your autoprefixer changes is already fixed in #3164 |
4167c77
to
3b78e69
Compare
@grizzm0 you're right, and I have been late to merge it. I'll get it in and rebase mine on yours. |
LGTM. Just need to rebase. cheers! |
3b78e69
to
5944140
Compare
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Close #3401
Close #3400