Replace sprockets/browserify with Webpack #2617

Gargron · Apr 29, 2017

Fix #1712, #1742

What this toolchain upgrade provides:

Live reloads during development
Friendlier asset compilation output (e.g. progress, final bundle size)
Code splitting, async chunk loading, smarter minifying
Automated management of JS translations

Untested potential benefits:

Smaller bundle size?

TODO:

No more best_in_place for admin site settings - needs replacement
No more local_time JS - needs integration or replacement
Make custom.scss work again

Development implications:

Run ./bin/webpack-dev-server at the same time as rails s
Run yarn run manage:translations to get a list of all missing/redundant translations in the web UI

@Gargron Great change!

But, application.js is depends on jQuery. see #2465

ykzts · Apr 29, 2017

@Gargron Great change!

But, application.js is depends on jQuery. see #2465

@ykzts Unfortunately, jQuery is still a dependency because of jquery-ujs which makes some non-GET links work. There is rails-ujs which is jQuery-free but it didn't work with webpack. jQuery is also used in public.js. On the upside, webpack splits the code into vendor.js and application.js/public.js bundles, so dependencies are not loaded twice.

Gargron · Apr 29, 2017

@ykzts Unfortunately, jQuery is still a dependency because of jquery-ujs which makes some non-GET links work. There is rails-ujs which is jQuery-free but it didn't work with webpack. jQuery is also used in public.js. On the upside, webpack splits the code into vendor.js and application.js/public.js bundles, so dependencies are not loaded twice.

@Gargron Uh... As far as I see, jquery-ujs is not used in the part where React is used. Even if you delete it, application.js will run with the exception of the notification.

ykzts · Apr 29, 2017

@Gargron Uh... As far as I see, jquery-ujs is not used in the part where React is used. Even if you delete it, application.js will run with the exception of the notification.

The web UI has one logout button which needs jquery-ujs

Gargron · Apr 29, 2017

The web UI has one logout button which needs jquery-ujs

Oh... I see. Certainly. I'm sorry.

ykzts · Apr 29, 2017

Oh... I see. Certainly. I'm sorry.

I don't understand why view specs are failing with this cryptic error. I changed nothing about them...

Gargron · Apr 29, 2017

I don't understand why view specs are failing with this cryptic error. I changed nothing about them...

nolanlawson · Apr 30, 2017

nolanlawson approved these changes Apr 30, 2017

View changes

config/webpack/production.js

nolanlawson · Apr 30, 2017

config/webpack/production.js

+        cascade: true,
+        keep_fargs: false,
+        warnings: true
+      },


Just setting compress to true should be fine, unless there' is something in particular you wanted to tweak here?

Was implementing suggestions from this comment: #1712 (comment)

I dunno, most every JS project I know of just uses uglify with mangle and compress set to true. You can tweak the compress settings in rare cases, but in my mind "mangle/compress" (aka uglifyjs -mc) is basically Uglify's "default" setting. :)

nolanlawson · Apr 30, 2017

config/webpack/production.js

+        warnings: true
+      },
+
+      mangle: false,


I don't often see mangle set to false, would this break something to turn on?

Afaik it would make trying to debug production errors way harder..?

A good solution for that is sourcemaps. :)

if mastodon published sourcmaps in prod I would be all about that. I personally like having mangle off otherwise though.

nolanlawson · Apr 30, 2017

config/webpack/production.js

+      algorithm: 'gzip',
+      test: /\.(js|css|svg|eot|ttf|woff|woff2)$/
+    })
+  ]


It'd be a good idea to also use the DefinePlugin to ensure that NODE_ENV is set to production which cuts out a lot of development-only code in React as well as other libraries (React docs on the subject). I also opened up a PR for doing the equivalent thing in Browserify FWIW: #2635

rails/webpacker sets NODE_ENV=production in bin/webpack

Huh that's odd, React's docs still say to add DefinePlugin, but maybe it's no longer necessary for Webpack 2? Just checked the output bundles after asset:precompile and I'm not seeing any NODE_ENV references, so LGTM 👍

FWIW, just tested the bundle size, and before this PR application.js is 2.97 MB, afterwards application.js is 563 kB and vendor.js is 1.57 MB, so combined they're 2.14 MB which is a huge reduction. Very nice! 🎉

nolanlawson · Apr 30, 2017

FWIW, just tested the bundle size, and before this PR application.js is 2.97 MB, afterwards application.js is 563 kB and vendor.js is 1.57 MB, so combined they're 2.14 MB which is a huge reduction. Very nice! 🎉

Travis keeps breaking because node-sass needs some binaries that apparently don't get cached by Travis?

Gargron · Apr 30, 2017

Travis keeps breaking because node-sass needs some binaries that apparently don't get cached by Travis?

Here are upgrade notes:

Ensure you have Node 6.x and not less
Install bundle/yarn dependencies
If it complains about node-sass, npm rebuild node-sass --force
Make sure CDN_HOST contains protocol part, if you are using it
Precompile, restart

Gargron · Apr 30, 2017

Here are upgrade notes:

Ensure you have Node 6.x and not less
Install bundle/yarn dependencies
If it complains about node-sass, npm rebuild node-sass --force
Make sure CDN_HOST contains protocol part, if you are using it
Precompile, restart

I'm not sure if you are happy for people to provide feedback here.
Feel free to tell me off if you don't think it is appropriate.

This branch is live on glitch.social (thanks Bea).
I've been trying for a while and there is noticeable improvement.
It definitely helps with recent issues I had on mobile #2648

(Edit: typo)

kyzh · Apr 30, 2017

I'm not sure if you are happy for people to provide feedback here.
Feel free to tell me off if you don't think it is appropriate.

This branch is live on glitch.social (thanks Bea).
I've been trying for a while and there is noticeable improvement.
It definitely helps with recent issues I had on mobile #2648

(Edit: typo)

mjankowski · May 1, 2017

mjankowski reviewed May 1, 2017

View changes

Gemfile

mjankowski · May 1, 2017

mjankowski reviewed May 1, 2017

View changes

mjankowski · May 1, 2017

app/javascript/styles/basics.scss

@@ -1,6 +1,6 @@
 body {
  font-family: 'Roboto', sans-serif;
-  background: $color1 image-url('background-photo.jpg');
+  background: $color1 url('../images/background-photo.jpg');


It seems weird that the stylesheets move into app/javascript/... -- is that normal for webpack?

mjankowski · May 1, 2017

mjankowski reviewed May 1, 2017

View changes

mjankowski · May 1, 2017

app/javascript/styles/fonts/montserrat.scss

@@ -0,0 +1,11 @@
+@font-face {


Same question here ... it seems weird that the fonts move into app/javascript/... -- is that normal?

It's not normal but if I leave anything in assets it will get picked up by sprockets, I think, and then it would get processed twice.

I guess I would have assumed that the JS moves to app/javascript but anything which really is still just a static asset either a) stays in app/assets (and is made available to the JS, where needed? I think webpack does this...?), or b) moves to like - app/fonts or something?

The thing I'm asking about here isn't the move away from sprockets and to webpack, but about the naming convention of having fonts and images in an app/javascript dir...

It isn't too weird really, as some React practices involve bundling CSS modules directly with components, e.g. CSS and JS side by side, with any relevant images included. We don't use that but it's still all close together.

mjankowski · May 1, 2017

mjankowski reviewed May 1, 2017

View changes

mjankowski · May 1, 2017

config/initializers/assets.rb

@@ -8,6 +8,6 @@

 # Precompile additional assets.
 # application.js, application.css, and all non-JS/CSS in app/assets folder are already added.
-Rails.application.config.assets.precompile += %w(application_public.js custom.css)
+# Rails.application.config.assets.precompile += %w(application_public.js custom.css)


is this left commented out on purpose, or should be a delete?

It's commented out by default so I left it commented out instead of deleting

Should we add a Procfile.dev (for people who use foreman in dev) or bin/server or some other way to include the running of the webpack-dev-server for people to get all in one process alongside the rails server and workers?

mjankowski · May 1, 2017

Should we add a Procfile.dev (for people who use foreman in dev) or bin/server or some other way to include the running of the webpack-dev-server for people to get all in one process alongside the rails server and workers?

mjankowski · May 3, 2017

mjankowski approved these changes May 3, 2017

View changes

I'm approving this on the assumption that we continue to find edge cases and work out best practices once the bulk of it is in.

On second analysis, it looks like this didn't shrink the bundle size much. 😕

796K	application-6d6d82d4e4776ad9b07e.js
8.0K	public-39f8a254bfe861a34311.js
2.0M	vendor-5e5fbf294d06423aa79e.js

Looks like we're still at around 2.8MB of JavaScript total. I've run webpack-bundle-analyzer on the latest master branch (4d22d03); here's a live view of the data. A few things jump out at me:

emojione takes up a ton, hopefully emojione/emojione#489 will get merged soon
all the various language JSON files take up a lot inside of Mastodon itself
buffer is unnecessary; it's required by an unused API in http-link-header
Lodash takes up a bit, worth looking into lodash-webpack-plugin and babel-plugin-lodash
Using Rollup, Mastodon's code could be one big index.js instead of many small modules

Mostly I don't see any easy wins here; just need to assiduously go through dependencies and try to remove what's not needed, replace where possible, etc. Code-splitting would also be good. Keep in mind that Sean Larkin from the Webpack team recommends <300KB (without gzip) for each chunk, whereas Addy Osmani on the Chrome team recommends <100KB to hit <3 seconds on 3G.

nolanlawson · May 5, 2017

On second analysis, it looks like this didn't shrink the bundle size much. 😕

796K	application-6d6d82d4e4776ad9b07e.js
8.0K	public-39f8a254bfe861a34311.js
2.0M	vendor-5e5fbf294d06423aa79e.js

Looks like we're still at around 2.8MB of JavaScript total. I've run webpack-bundle-analyzer on the latest master branch (4d22d03); here's a live view of the data. A few things jump out at me:

emojione takes up a ton, hopefully emojione/emojione#489 will get merged soon
all the various language JSON files take up a lot inside of Mastodon itself
buffer is unnecessary; it's required by an unused API in http-link-header
Lodash takes up a bit, worth looking into lodash-webpack-plugin and babel-plugin-lodash
Using Rollup, Mastodon's code could be one big index.js instead of many small modules

Mostly I don't see any easy wins here; just need to assiduously go through dependencies and try to remove what's not needed, replace where possible, etc. Code-splitting would also be good. Keep in mind that Sean Larkin from the Webpack team recommends <300KB (without gzip) for each chunk, whereas Addy Osmani on the Chrome team recommends <100KB to hit <3 seconds on 3G.

hinaloe · May 31, 2017

hinaloe reviewed May 31, 2017

View changes

Dockerfile

Gargron added some commits Apr 29, 2017

Replace browserify with webpack

b43bb79

Add react-intl-translations-manager

Loading status checks…

57b4a3d

Do not minify in development, add offline-plugin for ServiceWorker ba…
…ckground cache updates

Loading status checks…

3bb06c8

ykzts referenced this pull request Apr 29, 2017
Closed
Upgrade react-rails to v2.x #2465

Gargron added some commits Apr 29, 2017

Adjust tests and dependencies

Loading status checks…

7b323c5

Merge branch 'master' into feature-webpack

Loading status checks…

50561fe

Fix production deployments

Loading status checks…

ae6bdc9

Gargron referenced this pull request Apr 30, 2017
Closed
Remove Intl polyfill #2633

Remove offline-plugin and babili

Loading status checks…

56ece91

nolanlawson referenced this pull request Apr 30, 2017
Closed
Use envify + NODE_ENV=production for smaller JS bundle #2635

mjankowski referenced this pull request May 1, 2017
Closed
CLI utility to check coverage of a locale #1742
1 of 1 task complete

Fix issue with Immutable.List().unshift(...values) not working as exp…
…ected

Loading status checks…

949fccf

Gargron changed the title from [WIP] Replace sprockets/browserify with Webpack to Replace sprockets/browserify with Webpack May 1, 2017

Gargron added some commits May 1, 2017

Merge branch 'master' into feature-webpack

db5db35

Make travis load schema instead of running all migrations in sequence

Loading status checks…

2018526

Add react definitions to places that use JSX

Loading status checks…

9e3e179

Gargron referenced this pull request May 2, 2017
Closed
Increase React component spec coverage #2722
1 of 2 tasks complete

Gargron added some commits May 2, 2017

Merge branch 'master' into feature-webpack

Loading status checks…

d326aad

Add Procfile.dev for running rails, webpack and streaming API at the …
…same time

Loading status checks…

0008a2b

Gargron merged commit f5bf5eb into master May 3, 2017
2 of 3 checks passed

2 of 3 checks passed

codeclimate 8 new issues
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

continuous-integration/travis-ci/push The Travis CI build passed
Details

Gargron deleted the feature-webpack branch May 3, 2017

yhirano55 referenced this pull request May 3, 2017
Merged
update gems #2754

This was referenced May 7, 2017

Merged

Fix Node.js installation recommendation, also use 6.x tootsuite/documentation#210

Closed

Use the production build of react #2763

Closed

Add a Service Worker for offline support #2917

matcha440 referenced this pull request May 11, 2017
Closed
Assets fail to load #2973

Gargron added this to Done in v1.4 roadmap May 11, 2017

y-yagi added a commit to y-yagi/documentation that referenced this pull request May 13, 2017

Add doc about `webpack-dev-server`
Since Webpack used from tootsuite/mastodon#2617, it is necessary to start `webpack-dev-server`.

bb1e1a4

y-yagi referenced this pull request May 13, 2017
Closed
Add doc about `webpack-dev-server` #243

hondash referenced this pull request May 29, 2017
Closed
V1.4.1 custom #2

Aug	SEP	Oct
	05
2017	2018	2019

Please note that GitHub no longer supports your web browser.

tootsuite/mastodon

Join GitHub today

Replace sprockets/browserify with Webpack #2617

Conversation

Gargron commented Apr 29, 2017 • edited Edited 1 time Gargron edited Apr 30, 2017 (most recent)

Gargron added some commits Apr 29, 2017

This comment has been minimized.

ykzts commented Apr 29, 2017

This comment has been minimized.

Gargron commented Apr 29, 2017

This comment has been minimized.

ykzts commented Apr 29, 2017

This comment has been minimized.

Gargron commented Apr 29, 2017

This comment has been minimized.

ykzts commented Apr 29, 2017

ykzts referenced this pull request Apr 29, 2017

Gargron added some commits Apr 29, 2017

This comment has been minimized.

Gargron commented Apr 29, 2017

Gargron referenced this pull request Apr 30, 2017

Gargron added some commits Apr 30, 2017

nolanlawson approved these changes Apr 30, 2017 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

nolanlawson Apr 30, 2017 • edited Edited 1 time nolanlawson edited Apr 30, 2017 (most recent)

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

nolanlawson commented Apr 30, 2017

This comment has been minimized.

Gargron commented Apr 30, 2017

This comment has been minimized.

Gargron commented Apr 30, 2017

This comment has been minimized.

kyzh commented Apr 30, 2017 • edited Edited 1 time kyzh edited Apr 30, 2017 (most recent)

nolanlawson referenced this pull request Apr 30, 2017

mjankowski referenced this pull request May 1, 2017

Gargron changed the title from [WIP] Replace sprockets/browserify with Webpack to Replace sprockets/browserify with Webpack May 1, 2017

Gargron added some commits May 1, 2017

mjankowski reviewed May 1, 2017 View changes

mjankowski reviewed May 1, 2017 View changes

This comment has been minimized.

mjankowski reviewed May 1, 2017 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

mjankowski reviewed May 1, 2017 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

mjankowski commented May 1, 2017

Gargron added some commits May 2, 2017

Gargron referenced this pull request May 2, 2017

Gargron added some commits May 2, 2017

mjankowski approved these changes May 3, 2017 View changes

Hide details View details Gargron merged commit f5bf5eb into master May 3, 2017 2 of 3 checks passed

2 of 3 checks passed

Gargron deleted the feature-webpack branch May 3, 2017

yhirano55 referenced this pull request May 3, 2017

This comment has been minimized.

nolanlawson commented May 5, 2017 • edited Edited 1 time nolanlawson edited May 5, 2017 (most recent)

This was referenced May 7, 2017

matcha440 referenced this pull request May 11, 2017

Gargron added this to Done in v1.4 roadmap May 11, 2017

y-yagi added a commit to y-yagi/documentation that referenced this pull request May 13, 2017

y-yagi referenced this pull request May 13, 2017

hondash referenced this pull request May 29, 2017

hinaloe reviewed May 31, 2017 View changes

Gargron commented Apr 29, 2017 •

edited

Edited 1 time

Gargron edited Apr 30, 2017 (most recent)

nolanlawson approved these changes Apr 30, 2017

View changes

nolanlawson Apr 30, 2017 •

edited

Edited 1 time

nolanlawson edited Apr 30, 2017 (most recent)

kyzh commented Apr 30, 2017 •

edited

Edited 1 time

kyzh edited Apr 30, 2017 (most recent)

mjankowski reviewed May 1, 2017

View changes

mjankowski reviewed May 1, 2017

View changes

mjankowski reviewed May 1, 2017

View changes

mjankowski reviewed May 1, 2017

View changes

mjankowski approved these changes May 3, 2017

View changes

Gargron merged commit `f5bf5eb` into master May 3, 2017
2 of 3 checks passed

nolanlawson commented May 5, 2017 •

edited

Edited 1 time

nolanlawson edited May 5, 2017 (most recent)

hinaloe reviewed May 31, 2017

View changes