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

Clean up webpack builds #3641

Merged
merged 4 commits into from
Jan 18, 2018
Merged

Conversation

blink1073
Copy link
Contributor

@blink1073 blink1073 commented Jan 14, 2018

  • Adds TypeScript source map support to dev mode and tests when in watch mode. Adds ~2 secs to dev build but gives us TypeScript source maps.
    "In most cases cheap-module-eval-source-map is the best option." - https://webpack.js.org/guides/build-performance/

  • "It's not well known, but whitespace removal and symbol mangling accounts for 95% of the size reduction in minified code for most JavaScript - not elaborate code transforms." - uglify readme

    Production build takes ~1/2 the time and is ~10% bigger with compress: false.

  • I tried the recommended method to speed up TypeScript loading in the tests, but it actually slowed the tests down.

  • Also updates docs around debugging

@blink1073 blink1073 added this to the Beta 2 milestone Jan 14, 2018
@blink1073 blink1073 requested a review from jasongrout January 14, 2018 16:14
@jasongrout
Copy link
Contributor

Thanks!

Is it easy to turn off mangling in dev mode? I typically don't use source maps because the code the browser is running is not the same as the code shown in the source window, so it's much harder to inspect and step through code with the browser debugging tools.

It would be great if jupyter lab build also had a switch to turn off mangling, but that's not as important as dev mode, I think.

@jasongrout
Copy link
Contributor

jasongrout commented Jan 17, 2018

For example, here is a diff of the config files that gives what I personally would prefer in a development build. I realize that the common case of a user building should probably have some form of uglify running to make the code size much smaller.

diff --git a/dev_mode/webpack.config.js b/dev_mode/webpack.config.js
index d4e86022b..00a927f89 100644
--- a/dev_mode/webpack.config.js
+++ b/dev_mode/webpack.config.js
@@ -160,7 +160,6 @@ module.exports = {
     fs: 'empty'
   },
   bail: true,
-  devtool: 'cheap-module-eval-sourcemap',
   plugins: [
     new HtmlWebpackPlugin({
       template: path.join('templates', 'template.html'),
diff --git a/dev_mode/webpack.prod.config.js b/dev_mode/webpack.prod.config.js
index 156d98548..839511eff 100644
--- a/dev_mode/webpack.prod.config.js
+++ b/dev_mode/webpack.prod.config.js
@@ -1,23 +1,10 @@
 
-var UglifyJSPlugin = require('uglifyjs-webpack-plugin');
 var merge = require('webpack-merge');
 var webpack = require('webpack');
 var common = require('./webpack.config');
 
 module.exports = merge(common, {
-  devtool: 'source-map',
   plugins: [
-    new UglifyJSPlugin({
-      sourceMap: true,
-      parallel: true,
-      uglifyOptions: {
-        beautify: false,
-        ecma: 6,
-        mangle: true,
-        compress: false,
-        comments: false,
-      }
-    }),
     new webpack.DefinePlugin({
       'process.env.NODE_ENV': JSON.stringify('production')
     })
diff --git a/jupyterlab/staging/webpack.config.js b/jupyterlab/staging/webpack.config.js
index 0b9e29410..00a927f89 100644
--- a/jupyterlab/staging/webpack.config.js
+++ b/jupyterlab/staging/webpack.config.js
@@ -160,7 +160,6 @@ module.exports = {
     fs: 'empty'
   },
   bail: true,
-  devtool: 'cheap-source-map',
   plugins: [
     new HtmlWebpackPlugin({
       template: path.join('templates', 'template.html'),
diff --git a/jupyterlab/staging/webpack.prod.config.js b/jupyterlab/staging/webpack.prod.config.js
index 1a53a1177..ef3b1f501 100644
--- a/jupyterlab/staging/webpack.prod.config.js
+++ b/jupyterlab/staging/webpack.prod.config.js
@@ -1,5 +1,4 @@
 
-var UglifyJSPlugin = require('uglifyjs-webpack-plugin');
 var merge = require('webpack-merge');
 var webpack = require('webpack');
 var os = require('os');
@@ -7,18 +6,7 @@ var os = require('os');
 var common = require('./webpack.config');
 
 module.exports = merge(common, {
-  devtool: 'source-map',
   plugins: [
-    new UglifyJSPlugin({
-      sourceMap: true,
-      parallel: os.cpus().length,
-      uglifyOptions: {
-        beautify: false,
-        ecma: 6,
-        compress: true,
-        comments: false,
-      }
-    }),
     new webpack.DefinePlugin({
       'process.env.NODE_ENV': JSON.stringify('production')
     })

@blink1073
Copy link
Contributor Author

blink1073 commented Jan 17, 2018

There is no uglify when running jlpm run build, that is only done in with build:dev:prod. We recently added options to not uglify the app build either: #3594.

I think this PR improves development builds, because the created source maps are in TypeScript, making it more palatable to use features like await.

@jasongrout
Copy link
Contributor

Thanks, I didn't know about the --dev switch. Let me test with that...

@jasongrout
Copy link
Contributor

Is there a way for me to run essentially jupyter lab --dev-mode --watch --dev, where I get automatic watching of files, running in dev mode, and no uglifying? Can I do this with some combination of of jlpm run watch and jupyter lab --dev-mode?

@jasongrout
Copy link
Contributor

I was getting confused (I've been puzzling through the various build options and code...). It turns out that the sourcemap generation itself is what is making it impossible to debug without source maps. I'll try experimenting with different sourcemap options.

@jasongrout
Copy link
Contributor

It looks like it's the eval source map options that make it impossible to debug the code without source maps enabled, because those options put the module into a string that is evaluated.

Looking at the non-eval options that preserve the typescript source mapping, we have:

  • source-map - statement-level resolution?
  • cheap-module-source-map - single-line resolution

Here are some rough average incremental compilation timing results from each mode (cheap-module-eval-source-map, cheap-module-source-map, source-map), calculated by making a change to short and long source modules and watching the timing information.

  • For each mode, the wall clock time interval reported between File change detected. Starting incremental compilation... and Compilation complete. Watching for file changes. was 3 seconds.
  • Webpack also reported a timing, like this:
    Hash: 9b6cd69259929855d1d4
    Version: webpack 2.7.0
    Time: 631ms
    
    The rough averages for the timings reported were:
    • source-map: 2.2s
    • cheap-module-source-map: 700ms
    • cheap-module-eval-source-map: 500ms

It didn't feel like any one was slower than another, manually timing from when I hit save to when webpack finished printing its output - they all took around 8s, but the variation seemed to swamp the actual time - I saw times between 6s to 11s.

So - I'd be in favor of enabling source-map, as it gives the best debugging experience, and doesn't seem to be too much more time, at least on my computer. If not source-map, can we change to cheap-module-source-map?

@blink1073
Copy link
Contributor Author

I'd be in favor of cheap-module-source-map, both from a build and load time standpoint, though it isn't recommended by webpack, so I could be convinced to use source-map always.

@@ -160,7 +160,7 @@ module.exports = {
fs: 'empty'
},
bail: true,
devtool: 'cheap-source-map',
devtool: 'cheap-module-eval-sourcemap',
Copy link
Contributor

Choose a reason for hiding this comment

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

As noted in the comments, can we change this back to cheap-source-map (or even source-map - it doesn't seem to take that much longer for incremental compilation for me) - the eval version makes it really tedious to debug without using the source map.

@jasongrout
Copy link
Contributor

I'd be in favor of cheap-module-source-map, both from a build and load time standpoint, though it isn't recommended by webpack, so I could be convinced to use source-map always.

Either one is fine with me - I don't typically debug using source maps. I think webpack doesn't recommend it just because it's a compromise, but it might be the right compromise for us.

If I did debug using source maps, I'd probably want source-map, as it's kind of annoying sometimes not to be able to set a breakpoint in the middle of a line, etc.

@blink1073
Copy link
Contributor Author

Fair point, changing to source-map

@jasongrout
Copy link
Contributor

Fair point, changing to source-map

Awesome, thanks!

@jasongrout
Copy link
Contributor

Travis failures are unrelated doc errors.

@jasongrout jasongrout merged commit dd79cfe into jupyterlab:master Jan 18, 2018
@blink1073 blink1073 deleted the webpack-cleanup branch February 15, 2018 19:53
@blink1073 blink1073 restored the webpack-cleanup branch February 27, 2018 22:37
@blink1073 blink1073 deleted the webpack-cleanup branch July 16, 2018 10:39
@lock lock bot added the status:resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion. label Aug 8, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Aug 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement status:resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion. tag:Build System
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants