Tidy up webpack config #1058

gauravtiwari · 2017-12-01T18:13:04Z

This PR fixes a couple of things related to webpack config:

consistent structure for loaders
deterministic order for loaders, it will start matching modules from top and then first loader that matches the test will be used. This opens possibility for adding different loaders for say processing a svg image instead of using default file loader.
add url loader to process small static assets so it can be embedded directly as data uri instead of a file. https://github.com/webpack-contrib/url-loader
add case sensitive plugin to enforce casing on different platforms: https://github.com/Urthen/case-sensitive-paths-webpack-plugin
remove resolve-url-loader see How to profile this #1042 After testing with standard libraries like bootstrap it seems this loader is redundant and adds more complexity. We also had some security concerns earlier: url() Path Resolution in SASS/SCSS/CSS Stylesheets Exhaustively Searches Entire Project Root #932

gauravtiwari · 2017-12-01T18:18:14Z

The build time is slightly faster too with standard hello examples:

vs

javan · 2017-12-01T18:40:02Z

lib/install/config/webpacker.yml

+      'Access-Control-Allow-Origin': '*'
+    watch_options:
+      ignored: /node_modules/
+


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.

Yeah, just added the important ones that folks would like to change 👍

javan · 2017-12-01T18:40:32Z

package/environment.js

+      fs: 'empty',
+      net: 'empty',
+      tls: 'empty',
+      child_process: 'empty'


What is this for?

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.

https://webpack.js.org/configuration/node/#other-node-core-libraries

@gauravtiwari Is this still needed? Neither the docs or NodeStuffPlugin seem to use it!

javan · 2017-12-01T18:41:11Z

package/environment.js

+        strictExportPresence: true,
+        rules: [
+          { oneOf: this.loaders.values() }
+        ]


What do these options change?

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.

https://webpack.js.org/configuration/module/#rule-oneof

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?

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

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?

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;

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

Oh didn't really look into issues I was looking at the code to figure out why my config was breaking. Thanks @guilleiguaran!

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.

javan · 2017-12-01T18:42:00Z

package/environments/production.js

      },
      output: {
-        comments: false
+        comments: false,
+        ascii_only: true


What do these changes do?

https://github.com/mishoo/UglifyJS2/tree/harmony#output-options

I mean, why are you adding/changing these options? Do they resolve any known issues?

No, we didn't received any but just standardising the options for now (recommended ones).

Recommended where?

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.

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.

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

Just tested, turning it on doesn't increase the build size (all hello examples).

Oops typo there :) (ignore css)

javan · 2017-12-01T18:42:40Z

package/rules/file.js

+    /\.(css)$/i,
+    /\.html$/,
+    /\.json$/
+  ],


Looks like this could be combined to a single regex.

Yep, will do 👍

javan · 2017-12-01T18:43:25Z

The build time is slightly faster too

Where does the speed improvement come from?

gauravtiwari · 2017-12-02T09:49:22Z

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 resolve-url-loader is for very exceptional cases where a node module is using relative imports in css files and doesn't have that resource directly next to it, which means url() lookup won't work. The resolve url loaders, adds some kinda of file searching magic to lookup missing files if they aren't available directly in the path.

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.

gauravtiwari · 2017-12-02T09:51:49Z

Another place, where we might be reaping speed benefits is from using oneOf, since it doesn't go through all the loaders and only applies first matching loader: https://webpack.js.org/configuration/module/#rule-oneof

I will probably benchmark this and post some results here.

gauravtiwari · 2017-12-02T10:49:52Z

So, just tested the oneOf change doesn't really help with performance improvement.

Webpack: 3.5.5

Before

After

javan · 2017-12-02T14:29:31Z

package/environment.js

@@ -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())


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?

Good spot 👁 , will fix 👍

farthinker · 2017-12-07T06:28:03Z

package/rules/url.js

@@ -0,0 +1,13 @@
+const assetHost = require('../asset_host')
+
+module.exports = {


@gauravtiwari How can I reference small static images in Rails view template after using this url loader?

All images under 10KB would use url loader by default, this includes png, jpg, gif and bmp

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?

gauravtiwari requested a review from javan December 1, 2017 18:13

gauravtiwari force-pushed the webpack-config branch from 04f37dc to 5bb008f Compare December 1, 2017 18:21

javan reviewed Dec 1, 2017

View reviewed changes

gauravtiwari force-pushed the webpack-config branch from 5bb008f to e1f6ad7 Compare December 2, 2017 10:35

javan reviewed Dec 2, 2017

View reviewed changes

Tidy up webpack config

3a05570

gauravtiwari force-pushed the webpack-config branch from e1f6ad7 to 3a05570 Compare December 2, 2017 15:35

gauravtiwari merged commit 7738e6b into master Dec 4, 2017

gauravtiwari deleted the webpack-config branch December 4, 2017 18:13

farthinker reviewed Dec 7, 2017

View reviewed changes

n0nick mentioned this pull request Dec 19, 2017

file-loader using an 'exclude' pattern is too aggressive #1132

Closed

		@@ -0,0 +1,13 @@
		const assetHost = require('../asset_host')

		module.exports = {

Tidy up webpack config #1058

Tidy up webpack config #1058

Conversation

gauravtiwari commented Dec 1, 2017

gauravtiwari commented Dec 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gauravtiwari Dec 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gauravtiwari Dec 2, 2017 • edited Loading

Choose a reason for hiding this comment

farthinker Dec 8, 2017 • edited Loading

Choose a reason for hiding this comment

charlesvallieres Dec 14, 2017 • edited Loading

Choose a reason for hiding this comment

guilleiguaran Dec 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gauravtiwari Dec 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javan commented Dec 1, 2017

gauravtiwari commented Dec 2, 2017

gauravtiwari commented Dec 2, 2017

gauravtiwari commented Dec 2, 2017

Before

After

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

farthinker Dec 7, 2017 • edited Loading

Choose a reason for hiding this comment

gauravtiwari Dec 2, 2017 •

edited

Loading

gauravtiwari Dec 2, 2017 •

edited

Loading

farthinker Dec 8, 2017 •

edited

Loading

charlesvallieres Dec 14, 2017 •

edited

Loading

guilleiguaran Dec 14, 2017 •

edited

Loading

gauravtiwari Dec 2, 2017 •

edited

Loading

farthinker Dec 7, 2017 •

edited

Loading