-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Tidy up webpack config #1058
Tidy up webpack config #1058
Conversation
04f37dc
to
5bb008f
Compare
'Access-Control-Allow-Origin': '*' | ||
watch_options: | ||
ignored: /node_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.
webpacker.yml
is intended for config that needs to be read by both Ruby and JavaScript (webpack). Seems fine to add additional dev server options here, but we should be careful of bloat.
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.
Yeah, just added the important ones that folks would like to change 👍
fs: 'empty', | ||
net: 'empty', | ||
tls: 'empty', | ||
child_process: 'empty' |
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.
What is this for?
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.
So, some libraries that import nodejs (like fs
) modules but don't use them in the browser. Here, we are telling webpack to provide empty mocks for them so importing 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.
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.
@gauravtiwari Is this still needed? Neither the docs or NodeStuffPlugin seem to use it!
strictExportPresence: true, | ||
rules: [ | ||
{ oneOf: this.loaders.values() } | ||
] |
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.
What do these options change?
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.
Previously, we were adding loaders directly to rules array but that had some weird limitations like say, if I want to use some other loader for processing a svg file and we also by default ship the file loader, which also processes svg files. There was no away to ensure that the new svg loader is used for svg files, and not file loader. With this change we are saying, use the first loader that matches the criteria (i.e matches test regex or exclude regex).
Also, the order of processing loaders is now from top to bottom i.e. fixed.
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 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.
Rule.oneOf
An array of Rules from which only the first matching Rule is used when the Rule matches.
What if people are relying on files being processed by multiple loaders?
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.
Then whenever compiler encounters different format it will traverse again:
For example: Started with a .js
file -> js file has a css import -> look for a loader for css -> then css file has a image linked -> look for a loader for .png file -> so on....
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 can't figure out how to config the istanbul-instrumenter-loader
after this change.
The rule condition for istanbul-instrumenter-loader
is different from the rule condition for coffee-loader
. The istanbul-instrumenter-loader
need to exclude test files, but the 'coffee-loader' need to include the test file. And with oneOf
option, the two loaders have to be in the same 'coffee' rule. Then what should i do?
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.
Hi, this change (oneOf
) breaks all combinations like .coffee.erb
the coffee-loader doesn't use the erb-loader and the erb-loader doesn't use the coffee-loader.
As for :
Previously, we were adding loaders directly to rules array but that had some weird limitations like say, if I want to use some other loader for processing a svg file and we also by default ship the file loader, which also processes svg files.
I guess the workaround for that would be to overwrite the file-loader rule like we do for SASS, something like :
const SASSRule = environment.loaders.get("sass");
const SASSLoader = SASSRule.use.find(el => el.loader === "sass-loader");
// Add resolve-url-loader after SASS
SASSRule.use.splice(SASSRule.use.indexOf(SASSLoader), 0, {
loader: "resolve-url-loader"
});
This is how I fixed it if anybody runs into the same problem :
Environment.prototype._toWebpackConfig = Environment.prototype.toWebpackConfig;
Environment.prototype.toWebpackConfig = function() {
const webpackConfig = this._toWebpackConfig();
if (webpackConfig.module.rules[0].oneOf) {
webpackConfig.module.rules = webpackConfig.module.rules[0].oneOf;
}
return webpackConfig;
}
module.exports = environment;
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.
Hi, this change (oneOf) breaks all combinations like .coffee.erb the coffee-loader doesn't use the erb-loader and the erb-loader doesn't use the coffee-loader.
Can you report a new issue with this?
Edit: nvm, it was already reported in #1081
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.
Oh didn't really look into issues I was looking at the code to figure out why my config was breaking. Thanks @guilleiguaran!
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.
Sorry about this folks. There is a PR that should fix all these edge cases. Will merge and make a release as soon as it’s merged and issues are addressed.
}, | ||
output: { | ||
comments: false | ||
comments: false, | ||
ascii_only: true |
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.
What do these changes do?
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 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 mean, why are you adding/changing these options? Do they resolve any known issues?
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, we didn't received any but just standardising the options for now (recommended ones).
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.
Recommended where?
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.
Just noticed a lot of bugs here related to emojis and regex and it seemed like a good option to be on since emojis are everywhere :): https://github.com/mishoo/UglifyJS2/search?p=1&q=ascii_only&type=Issues&utf8=%E2%9C%93
They haven't turned this on by default because build size increases or something.
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.
If you are not happy I can turn it off and wait until an issue is reported. Since the standard config is now hidden, thought it would make sense to add options that commonly causes issues.
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.
Just noticed a lot of bugs here related to emojis and regex and it seemed like a good option to be on since emojis are everywhere :)
👍
They haven't turned this on by default because build css size increases
CSS size❓
If you are not happy I can turn it off and wait until an issue is reported
I'm ok with it as long as it doesn't bloat JS file sizes significantly
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.
Just tested, turning it on doesn't increase the build size (all hello examples).
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 typo there :) (ignore css)
package/rules/file.js
Outdated
/\.(css)$/i, | ||
/\.html$/, | ||
/\.json$/ | ||
], |
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.
Looks like this could be combined to a single regex.
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.
Yep, will do 👍
Where does the speed improvement come from? |
Most probably from removing resolve-url-loader, which has some magic file lookups (remember that security issue raised earlier #932). See #1042 The usage of I have tested and most standard css libraries works without it, like - bootstrap and foundation. If someone wants to use, they can update the loader themselves. |
Another place, where we might be reaping speed benefits is from using I will probably benchmark this and post some results here. |
5bb008f
to
e1f6ad7
Compare
package/environment.js
Outdated
@@ -25,6 +26,8 @@ const getLoaderList = () => { | |||
const getPluginList = () => { | |||
const result = new ConfigList() | |||
result.append('Environment', new webpack.EnvironmentPlugin(JSON.parse(JSON.stringify(process.env)))) | |||
result.append('NamedModules', new webpack.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.
This plugin will cause the relative path of the module to be displayed when HMR is enabled. Suggested for use in development. (https://webpack.js.org/plugins/named-modules-plugin/)
Should we be using this in all environments?
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.
Good spot 👁 , will fix 👍
e1f6ad7
to
3a05570
Compare
@@ -0,0 +1,13 @@ | |||
const assetHost = require('../asset_host') | |||
|
|||
module.exports = { |
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.
@gauravtiwari How can I reference small static images in Rails view template after using this url 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.
All images under 10KB would use url loader by default, this includes png, jpg, gif and bmp
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.
Then the url loader will break images under 10KB in projects with server rendering. Consider the image src generated with asset_pack_path
.
So I have to delete url loader in custom config in this case?
This PR fixes a couple of things related to webpack config: