It would be nice to offer the user the option of downloading a local copy of a JS file and hence having it included in aggregated JS. Perhaps a couple times a day on cron the file is downloaded again to check for changes?

Obviously there are concerns about licensing, etc, that make this potentially thorny, so it's not a high priority.

Comments

msonnabaum’s picture

StatusFileSize
new4.88 KB

Attached patch adds an option to cache locally if the script is external.

The http request is only fired if the file isn't already cached. I stole the function from google analytics which works this way.

I also added a cron hook to ensure that the cache is always primed.

David_Rothstein’s picture

Title: Download and agregate JS » Download and aggregate JS
Status: Active » Needs work
  1. This makes sense in general. For some of our use cases in Drupal Gardens it might not actually be an overall performance improvement though? (If all the sites are referencing the same exact file, it allows shared caching of the resource, whereas if they switch to each downloading it locally then they're on their own).
  2. Looking at the patch, at first glance I don't see why the cache needs to be primed on hook_cron(), given that javascript_libraries_cache() automatically primes it when the file doesn't exist... Or was the intention here to implement what Peter suggested (automatically force a re-download a couple times a day to check for changes)? In that case, it needs to pass TRUE as the second parameter to javascript_libraries_cache().
  3. +function javascript_libraries_cache($location, $sync_cached_file = FALSE) {
    .....
    +    if ($result->code == 200) {
    +      if (file_exists($file_destination)) {
    .....
    +      }
    +      else {
    .....
    +          // Return the local JS file path.
    +          return file_create_url($file_destination);
    +      }
    +    }
    

    Seems like the function should return file_create_url() somewhere in the "if" statement also?

pwolanin’s picture

I think it depends. Our assumption is that users in general visit only 1 site, so reducing http requests may be much more important than using a shared resource.

If the resource is e.g. a twitter or other JS file used commonly on the web it might make sense not to do this on the assumption that some fair fraction of browsers will have it cached already.

pwolanin’s picture

Feels like this is missing something.

If we have a cached copy it should be included in aggregated JS. however, if the JS is not aggregated, we ideally would use the external URL?

pwolanin’s picture

StatusFileSize
new5.21 KB

Revised patch. not really tested yet, but I think the overall logic is improved.

pwolanin’s picture

how much overhead is the extra N file stats per page load?

pwolanin’s picture

StatusFileSize
new7.17 KB

Revised patched, reduces code duplication, improves logic. Quick local testing looks good.

pwolanin’s picture

Status: Needs work » Needs review
StatusFileSize
new7.59 KB

A couple more improvements to the admin form

pwolanin’s picture

Status: Needs review » Fixed
StatusFileSize
new7.59 KB

improve hook_cron. No need to run the command expect once per day to sync.

Comitting this patch. Open new issues for follow-up.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.