-
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,6 +10,7 @@ const extname = require('path-complete-extname') | |
const webpack = require('webpack') | ||
const ExtractTextPlugin = require('extract-text-webpack-plugin') | ||
const ManifestPlugin = require('webpack-manifest-plugin') | ||
const CaseSensitivePathsPlugin = require('case-sensitive-paths-webpack-plugin') | ||
|
||
const { ConfigList, ConfigObject } = require('./config_types') | ||
const rules = require('./rules') | ||
|
@@ -25,6 +26,7 @@ const getLoaderList = () => { | |
const getPluginList = () => { | ||
const result = new ConfigList() | ||
result.append('Environment', new webpack.EnvironmentPlugin(JSON.parse(JSON.stringify(process.env)))) | ||
result.append('CaseSensitivePaths', new CaseSensitivePathsPlugin()) | ||
result.append('ExtractText', new ExtractTextPlugin('[name]-[contenthash].css')) | ||
result.append('Manifest', new ManifestPlugin({ publicPath: assetHost.publicPath, writeToFileEmit: true })) | ||
return result | ||
|
@@ -76,6 +78,14 @@ const getBaseConfig = () => | |
|
||
resolveLoader: { | ||
modules: ['node_modules'] | ||
}, | ||
|
||
node: { | ||
dgram: 'empty', | ||
fs: 'empty', | ||
net: 'empty', | ||
tls: 'empty', | ||
child_process: 'empty' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. So, some libraries that import nodejs (like There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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! |
||
} | ||
}) | ||
|
||
|
@@ -93,7 +103,10 @@ module.exports = class Environment { | |
entry: this.entry.toObject(), | ||
|
||
module: { | ||
rules: this.loaders.values() | ||
strictExportPresence: true, | ||
rules: [ | ||
{ oneOf: this.loaders.values() } | ||
] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more.
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 commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can't figure out how to config the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi, this change ( As for :
I guess the workaround for that would be to overwrite the file-loader rule like we do for SASS, something like :
This is how I fixed it if anybody runs into the same problem :
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 commentThe 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 commentThe 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. |
||
}, | ||
|
||
plugins: this.plugins.values(), | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,11 +10,16 @@ module.exports = class extends Environment { | |
|
||
this.plugins.append('UglifyJs', new webpack.optimize.UglifyJsPlugin({ | ||
sourceMap: true, | ||
mangle: { | ||
safari10: true | ||
}, | ||
compress: { | ||
warnings: false | ||
warnings: false, | ||
comparisons: false | ||
}, | ||
output: { | ||
comments: false | ||
comments: false, | ||
ascii_only: true | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe 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 commentThe 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 commentThe 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 commentThe 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more.
👍
CSS size❓
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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Oops typo there :) (ignore css) |
||
} | ||
})) | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,6 @@ | ||
module.exports = { | ||
test: /\.coffee(\.erb)?$/, | ||
use: ['coffee-loader'] | ||
use: [{ | ||
loader: 'coffee-loader' | ||
}] | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,6 @@ | ||
module.exports = { | ||
test: /\.(ts|tsx)?(\.erb)?$/, | ||
use: ['ts-loader'] | ||
use: [{ | ||
loader: 'ts-loader' | ||
}] | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
const assetHost = require('../asset_host') | ||
|
||
module.exports = { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe 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 |
||
test: [/\.bmp$/, /\.gif$/, /\.jpe?g$/, /\.png$/], | ||
use: [{ | ||
loader: 'url-loader', | ||
options: { | ||
limit: 10000, | ||
name: '[name]-[hash].[ext]', | ||
publicPath: assetHost.publicPathWithHost | ||
} | ||
}] | ||
} |
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 👍