Skip to content
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

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

Open
btbonval opened this issue Feb 18, 2015 · 10 comments

Comments

@btbonval
Copy link
Member

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
Copy link
Member Author

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

@yourcelf
Copy link
Contributor

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 django compressor: is it doing anything? besides 500 crashing Django? #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
Copy link
Contributor

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
Copy link
Member Author

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
Copy link
Contributor

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Contributor

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
Copy link
Member Author

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
Copy link
Member Author

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.

@btbonval btbonval changed the title django-compressor does not like wysihtml5 drop offline caching from django-compressor to support compressing dynamic includes Feb 20, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants