Upgrade to Webpack 3 #10279

evq · 2017-08-03T20:26:55Z

Submitter Checklist:

Submitted a ticket for my issue if one did not already exist.
Used Github auto-closing keywords in the commit message.
Added/updated tests for this change (for new code or code which already has tests).
Ran git rebase -i to squash commits (if needed).
Tagged reviewers and labelled the pull request as needed.

Test Plan:

I tested that the browser started up, the about pages loaded, and webtorrent loaded and functioned correctly in two scenarios:

Running the browser via npm run watch and npm start
Producing a build via CHANNEL=dev npm run build-package and running the resulting binary

Reviewer Checklist:

Tests

Adequate test coverage exists to prevent regressions
Tests should be independent and work correctly when run individually or as a suite ref
New files have MPL2 license header

luixxiul · 2017-08-06T12:54:21Z

note: after this PR is merged, we could test to upgrade file-loader to v1.0.0. Please see: #10203

bsclifton · 2017-08-07T08:19:39Z

.babelrc

@@ -3,7 +3,7 @@
    "react",
    ["env", {
      "targets": {
-        "chrome": 57
+        "chrome": 60


This is a great change too 😄

bsclifton · 2017-08-07T08:22:15Z

cc: @gyandeeps (who had originally provided a webpack 2 upgrade PR that we didn't have enough time/knowledge to review at the time)

gyandeeps · 2017-08-07T15:04:47Z

These changes looks good to me.

The best part about Webpack 3 is webpack.optimize.ModuleConcatenationPlugin which helps in content size reduction as well as improves runtime perf of the code. I would recommend trying that on the repo (at least for prod version). Make sure we test it thoroughly.

luixxiul · 2017-08-07T15:48:42Z

also when I tested the PR I noticed that the CPU usage on npm run watch reduced massively, which helps to prevent our laptops from consuming the battery :-)

cezaraugusto · 2017-08-09T03:15:45Z

webpack.config.js

@@ -23,7 +23,7 @@ function config () {
            path.resolve(__dirname, 'app', 'browser', '*'),
            path.resolve(__dirname, 'app', 'extensions', '*')
          ],
-          loader: 'babel'
+          loader: 'babel-loader'


I guess we should update it to 7.1 to match webpack3. current version (6.2.0) only matches 1.x and 2.x (with deprecation warnings)

ref: https://github.com/babel/babel-loader/

cezaraugusto · 2017-08-09T03:26:28Z

webpack.config.js

+  // In development we overwrite the output path so the dev server serves everything from the gen subdir
+  if (env === 'development') {
+    // Note that this directory will never be created since the dev server does not write out to files
+    merged.output.path = path.resolve(__dirname, 'app', 'extensions', 'gen')


it wrote for me. maybe put in .gitignore? or maybe it will be needed eventually due to hash changes?

also having Content not from webpack is served from /Users/cezaraugusto/dev/browser-laptop/app/extensions/brave, is that expected?

weird, I will add to .gitignore.

that is expected and the same behavior as before (content base was previously set via --content-base=./app/extensions/brave" in the webpack-dev-server params)

cezaraugusto · 2017-08-09T03:29:45Z

I left some comments/questions but looking forward for this. @NejcZdovc can you try this out too? app seems to be working fine and build seems to be faster for local development, looking good to me.

luixxiul · 2017-08-09T03:31:34Z

package.json

-    "webpack": "^1.12.9",
-    "webpack-dev-server": "^1.14.0",
+    "webpack": "^3.4.1",
+    "webpack-dev-server": "^2.6.1",


How about removing the carets for now (to avoid the failures) until those updates are confirmed to work properly and the automated tests are added, because those are major updates and they are not tested thoroughly yet? This is the lesson that I learned when I updated Aphrodite to ^1.2.3. Just my two cents.

@luixxiul you're referring to possible future failures due to minor version bumps? does switching to tildes instead so we can use patch version bumps sound good?

sgtm. if we keep following the development and make sure that failures which might be introduced by future minor updates can be fixed easily, let's change the tildes to carets :-)

NejcZdovc · 2017-08-09T10:29:34Z

@gyandeeps I agree with you that we should be adding more plugins that would help us out with perf and size. What I would suggest that we keep this PR clean and should only include upgrade code and then we create separate PR for introducing new features. This way we prevent any strange regressions and start using new webpack sooner. wdyt all?

gyandeeps · 2017-08-09T14:04:40Z

@NejcZdovc Makes absolute sense. Thanks 😄

evq · 2017-08-09T20:04:36Z

All comments should be addressed, thanks everyone! :)

cezaraugusto

lgtm

cezaraugusto · 2017-08-10T21:39:09Z

ok @bsclifton found a problem when trying to make a build. I did some research and uglify-js seems to have trouble minifying es6. I added babel-preset-es2015 and added the preset to .babelrc which made the build run longer but broke anyway. ATM I'm not able to find a good version of webpack's uglify plugin to make the release happen. I'm labeling PR/blocked until that is solved.

gyandeeps · 2017-08-10T21:54:00Z

i would recommend against babel-preset-es2015 as its tranforms all the code to ES5. which is not good for chrome 60. Thats why way back in the day i moved to babel env plugin which smart enough to tranform code based on platform (chrome 60 in this case).

Uglify js has a es branch but i havent tried it. I would say try babili (babel minifier) which works well. It has some issues but worth a shot.

gyandeeps · 2017-08-10T21:55:19Z

package.json

    "uuid": "^3.0.1",
    "webdriverio": "4.7.1",
-    "webpack": "^1.12.9",
-    "webpack-dev-server": "^1.14.0",
+    "webpack": "~3.4.1",


this is already old 😄 ... latest is 3.5.3

evq · 2017-08-10T22:33:15Z

I got a branch working with babili but it is really, really slow.

https://github.com/brave/browser-laptop/tree/evq/babili

evq · 2017-08-10T23:05:31Z

Branch using uglify-es - https://github.com/brave/browser-laptop/tree/evq/uglify-es

petemill · 2017-08-14T09:53:22Z

What's the goal with uglify / babili for the project? Is it primarily size reduction, code obfuscation or to prevent misuse? I'm guessing the former since the source is open?

Here's some analysis of the different branches:

uglify-es took 1m30s to build the package and resulted in 1.96mb aboutPages.entry.js and 0.98mb app.entry.js
babili took 6m30s to build the package and resulted in 2.01mb aboutPages.entry.js and 0.97mb app.entry.js
webpack3 with no minify took 1m to build the package and resulted in 3.53mb aboutPages.entry.js and 2.19mb app.entry.js
master (webpack 1 w/uglify) took 1m to build the package and resulted in 3.95mb aboutPages.entry.js and 2.75mb app.entry.js
uglify (with mangling turned off) took 1m25s to build the package and resulted in 2.41mb aboutPages.entry.js and 1.42mb app.entry.js

In all cases the package seemed to run fine, albeit limited and manual testing, so maybe for now it's ok to go with uglify-es.

Both uglify and babili have a subset of plugins / options that can be turned off, and that may help with the speed / size.

Hope that helps somewhat to get this moving. Happy to help in any other way.

evq · 2017-08-14T18:25:42Z

@petemill Awesome, thanks for the detailed analysis! :) Seems like uglify-es is a clear winner vs babili, I've squashed it into this branch.

cezaraugusto · 2017-08-14T21:20:45Z

@petemill thanks for the report. Results from me were very similar to yours. I did another one with package size:

CHANNEL=dev npm run build-package

371.9 MB - webpack3 uglify-es
365.9 MB - webpack3 babili
361.5 MB - webpack1

While webpack3 with uglify-es have a bigger size, there are several options we can use to reduce it. I'm endorsing my last approval.

Will defer merging to @bsclifton as he may have other testing cases. @evq thanks for leading this and for everybody that helped. awesome work!

bsclifton

Awesome collaboration on this one, folks 😄
@evq @cezaraugusto @petemill @gyandeeps @luixxiul

I just tried out packaging / creating installer... appears to work flawlessly! 😄

evq added the PR/work-in-progress ⚒ label Aug 3, 2017

evq changed the title ~~WIP Upgrade to Webpack 3~~ Upgrade to Webpack 3 Aug 3, 2017

NejcZdovc assigned evq Aug 4, 2017

evq force-pushed the evq/webpack_3 branch from c267a06 to 62c83f9 Compare August 4, 2017 20:36

evq requested a review from NejcZdovc August 4, 2017 20:40

evq removed the PR/work-in-progress ⚒ label Aug 4, 2017

luixxiul added the dev-setup label Aug 5, 2017

luixxiul added this to the 0.21.x (Nightly Channel) milestone Aug 5, 2017

bsclifton reviewed Aug 7, 2017

View reviewed changes

.babelrc

@@ -3,7 +3,7 @@

"react",

["env", {

"targets": {

"chrome": 57

"chrome": 60

Copy link

Member

bsclifton Aug 7, 2017

Choose a reason for hiding this comment

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

This is a great change too 😄

cezaraugusto reviewed Aug 9, 2017

View reviewed changes

luixxiul reviewed Aug 9, 2017

View reviewed changes

evq force-pushed the evq/webpack_3 branch 2 times, most recently from ce848d6 to 0c161db Compare August 9, 2017 08:36

evq force-pushed the evq/webpack_3 branch from 0c161db to d26e3a8 Compare August 9, 2017 20:03

cezaraugusto approved these changes Aug 10, 2017

View reviewed changes

cezaraugusto added the PR/blocked label Aug 10, 2017

gyandeeps reviewed Aug 10, 2017

View reviewed changes

petemill mentioned this pull request Aug 14, 2017

SwitchControl text now toggles checked state, and is consistently used #10470

Merged

8 tasks

Switch to webpack 3, switch babel to chromium 60

28e575d

evq force-pushed the evq/webpack_3 branch from d26e3a8 to 28e575d Compare August 14, 2017 18:13

cezaraugusto removed the PR/blocked label Aug 15, 2017

bsclifton approved these changes Aug 16, 2017

View reviewed changes

bsclifton added QA/no-qa-needed release-notes/exclude perf labels Aug 16, 2017

bsclifton merged commit 3e8c8dd into master Aug 16, 2017

bsclifton deleted the evq/webpack_3 branch August 16, 2017 05:35

bbondy modified the milestones: 0.21.x (Developer Channel), 0.20.x (Beta Channel) Oct 25, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade to Webpack 3 #10279

Upgrade to Webpack 3 #10279

evq commented Aug 3, 2017 •

edited

Loading

luixxiul commented Aug 6, 2017

bsclifton Aug 7, 2017

bsclifton commented Aug 7, 2017

gyandeeps commented Aug 7, 2017 •

edited

Loading

luixxiul commented Aug 7, 2017

cezaraugusto Aug 9, 2017

cezaraugusto Aug 9, 2017

evq Aug 9, 2017

cezaraugusto Aug 9, 2017

evq Aug 9, 2017

cezaraugusto commented Aug 9, 2017

luixxiul Aug 9, 2017

evq Aug 9, 2017 •

edited

Loading

luixxiul Aug 9, 2017 •

edited

Loading

NejcZdovc commented Aug 9, 2017

gyandeeps commented Aug 9, 2017

evq commented Aug 9, 2017

cezaraugusto left a comment

cezaraugusto commented Aug 10, 2017

gyandeeps commented Aug 10, 2017 •

edited

Loading

gyandeeps Aug 10, 2017 •

edited

Loading

evq commented Aug 10, 2017

evq commented Aug 10, 2017

petemill commented Aug 14, 2017

evq commented Aug 14, 2017

cezaraugusto commented Aug 14, 2017

bsclifton left a comment

Upgrade to Webpack 3 #10279

Upgrade to Webpack 3 #10279

Conversation

evq commented Aug 3, 2017 • edited Loading

luixxiul commented Aug 6, 2017

bsclifton Aug 7, 2017

Choose a reason for hiding this comment

bsclifton commented Aug 7, 2017

gyandeeps commented Aug 7, 2017 • edited Loading

luixxiul commented Aug 7, 2017

cezaraugusto Aug 9, 2017

Choose a reason for hiding this comment

cezaraugusto Aug 9, 2017

Choose a reason for hiding this comment

evq Aug 9, 2017

Choose a reason for hiding this comment

cezaraugusto Aug 9, 2017

Choose a reason for hiding this comment

evq Aug 9, 2017

Choose a reason for hiding this comment

cezaraugusto commented Aug 9, 2017

luixxiul Aug 9, 2017

Choose a reason for hiding this comment

evq Aug 9, 2017 • edited Loading

Choose a reason for hiding this comment

luixxiul Aug 9, 2017 • edited Loading

Choose a reason for hiding this comment

NejcZdovc commented Aug 9, 2017

gyandeeps commented Aug 9, 2017

evq commented Aug 9, 2017

cezaraugusto left a comment

Choose a reason for hiding this comment

cezaraugusto commented Aug 10, 2017

gyandeeps commented Aug 10, 2017 • edited Loading

gyandeeps Aug 10, 2017 • edited Loading

Choose a reason for hiding this comment

evq commented Aug 10, 2017

evq commented Aug 10, 2017

petemill commented Aug 14, 2017

evq commented Aug 14, 2017

cezaraugusto commented Aug 14, 2017

bsclifton left a comment

Choose a reason for hiding this comment

evq commented Aug 3, 2017 •

edited

Loading

gyandeeps commented Aug 7, 2017 •

edited

Loading

evq Aug 9, 2017 •

edited

Loading

luixxiul Aug 9, 2017 •

edited

Loading

gyandeeps commented Aug 10, 2017 •

edited

Loading

gyandeeps Aug 10, 2017 •

edited

Loading