FinalsClub / karmaworld

KarmaNotes.org v3.0
GNU Affero General Public License v3.0
7 stars 6 forks source link

drop offline caching from django-compressor to support compressing dynamic includes #401

Open btbonval opened 9 years ago

btbonval commented 9 years ago

Follow up from #397 .

When wysihtml5 CSS or JS files are included in compressor and offline compression is enabled, it causes offline key errors which 500 crash the server in production mode. @yourcelf experienced TemplateSyntaxErrors using a system in dev mode.

btbonval commented 9 years ago

If we can reasonably describe the steps to reproduce this issue, it should probably be filed with django-compressor upstream.

yourcelf commented 9 years ago

AHA: http://django-compressor.readthedocs.org/en/latest/usage/#pre-compression explains what went on in #397.

The form media for WYSIHTML5 was provided as a variable (namely, form media), and that variable (or an analog for it) is not specified in the COMPRESS_OFFLINE_CONTEXT setting.

I was thinking about this, and wondering how the offline compression command would populate the form variable in order to discover the media outside of the request/response cycle. The answer is: it doesn't. Unless you explicitly declare context for it to use when generating files for offline compression, the vars won't be defined. In short, django-compressor never saw inside note_edit_form.media during manage.py compress.

Possible solutions:

  1. Just keep media out of compress blocks. PRO: simple, I think we're already doing this now after your fixes in #397. CON: multiple HTTP requests for that media.
  2. List the assets inline, rather than relying on variable form media. PRO: we get asset squashing. CON: duplication.
  3. Define COMPRESS_OFFLINE_CONTEXT to include the form media. I can't think of any reason why this is any better than 2, and it has the CON of being less obvious to people reading the code.

Regarding the wysihtml5 / jsmin parsing: I think the simple solution is that if we put wysihtml5 assets in compress blocks, use the un-minified versions to avoid the crash. Whatever minification wysihtml5 is doing appears to root out a bug in jsmin. The minification we get from compress should do a good enough job. Alternatively, if we just keep wysihtml5 away from django-compressor, using the minified versions of the assets they ship should be fine.

yourcelf commented 9 years ago

possible solution 4: switch away from offline compression in production. This would be essentially the same, except that the first request after the cache is cleared would be delayed while the server generated compressed assets. After that, performance would be roughly identical. This option gives us the most flexibility with least duplication.

btbonval commented 9 years ago

How would the fourth solution resolve the issue? Wouldn't compress still need to combine the static assets together, and still be unable to read the static assets due to variables?

Or does it perform compression on-the-fly for each serve? That would seem to circumvent static hosting.

So far I think the first option is working well and the third option might be something worth exploring; our README documentation is monolithic, so we just need to find a nice place in there to document how compressor works for this system.

I think you nailed the problem though. I'm amazed that django-compressor doesn't have code to check for directives it cannot process, and instead naively tries to parse directives as though they were static assets.

yourcelf commented 9 years ago

The way the fourth option works is:

  1. The compress tag parses its block contents at render-time, and computes a hash identifying an asset.
  2. The compress tag looks in the cache to find an associated filename (basically, something equivalent to manifest.json, but which exists only in the in-memory cache).
  3. If a filename is found in the cache, put that in the html. No further processing. If the name is not found in the cache, compress the contents of the block (running concatenation, minification, etc), write the result using compress's configured storage (e.g. S3), and put the resulting filename into the cache. This first load is slow, because you've got to concatenate, minify, and write to S3 before you can render the page. But subsequent renders hit the cache and are just as fast as offline-mode.

So compress does need to still combine the static assets together; it just writes to s3 on page load (within the request/response cycle) instead of when the management command is run. As a result, it has access to the render context without having to duplicate that.

If you did something silly like handing compress a different dynamic list of assets on each page load, compress would have to keep reconcatenating/minifying/etc. on every page load, and performance would drop drastically. But in this case, the list of assets is always the same unless the definition of the form class changes, so practically, every request beyond the first is the same and just hits the cache to determine the filename.

btbonval commented 9 years ago

I already like the fourth option because it doesn't require manifest.json on the default store. In theory, the software should be running online for long stretches, so keeping that small amount of detail in memory should not be an issue.

btbonval commented 9 years ago

There is a potential problem with CloudFront/S3 latency. If the server pushes to S3, we might not see that result on CloudFront immediately. That's going to require CloudFront to reach back, pull forward, and cache the file on its side.

Thankfully these files are uniquely named.

yourcelf commented 9 years ago

Right: anything compress touches should have a hash of its contents as part of its filename. So if content changes a little bit (as long as we aren't ludicrously unlucky to stumble on a hash collision) it'll be a new filename.

btbonval commented 9 years ago

So if I understand this process, there would never be a need to run manage.py compress, because that compiles the compression list offline. Instead, all compression would happen on-the-fly.

I think that is a big PRO to dropping offline.

btbonval commented 9 years ago

Alright, I'm going to rename this ticket to indicate we'd like to drop compressor offline support, with the goal of putting wyishtml5 back into the compress tags.

This is a lower priority enhancement task since the current solution works fine and doesn't cause problems as-is.