Skip to content
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

Merged
merged 1 commit into from
Dec 4, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,27 @@ cssLoader.options = Object.assign(cssLoader.options, {
})
```

- Expose rest of configurable dev server options from webpacker.yml

```yml
quiet: false
headers:
'Access-Control-Allow-Origin': '*'
watch_options:
ignored: /node_modules/
```
- Enforce deterministic loader order in desc order, starts processing from top to bottom
- Enforce the entire path of all required modules match the exact case of the actual path on disk using [case sensitive paths plugin](https://github.com/Urthen/case-sensitive-paths-webpack-plugin).
- Add url loader to process and embed smaller static files
### Removed
- resolve url loader [#1042](https://github.com/rails/webpacker/issues/1042)
### Added (Gem)
- Allow skipping webpacker compile using an env variable
Expand Down
7 changes: 7 additions & 0 deletions lib/install/config/webpacker.yml
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,15 @@ development:
# Inline should be set to true if using HMR
inline: true
overlay: true
compress: true
disable_host_check: true
use_local_ip: false
quiet: false
headers:
'Access-Control-Allow-Origin': '*'
watch_options:
ignored: /node_modules/

Copy link
Contributor

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.

Copy link
Member Author

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 👍


test:
<<: *default
Expand Down
3 changes: 2 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
"babel-plugin-transform-object-rest-spread": "^6.26.0",
"babel-polyfill": "^6.26.0",
"babel-preset-env": "^1.6.1",
"case-sensitive-paths-webpack-plugin": "^2.1.1",
"coffee-loader": "^0.9.0",
"compression-webpack-plugin": "^1.0.1",
"css-loader": "^0.28.7",
Expand All @@ -31,9 +32,9 @@
"postcss-loader": "^2.0.9",
"postcss-smart-import": "^0.7.5",
"rails-erb-loader": "^5.2.1",
"resolve-url-loader": "^2.2.0",
"sass-loader": "^6.0.6",
"style-loader": "^0.19.0",
"url-loader": "^0.6.2",
"webpack": "^3.8.1",
"webpack-manifest-plugin": "^1.3.2"
},
Expand Down
7 changes: 6 additions & 1 deletion package/config.js
Original file line number Diff line number Diff line change
@@ -1,10 +1,15 @@
const { resolve } = require('path')
const { safeLoad } = require('js-yaml')
const { readFileSync } = require('fs')
const deepMerge = require('./utils/deep_merge')

const defaultFilePath = require.resolve('../lib/install/config/webpacker.yml')
const filePath = resolve('config', 'webpacker.yml')

const environment = process.env.NODE_ENV || 'development'
const config = safeLoad(readFileSync(filePath), 'utf8')[environment]
const defaultConfig = safeLoad(readFileSync(defaultFilePath), 'utf8')[environment]
const appConfig = safeLoad(readFileSync(filePath), 'utf8')[environment]
const config = deepMerge(defaultConfig, appConfig)

const isBoolean = str => /^true/.test(str) || /^false/.test(str)

Expand Down
15 changes: 14 additions & 1 deletion package/environment.js
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Expand All @@ -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
Expand Down Expand Up @@ -76,6 +78,14 @@ const getBaseConfig = () =>

resolveLoader: {
modules: ['node_modules']
},

node: {
dgram: 'empty',
fs: 'empty',
net: 'empty',
tls: 'empty',
child_process: 'empty'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this for?

Copy link
Member Author

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link

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!

}
})

Expand All @@ -93,7 +103,10 @@ module.exports = class Environment {
entry: this.entry.toObject(),

module: {
rules: this.loaders.values()
strictExportPresence: true,
rules: [
{ oneOf: this.loaders.values() }
]
Copy link
Contributor

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?

Copy link
Member Author

@gauravtiwari gauravtiwari Dec 2, 2017

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

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?

Copy link
Member Author

@gauravtiwari gauravtiwari Dec 2, 2017

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....

Copy link

@farthinker farthinker Dec 8, 2017

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?

Copy link

@charlesvallieres charlesvallieres Dec 14, 2017

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;

Copy link
Member

@guilleiguaran guilleiguaran Dec 14, 2017

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

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!

Copy link
Member Author

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.

},

plugins: this.plugins.values(),
Expand Down
15 changes: 7 additions & 8 deletions package/environments/development.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,8 @@ module.exports = class extends Environment {
},
devServer: {
clientLogLevel: 'none',
compress: true,
compress: devServer.compress,
quiet: devServer.quiet,
disableHostCheck: devServer.disable_host_check,
host: devServer.host,
port: devServer.port,
Expand All @@ -31,17 +32,15 @@ module.exports = class extends Environment {
useLocalIp: devServer.use_local_ip,
public: devServer.public,
publicPath: assetHost.publicPath,
historyApiFallback: true,
headers: {
'Access-Control-Allow-Origin': '*'
historyApiFallback: {
disableDotRule: true
},
headers: devServer.headers,
overlay: devServer.overlay,
watchOptions: {
ignored: /node_modules/
},
stats: {
errorDetails: true
}
},
watchOptions: devServer.watch_options
}
})
}
Expand Down
9 changes: 7 additions & 2 deletions package/environments/production.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

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?

Copy link
Member Author

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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Recommended where?

Copy link
Member Author

@gauravtiwari gauravtiwari Dec 2, 2017

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.

Copy link
Member Author

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.

Copy link
Contributor

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

Copy link
Member Author

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).

Copy link
Member Author

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)

}
}))

Expand Down
4 changes: 3 additions & 1 deletion package/rules/coffee.js
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'
}]
}
5 changes: 2 additions & 3 deletions package/rules/css.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,8 @@ const styleLoader = {
const extractOptions = {
fallback: styleLoader,
use: [
{ loader: 'css-loader', options: { minimize: isProduction, sourceMap: true, importLoaders: 3 } },
{ loader: 'postcss-loader', options: { sourceMap: true, config: { path: postcssConfigPath } } },
{ loader: 'resolve-url-loader', options: { attempts: 1 } }
{ loader: 'css-loader', options: { minimize: isProduction, sourceMap: true, importLoaders: 2 } },
{ loader: 'postcss-loader', options: { sourceMap: true, config: { path: postcssConfigPath } } }
]
}

Expand Down
2 changes: 1 addition & 1 deletion package/rules/elm.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,5 +19,5 @@ const elmWebpackLoader = {
module.exports = {
test: /\.elm(\.erb)?$/,
exclude: [/elm-stuff/, /node_modules/],
use: isProduction ? [elmWebpackLoader] : ['elm-hot-loader', elmWebpackLoader]
use: isProduction ? [elmWebpackLoader] : [{ loader: 'elm-hot-loader' }, elmWebpackLoader]
}
2 changes: 1 addition & 1 deletion package/rules/file.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ const { source_path } = require('../config')
const assetHost = require('../asset_host')

module.exports = {
test: /\.(jpg|jpeg|png|gif|tiff|ico|svg|eot|otf|ttf|woff|woff2)$/i,
exclude: /\.(js|jsx|coffee|ts|tsx|vue|elm|scss|sass|css|html|json)?(\.erb)?$/,
use: [{
loader: 'file-loader',
options: {
Expand Down
2 changes: 2 additions & 0 deletions package/rules/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,14 @@ const coffee = require('./coffee')
const elm = require('./elm')
const erb = require('./erb')
const file = require('./file')
const url = require('./url')
const css = require('./css')
const sass = require('./sass')
const typescript = require('./typescript')
const vue = require('./vue')

module.exports = {
url,
babel,
coffee,
elm,
Expand Down
5 changes: 4 additions & 1 deletion package/rules/sass.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,10 @@ let sassLoader = JSON.parse(JSON.stringify(cssLoader))

sassLoader = deepMerge(sassLoader, {
test: /\.(scss|sass)$/i,
use: [{ loader: 'sass-loader', options: { sourceMap: true } }]
use: [{
loader: 'sass-loader',
options: { sourceMap: true }
}]
})

module.exports = sassLoader
4 changes: 3 additions & 1 deletion package/rules/typescript.js
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'
}]
}
13 changes: 13 additions & 0 deletions package/rules/url.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
const assetHost = require('../asset_host')

module.exports = {

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?

Copy link
Member Author

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

Copy link

@farthinker farthinker Dec 7, 2017

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?

test: [/\.bmp$/, /\.gif$/, /\.jpe?g$/, /\.png$/],
use: [{
loader: 'url-loader',
options: {
limit: 10000,
name: '[name]-[hash].[ext]',
publicPath: assetHost.publicPathWithHost
}
}]
}
8 changes: 5 additions & 3 deletions package/utils/deep_merge.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
const { isObject, isArray, isEqual } = require('./helpers')
const {
isObject, isArray, isEqual, isEmpty
} = require('./helpers')

const deepMerge = (target, source) => {
if (!target) return source
if (!source) return target
if (isEmpty(target)) return source
if (isEmpty(source)) return target
if (isEqual(target, source)) return source
if (isArray(target) && isArray(source)) return [...new Set([...target, ...source])]
if (!(isObject(target) && isObject(source))) return source
Expand Down
3 changes: 3 additions & 0 deletions package/utils/helpers.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ const isObject = value => (
(value.length === undefined || value.length === null)
)

const isEmpty = value => (value === null || value === undefined)

const isString = key => key && typeof key === 'string'

const isStrPath = (key) => {
Expand All @@ -23,6 +25,7 @@ module.exports = {
isObject,
isArray,
isEqual,
isEmpty,
isStrPath,
canMerge,
prettyPrint
Expand Down
Loading