In #769226: Optimize JS/CSS aggregation for front-end performance and DX, the drupal_add_js() API was changed to take JS_* constants as 'group' not as part of 'weight'. 'weight' and 'group' are now separate. Attached very simple patch to implement this.

CommentFileSizeAuthor
#6 ga_js_grouping1.patch1.07 KBhass
ga-drupal-add-js.patch899 bytesgábor hojtsy

Comments

hass’s picture

Have you tested this? I have tried the group param in D7 beta1 or beta2 and it has not worked. The order was still incorrect. This is why I kept it as it is.

hass’s picture

Status: Needs review » Needs work

Tested again with D7 beta3 and group is broken. The code is NOT inserted before jquery.js. I guess it is a core bug, but I haven't cared about it when I found it weeks ago.

hass’s picture

Status: Needs work » Postponed
effulgentsia’s picture

Status: Postponed » Needs work

See #769226-212: Optimize JS/CSS aggregation for front-end performance and DX.

You either need:

array('type' => 'inline', 'group' => JS_LIBRARY, 'every_page' => TRUE, 'weight' => -21)

OR

array('type' => 'inline', 'group' => JS_LIBRARY-1)

Since this is 'inline', the net effect would be the same with either case. If this were a file, the difference would be that the former would result in being in the same aggregate file as the other JS_LIBRARY files, while the latter would result in it being in a different aggregate file.

Properly using the 'every_page' flag is a little confusing, and may need better documentation. Here's what we have so far in the PHPDoc of drupal_add_js():

every_page: For optimal front-end performance when aggregation is enabled, this should be set to TRUE if the JavaScript is present on every page of the website for users for whom it is present at all. This defaults to FALSE. It is set to TRUE for JavaScript files that are added via module and theme .info files. Modules that add JavaScript within hook_init() implementations, or from other code that ensures that the JavaScript is added to all website pages, should also set this flag to TRUE. All JavaScript files within the same group and that have the 'every_page' flag set to TRUE and do not have 'preprocess' set to FALSE are aggregated together into a single aggregate file, and that aggregate file can be reused across a user's entire site visit, leading to faster navigation between pages. However, JavaScript that is only needed on pages less frequently visited, can be added by code that only runs for those particular pages, and that code should not set the 'every_page' flag. This minimizes the size of the aggregate file that the user needs to download when first visiting the website. JavaScript without the 'every_page' flag is aggregated into a separate aggregate file. This other aggregate file is likely to change from page to page, and each new aggregate file needs to be downloaded when first encountered, so it should be kept relatively small by ensuring that most commonly needed JavaScript is added to every page. 

And there's some additional comments in drupal_sort_css_js(). Please suggest improvements.

hass’s picture

This is confusing.. I guess I need to read it again a few times. I need to do some testing to undestand how it works, but the every page sounds very bad as ga does not add code to every page and if nevertheless kept in aggregation it will cause JS errors as the click handler is registered, but the functions are missing. Strange logic if it works this way.

Over all I have not undstand the group stuff yet and why a weight is ignored. Major WTF

hass’s picture

Status: Needs work » Needs review
StatusFileSize
new1.07 KB
hass’s picture

Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

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