Problem/Motivation

HTML5 proposes/solidifies asynchronous loading of JavaScript files based on two attributes: defer and async. Drupal has supported defer since at least D6, and support for async is currently in the works (#1140356: Add async, onload property to script tags).

Current behavior is that once you turn JS aggregation on, adding a defer or async property to your drupal_add_js call almost definitely results in the script being bundled into an aggregate without the property set. As a result, code you were expecting to fire asynchronously is now blocking. As such, we should be aggregating async and defer scripts into their own bundles.

Proposed resolution

Presumably, something will be added to drupal_group_js that groups "async" values with one another and "defer" values with one another, respecting their weights.

Remaining tasks

User interface changes

None

API changes

Though this affects JavaScript aggregation, this should have little-to-no API impact.

Files: 
CommentFileSizeAuthor
#9 d8-js_bundling_async_defer-1587536-9.patch3.97 KBarnested
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch d8-js_bundling_async_defer-1587536-9.patch. Unable to apply patch. See the log in the details link for more information. View
#8 d8-js_bundling_async_defer-1587536-8.patch3.43 KBiamEAP
PASSED: [[SimpleTest]]: [MySQL] 36,820 pass(es). View
#2 d8-js_bundling_async_defer-1587536-1.patch2.82 KBiamEAP
PASSED: [[SimpleTest]]: [MySQL] 36,695 pass(es). View
#1 d8-add_async_attr-1140356-24.patch1.99 KBiamEAP
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch d8-add_async_attr-1140356-24_0.patch. Unable to apply patch. See the log in the details link for more information. View

Comments

iamEAP’s picture

Status: Active » Needs review
FileSize
1.99 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch d8-add_async_attr-1140356-24_0.patch. Unable to apply patch. See the log in the details link for more information. View

Wrong patch. Disregard.

iamEAP’s picture

FileSize
2.82 KB
PASSED: [[SimpleTest]]: [MySQL] 36,695 pass(es). View

Here's the right one.

This is totally untested. Just posting to get the conversation rolling.

ericduran’s picture

Status: Needs review » Needs work
+++ b/core/includes/common.incundefined
@@ -4353,31 +4351,39 @@ function drupal_group_js($javascript) {
+          $index = md5(serialize($group_keys));

Why change this to md5? Lets keep this simple and just add the new item to the group_keys.

ericduran’s picture

Issue summary: View changes

Fixing history related to defer attribute.

iamEAP’s picture

In the existing code, if you came across a JS file that had async, the logic would say, "This set of group keys is different than the last one. We need a new bundle," and would proceed to create a new bundle. Then, if the next file didn't need async, but otherwise had all of the same group keys, it'd say, "This set of group keys is different than the last one, so we need a new bundle." Which would result in three separate bundles when only two were needed. We don't want this.

The md5 wrapped around the serialize (which is a technique I'm borrowing from Views, where they use it to generate Views Cache IDs based on a series of keys), just attempts to minimize the number of aggregates.

iamEAP’s picture

Also, I guess I didn't realize drupal_group_js was an API addition to Drupal 8. Utilizing it here might make a backport very murky.

catch’s picture

Priority: Normal » Major

Marking this major now that #1140356: Add async, onload property to script tags is in.

ericduran’s picture

@iamEAP drupal_group_js is an addition in D8 but there is currently a D7 backport that is RTBC which is holding up a couple of other drupal js related patch. The issue is #865536: drupal_add_js() is missing the 'browsers' option. Once that gets in all these changes become a lot easier to backport :)

iamEAP’s picture

Status: Needs work » Needs review
FileSize
3.43 KB
PASSED: [[SimpleTest]]: [MySQL] 36,820 pass(es). View

Forgot the browsers property for bundling.

Setting this back to needs review for more/wider discussion.

arnested’s picture

FileSize
3.97 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch d8-js_bundling_async_defer-1587536-9.patch. Unable to apply patch. See the log in the details link for more information. View

I tried the patch in #8 and it seems to group the async scripts in its own group (I have not made a thorough review though).

But what the patch did not do was to actually put the async attribute on the script tag of the aggregated script.

Attached patch adds the attributes (also for the defer attribute) on the aggregated script tags, but is otherwise the same patch as the one in #8.

iamEAP’s picture

Nevermind. Disregard.

yurtboy’s picture

Applied cleanly

Checking patch core/includes/common.inc...
Hunk #1 succeeded at 4180 (offset -45 lines).
Hunk #2 succeeded at 4256 (offset -45 lines).
Hunk #3 succeeded at 4273 (offset -45 lines).
Applied patch core/includes/common.inc cleanly.
yurtboy’s picture

Applied cleanly

Checking patch core/includes/common.inc...
Hunk #1 succeeded at 4180 (offset -45 lines).
Hunk #2 succeeded at 4256 (offset -45 lines).
Hunk #3 succeeded at 4273 (offset -45 lines).
Applied patch core/includes/common.inc cleanly.
ericduran’s picture

Do we have test that test the different aggregation with the new properties?

xjm’s picture

Issue summary: View changes

Updated issue summary.

tim.plunkett’s picture

Category: bug » task
Priority: Major » Normal

I read the issue summary several times, and I don't see how this is a bug. If it is, can the summary clarify what is broken?

iamEAP’s picture

Suppose you add a JS file like so:

drupal_add_js('/path/to/my/file.js', array('defer' => TRUE));

When JS aggregation is disabled, this shows up just fine with the defer attribute set properly. However, once you turn aggregation on, the script you added will almost definitely be bundled into an aggregate without the defer property set. As a result, code you were expecting to fire asynchronously now fires in line. The same is true for the async property.

In other words, your async/defer scripts don't behave as expected if you have JS aggregation enabled.

iamEAP’s picture

Issue summary: View changes

Updated issue summary.

iamEAP’s picture

Category: task » bug

Updated the summary with the above details.

tim.plunkett’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests, +Needs issue summary update

Okay, that sounds reasonable. Can you copy that into the issue summary?

As @ericduran points out, this should have automated tests.

tim.plunkett’s picture

Issue summary: View changes

Updating summary to describe the bug.

cweagans’s picture

cweagans’s picture

Issue summary: View changes

Further clarification of the problem/motivation/bug.

nod_’s picture

Issue summary: View changes
Issue tags: +Novice, +JavaScript

From what I see it's fixed, anyone can confirm?

mikeytown2’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 9: d8-js_bundling_async_defer-1587536-9.patch, failed testing.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.