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

Replace sprockets/browserify with Webpack #2617

Merged
merged 22 commits into from May 3, 2017

Conversation

Projects
7 participants
@Gargron
Member

Gargron commented 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
@ykzts

This comment has been minimized.

Collaborator

ykzts commented Apr 29, 2017

@Gargron Great change!

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

@Gargron

This comment has been minimized.

Member

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

@ykzts

This comment has been minimized.

Collaborator

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

@Gargron

This comment has been minimized.

Member

Gargron commented Apr 29, 2017

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

@ykzts

This comment has been minimized.

Collaborator

ykzts commented Apr 29, 2017

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

@ykzts ykzts referenced this pull request Apr 29, 2017

Closed

Upgrade react-rails to v2.x #2465

Gargron added some commits Apr 29, 2017

@Gargron

This comment has been minimized.

Member

Gargron commented Apr 29, 2017

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

@Gargron Gargron referenced this pull request Apr 30, 2017

Closed

Remove Intl polyfill #2633

@Gargron Gargron force-pushed the feature-webpack branch from 0fbd693 to 13a644f Apr 30, 2017

Gargron added some commits Apr 30, 2017

const webpack = require('webpack')
const merge = require('webpack-merge')
const CompressionPlugin = require('compression-webpack-plugin')
const BabiliPlugin = require('babili-webpack-plugin')

This comment has been minimized.

@nolanlawson

nolanlawson Apr 30, 2017

Collaborator

I'm not sure it's worthwhile to use both Babili and Uglify; AFAIK Uglify still produces smaller code than Babili, although the downside is that it only works on ES5, so e.g. if we wanted a pure ES6 build then that's the point where Babili would be very useful.

This comment has been minimized.

@Gargron

Gargron Apr 30, 2017

Member

This article claimed that babili was preferable to uglify because it could understand the new syntaxes better.

This comment has been minimized.

@nolanlawson

nolanlawson Apr 30, 2017

Collaborator

Yeah so that doesn't really apply to tree-shaking; tree-shaking is about bundling ES modules, which is handled by Webpack 2 before the rest of the ES6-to-ES5 transpilation. You can test it, but I'm fairly certain Uglify alone (with mangle+compress) will still beat Babili, assuming you're outputting ES5 code. (In fact it looks like Babili's own benchmarks still show Uglify winning, although I know they're making progress.)

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

This comment has been minimized.

@nolanlawson

nolanlawson Apr 30, 2017

Collaborator

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

This comment has been minimized.

@Gargron

Gargron Apr 30, 2017

Member

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

This comment has been minimized.

@nolanlawson

nolanlawson Apr 30, 2017

Collaborator

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

warnings: true
},
mangle: false,

This comment has been minimized.

@nolanlawson

nolanlawson Apr 30, 2017

Collaborator

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

This comment has been minimized.

@Gargron

Gargron Apr 30, 2017

Member

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

This comment has been minimized.

@nolanlawson

nolanlawson Apr 30, 2017

Collaborator

A good solution for that is sourcemaps. :)

This comment has been minimized.

@nightpool

nightpool May 2, 2017

Collaborator

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

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

This comment has been minimized.

@nolanlawson

nolanlawson Apr 30, 2017

Collaborator

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

This comment has been minimized.

@Gargron

Gargron Apr 30, 2017

Member

rails/webpacker sets NODE_ENV=production in bin/webpack

This comment has been minimized.

@nolanlawson

nolanlawson Apr 30, 2017

Collaborator

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 👍

@nolanlawson

This comment has been minimized.

Collaborator

nolanlawson commented 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! 🎉

@Gargron

This comment has been minimized.

Member

Gargron commented Apr 30, 2017

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

@Gargron

This comment has been minimized.

Member

Gargron commented 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
@kyzh

This comment has been minimized.

kyzh commented 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)

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

Gemfile Outdated
@@ -58,18 +55,18 @@ gem 'sprockets-rails', require: 'sprockets/railtie'
gem 'statsd-instrument'
gem 'twitter-text'
gem 'tzinfo-data'
gem 'webpacker', github: 'rails/webpacker'

This comment has been minimized.

@mjankowski

mjankowski May 1, 2017

Collaborator

Using the github: style here produces a security warning on bundle install -- using https://.... will fix it.

But also, I think this is on rubygems now: https://rubygems.org/gems/webpacker/versions/1.2

This comment has been minimized.

@Gargron

Gargron May 1, 2017

Member

Okay. Just did what the webpacker readme recommended. Will gladly pin the version.

This comment has been minimized.

@mjankowski

mjankowski May 2, 2017

Collaborator

Yeah ... I think this was only possible once Rails 5.1 came out, and before that you DID need to pull from GH repo

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

This comment has been minimized.

@mjankowski

mjankowski May 1, 2017

Collaborator

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

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

This comment has been minimized.

@mjankowski

mjankowski May 1, 2017

Collaborator

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

This comment has been minimized.

@Gargron

Gargron May 1, 2017

Member

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.

This comment has been minimized.

@mjankowski

mjankowski May 2, 2017

Collaborator

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

This comment has been minimized.

@Gargron

Gargron May 2, 2017

Member

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.

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

This comment has been minimized.

@mjankowski

mjankowski May 1, 2017

Collaborator

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

This comment has been minimized.

@Gargron

Gargron May 1, 2017

Member

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

@mjankowski

This comment has been minimized.

Collaborator

mjankowski commented 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?

Gargron added some commits May 2, 2017

Fix missing React import in WarningContainer. Optimize rendering perf…
…ormance by using ImmutablePureComponent instead of

React.PureComponent. ImmutablePureComponent uses Immutable.is() to compare props. Replace dynamic callback bindings in
<UI />

@Gargron Gargron referenced this pull request May 2, 2017

Closed

Increase React component spec coverage #2722

1 of 2 tasks complete
@mjankowski

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.

@Gargron Gargron merged commit f5bf5eb into master May 3, 2017

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 Gargron deleted the feature-webpack branch May 3, 2017

@yhirano55 yhirano55 referenced this pull request May 3, 2017

Merged

update gems #2754

@nolanlawson

This comment has been minimized.

Collaborator

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

@matcha440 matcha440 referenced this pull request May 11, 2017

Closed

Assets fail to load #2973

@Gargron 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`.

@hondash hondash referenced this pull request May 29, 2017

Closed

V1.4.1 custom #2

Dockerfile Outdated
@@ -32,14 +30,14 @@ RUN echo "@edge https://nl.alpinelinux.org/alpine/edge/main" >> /etc/apk/reposit
imagemagick@edge \
ca-certificates \
&& npm install -g npm@3 && npm install -g yarn \
&& bundle install --deployment --without test development \
&& yarn --ignore-optional --pure-lockfile \
&& yarn cache clean \

This comment has been minimized.

@hinaloe

hinaloe May 31, 2017

Contributor

Why remove yarn cache clean task? for rails/webpacker#405 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment