This module unexpectedly loads all libraries set to use a different version of jQuery all the time.

I have some custom js that runs on certain pages and requires certain libraries, so on that page I include the required libraries using drupal_add_js() - in the order they have to be added in.

By using the patch at #1553526: Don't reorder exisiting javascript files, this works as the module is then respecting my library order.

However, on pages where I don't include the libraries (because they aren't needed), this module loads them anyway, in its own order.
So I am getting js loaded that isn't being used, which is a performance hit; and I'm getting js errors because the libraries are being loaded in the incorrect order, which is breaking my site.

I would suggest not loading any libraries that haven't already been added. Why does this module need to add libraries when the person using the libraries will have already done it using drupal_add_js() or some other method?

Alternatively, if you still want to do that, I would suggest a checkbox on the admin page to select the behaviour you want.
Again, I would suggest that the default would be to not include these libraries, because people will find they site might break, however maybe to not change existing functionality on existing users maybe the default should be to include them.
- Although, maybe the default could be off and there can be an update function to set it on for existing sites.

Also, for people who choose to have the libraries included magically, when they aren't already there, they must be able to reorder the libraries in the settings so that library dependencies can be honoured.
So maybe a draggable table of checkboxes to store the weight and save them in the correct order so when they are added they are added in order.
- Maybe that should have it's own issue?

- I kind of think this is critical because it can completely break js across your site, but there is a workaround that is to just include your libraries on every page and take the performance hit.

CommentFileSizeAuthor
#11 jqmulti-add_reorder_js-1556578-11.patch11.37 KBgoron
#7 jqmulti-jqmulti_libraries_always-d6-1556578-6.patch6.14 KBgoron
#5 jqmulti-jqmulti_libraries_always-d6-1556578-5.patch5.51 KBgoron
#4 jqmulti-jqmulti_libraries_always-1556578-4.patch4.01 KBgoron
#3 jqmulti-jqmulti_libraries_always-1556578-3.patch4 KBrooby
#2 jqmulti-jqmulti_libraries_always-1556578-2.patch4.01 KBrooby

Comments

rooby’s picture

rooby commented

I ran into an issue making a patch to fix this.

The issue is that jqmulti_get_files() is getting files from a number of different sources.
Files coming in from hook_jqmulti_libraries() and hook_jqmulti_files() need to be always included, but files coming in via the 'jqmulti_libraries' variable should not be blindly included on every page.

Because the list returned from jqmulti_get_files() doesn't contain any information regarding where it has come from we have no way of stripping out files that shouldn't be loaded on every page.

It could be done within the jqmulti_get_files() function but then we lose caching benefits for that part of the function because it has to be checked on every page.

I know you might way just use hook_jqmulti_libraries() instead of drupal_add_js(), but that doesn't cover the times when some other drupal contrib module is using drupal_add_js().

Plus there is also a weighting issue with those hooks too, see #1556624: hook_jqmulti_files() & hook_jqmulti_libraries() should have weighting functionality.

So:
* This issue
* #1556624: hook_jqmulti_files() & hook_jqmulti_libraries() should have weighting functionality
* #1553526: Don't reorder exisiting javascript files

Are all semi related and a bit of rewriting might be in order, although #1553526: Don't reorder exisiting javascript files is separate enough to go by itself.

That will take a bit longer than I have to spend at this time so I might have to come back to do it properly.

rooby’s picture

rooby commented
Status: Active » Needs review
StatusFileSize
new jqmulti-jqmulti_libraries_always-1556578-2.patch4.01 KB

I'm not really seeing a way around it that still keeps caching for everything.

Also, from #1:

I know you might way just use hook_jqmulti_libraries() instead of drupal_add_js(), but that doesn't cover the times when some other drupal contrib module is using drupal_add_js().

That isn't even valid because hook_jqmulti_libraries() doesn't allow you to contol what pages a library is displayed on and also has no concept of weight.

Here is the quick patch I use.
It is likely not suitable for everyone as it means even libraries included in hook_jqmulti_libraries() are not added to the page unless they have already been added via drupal_add_js().

rooby’s picture

rooby commented
Title: Don't add libraries that aren't being used » Don't add libraries/files that aren't being used
StatusFileSize
new jqmulti-jqmulti_libraries_always-1556578-3.patch4 KB

Actually, now I think about it, this setting should affect libraries using jqmulti via hook_jqmulti_libraries() & hook_jqmulti_files().

Here is a new patch, with the only difference being re-wording of the description of the setting.

Now the setting means that if you have it unchecked, jqmulti will not attach any javascript to the page that hasn't already been properly added, for example via drupal_add_js().

This includes hook_jqmulti_libraries() & hook_jqmulti_files().
So if you add a library using hook_jqmulti_libraries() all it is doing is making that library use jqmulti's jQuery.
You still have to add the files yourslef when you need them via drupal_add_js() or similar.

Which is actually how I would expect this module to work anyway.

This module is providing the ability to make your libraries use a different version of jQuery.
ie. wrapping files between a different version of jQuery and switch.js.
That's it. It wouldn't expect it to be actually attaching all the libraries and files.

This patch is now completely un-reliant on any of the previously mentioned issues.

goron’s picture

goron commented
StatusFileSize
new jqmulti-jqmulti_libraries_always-1556578-4.patch4.01 KB

Here's a reroll of the patch so it applies cleanly to current dev (I added an update hook). Still checking out this patch. I definitely agree with the idea and the implementation looks good. Looking at the details.

goron’s picture

goron commented
Status: Needs review » Patch (to be ported)
StatusFileSize
new jqmulti-jqmulti_libraries_always-d6-1556578-5.patch5.51 KB

I got the chance to look at this because I needed to fix this issue for a D6 site. So I've committed this to the 6.x branch.

I'm including the patch here. There were only 2 things changed because of the Drupal version. The rest of the changes from the patch in #4 are wording changes. A patch for D7 based off this one is welcome. Otherwise I'll do my best to port asap.

goron’s picture

goron commented

Thanks rooby!

http://drupalcode.org/project/jqmulti.git/commit/0757c77

goron’s picture

goron commented
StatusFileSize
new jqmulti-jqmulti_libraries_always-d6-1556578-6.patch6.14 KB

And of course I forgot the most important part of the functionality. Included is the complete patch and the final commit is http://drupalcode.org/project/jqmulti.git/commit/2e6872c

deciphered’s picture

deciphered commented
Status: Patch (to be ported) » Needs work 
+++ b/jqmulti.moduleundefined
@@ -88,6 +88,12 @@ function jqmulti_alter_scripts($scripts) {
+    $files = array_intersect_key($files, $modulejs);

Where exactly did '$modulejs' come from? It's not defined anywhere or referenced anywhere other than this line, and therefore causes errors.

deciphered’s picture

deciphered commented
Status: Needs work » Patch (to be ported)

Strike that, bad assumption that the last patch was supposed to work against D7 version as is... it's not.

rooby’s picture

rooby commented

So based on #5, all we need is the patch from #3 with the text changes from #7 for latest dev?

Is that right?

goron’s picture

goron commented
Title: Don't add libraries/files that aren't being used, don't reorder files » Don't add libraries/files that aren't being used
Status: Needs review » Patch (to be ported)
StatusFileSize
new jqmulti-add_reorder_js-1556578-11.patch11.37 KB

Ok, here's a new patch for D7. This revamps the way jqmulti adds the JS files.
- Adds a "load libraries always" checkbox that makes the module act like it has until now - load all selected libraries on every page
- Enables this option for all existing installs to make sure module behavior doesn't change
- If this option is disabled, does not load files unless they've been added in some Drupal method (the main point of this issue)
- Does not reorder JS files, except ones that are added to jqmulti. For those, it doesn't reorder relative to each other (i.e. if you add a bunch of library js and also target to jqmulti, they will be reordered to be added with all the other jqmulti stuff, but should still maintain the right order relative to each other)
- Cleans up code
- Improves README and adds the new "load always" option to the README

I need help testing this and making sure everything works as it should. Please try applying the patch and let me know what scenario worked or didn't work. I will have limited time to do more testing, but can't commit this till it gets fully tested.

goron’s picture

goron commented
Status: Patch (to be ported) » Needs review
goron’s picture

goron commented
Title: Don't add libraries/files that aren't being used » Don't add libraries/files that aren't being used, don't reorder files
goron’s picture

goron commented
Title: Don't add libraries/files that aren't being used » Don't add libraries/files that aren't being used, don't reorder files
Status: Patch (to be ported) » Fixed

Committed

Status: Fixed » Closed (fixed)

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

rooby’s picture

rooby commented
Status: Closed (fixed) » Needs work

This patch removed the code added in #1553526: Don't reorder exisiting javascript files that properly ordered JS files.

This was only added in #11 but wasn't in any of the patches before that.

Do you know why this was added?

rooby’s picture

rooby commented
Status: Needs work » Closed (fixed)

I found a problem in some unrelated code so at this stage I'm taking that back :)
If I do actually find a problem here I'll reopen again.
Sorry for the noise.