662303 - Implement static file compression and bundling

Status: RESOLVED FIXED

(Webtools Graveyard :: Elmo, defect, P2)

Description

[Peter Bengtsson [:peterbe]]

All .js and .css files and ideally possible inline Javascript and CSS to be compressed (aka. minified) and bundled (if possible).

Comment 1

[Peter Bengtsson [:peterbe]]

Attachment: django_compressor fully implemented

This upgrades to elmo-lib with django_compressor. 

Note the changes to scripts/update_site.py. It executes the actual compression offline. The javascript compression is done with rJSmin (http://opensource.perlig.de/rjsmin/) and the CSS compression is done with cssmin (http://pypi.python.org/pypi/cssmin/) both which are bundled in django_compressor. 

Fortunately nothing needs to change in Apache to land this one. That's going to be taken care of in bug 661122.

For development, where you have `DEBUG = True` django_compressor automatically assumes `COMPRESS = not DEBUG` so when in development mode, the compressor tag just voids. However, for the sake of testing this (since our test coverage is still poor) I suggest you put `COMPRESS = True` in your 'settings/local.py' and observe that all static resources are captured properly. 

Note also, that I've tried to "standardize" on the use of jQuery UI. Their CSS files are referenced in 3 templates. Some of them templates don't need ALL files but by making one big common package with all files we'll be able to client cache and use the same file for all templates with only one download. I might consider a new bug later to write that only once in a templatetag so that we don't have to rely on copy-and-paste.

Comment 2

[Axel Hecht [:Pike]]

I don't understand the practical impact of this patch. What do pages get served today, and what after that? Is there a exec summary of that without going through all three subprojects here?

Comment 3

[Peter Bengtsson [:peterbe]]

The exec summary is that:

  <script src="/js/foo.js"></script>
  <script src="/js/bar.js"></script>
  <script>inline function ()</script>

becomes:

  <script src="js/CACHE/bv206x51.js"></script>

The content of the static files are concatenated into one file and also white-space compressed. 

Lastly, due to the unique name of the new file, it's never going to be re-used so you can set 100% aggressive cache headers on that pattern which ultimately makes client have to download less static resources.

Comment 4

[Axel Hecht [:Pike]]

(In reply to Peter Bengtsson [:peterbe] from comment #3)
<...>
> Lastly, due to the unique name of the new file, it's never going to be
> re-used so you can set 100% aggressive cache headers on that pattern which
> ultimately makes client have to download less static resources.

Can you check that? Reading the code, the cache key is only based on the text content in the template, and not on the static content. Which is a good choice perf-wise, just want to make sure that we're having the right expectations.

Also, I'm surprised that django-compressor already landed in elmo-lib, and that it landed as a dump and not a submodule?

Comment 5

[Axel Hecht [:Pike]]

Comment on attachment 550347 [details] [diff] [review]
django_compressor fully implemented

Cancelling the review request, waiting on an answer on my comment 4. Please re-request when we know what we're actually getting here.

Comment 6

[Peter Bengtsson [:peterbe]]

(In reply to Axel Hecht [:Pike] from comment #5)
> Comment on attachment 550347 [details] [diff] [review]
> django_compressor fully implemented
> 
> Cancelling the review request, waiting on an answer on my comment 4. Please
> re-request when we know what we're actually getting here.

Regarding comment 4, you're right! This is really disappointing. I'm not impressed. Going to investigate what my options are.

Comment 7

[Peter Bengtsson [:peterbe]]

Attachment: Only improvement is that it's tracking the latest elmo-lib which now has the latest django_compressor

No changes to the elmo apps template code. Just pointing to a never version of elmo-lib which has very latest django_compressor in it. 

Truthfully, I shouldn't have messed around with elmo-lib the first time. I guess the dirty looking `git status` confused me. 
Being as that was already done (by my mistake) I have at least fixed it by removing it as a dump inside elmo-lib and instead track it as a submodule.

Comment 8

[Peter Bengtsson [:peterbe]]

(In reply to Axel Hecht [:Pike] from comment #4)
> (In reply to Peter Bengtsson [:peterbe] from comment #3)
> <...>
> > Lastly, due to the unique name of the new file, it's never going to be
> > re-used so you can set 100% aggressive cache headers on that pattern which
> > ultimately makes client have to download less static resources.
> 
> Can you check that? Reading the code, the cache key is only based on the
> text content in the template, and not on the static content. Which is a good
> choice perf-wise, just want to make sure that we're having the right
> expectations.
> 
Today was a big bowl of fail. I tried reading the code but couldn't figure out how it's done so I confirmed your suspicion by just testing it. Putting my local settings into production mode, compress, check in Firebug, edit static/css/style.css, compress (again), check in Firebug and it did not change!

So I started a new issue https://github.com/jezdez/django_compressor/issues/111 and set about working on a fork to fix this problem for jezdez. Second failure was that I couldn't fix it. I think I underestimated the complexity code. It was after a bunch of stepping with pdb that I figured out that it **does** depend on the file content. 

The trick is that you first need to run `collectstatic` and *then* run `compress`. The `compress` command relies on inspecting **the files that were collected**!

I was able to confirm this by doing:
  1) run collectstatic
  2) run compress
  3) check filename in Firebug
  4) edit static/css/style.css
  5) run collectstatic
  6) run compress
  7) check filename in Firebug and YES! it changes

All is well. Our update_site.py script makes sure it runs collectstatic always before running compress. 

> Also, I'm surprised that django-compressor already landed in elmo-lib, and
> that it landed as a dump and not a submodule?

See Comment 7 above about this mistake.

Comment 9

[Axel Hecht [:Pike]]

Is there another site where I can see that in action?

Comment 10

[Peter Bengtsson [:peterbe]]

(In reply to Axel Hecht [:Pike] from comment #9)
> Is there another site where I can see that in action?

I don't own any that uses it. If you want to chat about it, let me call you. Hit me up on IRC and we can chat about it.

Comment 11

[Peter Bengtsson [:peterbe]]

(In reply to Axel Hecht [:Pike] from comment #9)
> Is there another site where I can see that in action?

I can make a quick screencast to show you.

Comment 12

[Peter Bengtsson [:peterbe]]

Attachment: Fix for last missing bits that aren't compressed

Fixed missing compression of css and js in pushes and l10nstats/tree_progress.html

Comment 13

[Axel Hecht [:Pike]]

Comment on attachment 557459 [details] [diff] [review]
Fix for last missing bits that aren't compressed

Review of attachment 557459 [details] [diff] [review]:
-----------------------------------------------------------------

Cancelling review, according to our call, we want to go for packaging only for now.

Comment 14

[Peter Bengtsson [:peterbe]]

Attachment: Upgrades to the latest django_compressor commit which fixed the regression

The only difference from before is...

* tracking the latest django_compressor which fixes the showstopper bug I mentioned

* we use a JS compressor that does nothing to the actual content. No compression at all. The CSS is still compressed but CSS is so trivial that it can't go wrong.

Comment 15

[Peter Bengtsson [:peterbe]]

Attachment: This time with tracking release tag 1.1.1

Comment 16

[Axel Hecht [:Pike]]

Comment on attachment 571352 [details] [diff] [review]
This time with tracking release tag 1.1.1

Review of attachment 571352 [details] [diff] [review]:
-----------------------------------------------------------------

::: requirements/prod.txt
@@ +4,5 @@
>  html5lib
>  python-memcached
> +django_compressor
> +django-appconf
> +versiontools

versiontools surprises me here. Apparently that's needed by django-appconf? But how do we get that through appconf being in vendor-local?

Comment 17

[Peter Bengtsson [:peterbe]]

Attachment: Now with versiontools in vendor-local/lib/python

Comment 18

[Peter Bengtsson [:peterbe]]

Finally landed
https://github.com/mozilla/elmo/commit/b72a74e937a3858c515e5d311b452521d22ef2a8

Comment 19

[Axel Hecht [:Pike]]

Adding the two regressions as dependencies, bugs 701216,700521.

I hope we're not going to run into a whole lot more of stuff that breaks between the development and production environments, that'd make hacking on elmo a bit of a gamble.

Comment 20

[Peter Bengtsson [:peterbe]]

I hope so too. However, I'm on this baby like a hawk so if shit merges with the fan I'm all over it.