A patch straight from #1542344: Use AMD for JS architecture that declare all JS files as library and add the relevant dependencies to all scripts.

SebCorbin should be credited, he's the one who wrote everything, I'm just putting it in a patch.

This will greatly simplify issues like #1279226: jQuery and Drupal JavaScript libraries and settings are output even when no JS is added to the page as well as prepare core for a module-oriented JS.

Basically, we shouldn't be using drupal_add_js anymore, all scripts should be declared in library_info since pretty much all of them have dependencies on drupal and jQuery. As D8 is focused on mobile, it's critical to be able to add JS that is not implicitly adding jQuery and drupal.js to the page.

The naming isn't totally consistent, feedback welcome. There is still one drupal_add_js left in the color module. not sure how best to fix it yet.

CommentFileSizeAuthor
#113 core-js-explicit-dependencies-1737148-113.patch74.52 KBnod_
#111 core-js-explicit-dependencies-1737148-111.patch74.25 KBnod_
#107 core-js-explicit-dependencies-1737148-107.patch74.26 KBnod_
#107 interdiff.txt3.55 KBnod_
#93 js-deps-1737148-86.patch74.54 KBnod_
#91 core-js-explicit-dependencies-1737148-91.patch73.88 KBnod_
#86 js-deps-1737148-86.patch74.54 KBSebCorbin
#86 interdiff.txt596 bytesSebCorbin
#85 js-deps-1737148-85.patch77.34 KBSebCorbin
#85 interdiff.txt645 bytesSebCorbin
#83 js-deps-1737148-82.patch77.47 KBSebCorbin
#78 js-deps-1737148-78.patch77.59 KBSebCorbin
#78 interdiff.txt966 bytesSebCorbin
#76 js-deps-1737148-76.patch77.49 KBSebCorbin
#76 interdiff.txt1.95 KBSebCorbin
#63 js-deps-1737148-63.patch75.52 KBlarowlan
#63 js-deps-1737148-63.interdiff.txt2.65 KBlarowlan
#61 js-deps-1737148-61.patch75.92 KBAlbert Volkman
#61 interdiff.txt3.15 KBAlbert Volkman
#58 js-deps-1737148-58.patch75.85 KBAlbert Volkman
#58 interdiff.txt3.36 KBAlbert Volkman
#55 js-deps-1737148.55.patch76.23 KBlarowlan
#55 js-deps-1737148.55.interdiff.txt939 byteslarowlan
#43 core-js-explicit-dependencies-1737148-41.patch74.89 KBnod_
#36 core-js-explicit-dependencies-1737148-36.patch74.81 KBnod_
#32 js-deps-1737148.32.patch75.77 KBlarowlan
#32 js-deps-1737148.interdiff.txt6.4 KBlarowlan
#28 core-js-explicit-dependencies-1737148-28.patch74.35 KBnod_
#27 interdiff.txt19.42 KBnod_
#25 core-js-explicit-dependencies-1737148-25.patch74.08 KBnod_
#23 drupal-1737148-23.patch73.02 KBtim.plunkett
#23 interdiff.txt2.9 KBtim.plunkett
#18 core-js-explicit-dependencies-1737148-18.patch73.03 KBnick_schuch
#16 core-js-explicit-dependencies-1737148-16.patch73.01 KBnod_
#14 core-js-explicit-dependencies-1737148-14.patch58.42 KBnod_
#12 core-js-explicit-dependencies-1737148-12.patch58.3 KBnod_
#12 core-js-explicit-dependencies-1737148-12-no-locale.patch57.63 KBnod_
#9 core-js-explicit-dependencies-1737148-9.patch52.62 KBnod_
#4 core-js-explicit-dependencies-1737148-4.patch52.67 KBnod_
#3 core-js-explicit-dependencies-1737148-2.patch52.68 KBnod_
#2 drupal-1737148-2.patch45.41 KBtim.plunkett
#2 interdiff.txt822 bytestim.plunkett
core-js-explicit-dependencies.patch45.42 KBnod_
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work

The last submitted patch, core-js-explicit-dependencies.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
822 bytes
45.41 KB

Part of locale_library_info_alter() was off.

nod_’s picture

All right, awesome new patch that actually fix #1279226: jQuery and Drupal JavaScript libraries and settings are output even when no JS is added to the page.

There are a few things related to ajaxPageState moved around, this should be using #1573020: Use data-* for CSS and JS files list to begin with so that we don't even need to do that in PHP.

nod_’s picture

Thanks, rerolled with changes from #2

nod_’s picture

Priority: Normal » Major

Bumping since it takes care of a nasty bug.

Wim Leers’s picture

Priority: Major » Normal

I *think* my review over at #1542344-44: Use AMD for JS architecture has already been incorporated?

Status: Needs review » Needs work

The last submitted patch, core-js-explicit-dependencies-1737148-4.patch, failed testing.

nod_’s picture

Priority: Normal » Major
Status: Needs work » Needs review

Putting back to major and NR since testbot is going crazy.

Yes the review has been addressed by SebCorbin before I made the patch, check the AMD sandbox log :p

nod_’s picture

Status: Needs review » Needs work

The last submitted patch, core-js-explicit-dependencies-1737148-9.patch, failed testing.

tim.plunkett’s picture

Not sure why its failing exactly, but a note for the next reroll:

+++ b/core/modules/locale/locale.moduleundefined
@@ -879,3 +878,33 @@ function _locale_rebuild_js($langcode = NULL) {
 }
+
+
+/**
+ * Implements hook_library_info().
+ */
+function locale_library_info() {

This should come before locale_library_info_alter(). Also, adds an extra blank line by mistake.

nod_’s picture

Added more dependencies.

On patch without the local changes to see if that's the only thing crashing the tests.

Status: Needs review » Needs work

The last submitted patch, core-js-explicit-dependencies-1737148-12-no-locale.patch, failed testing.

nod_’s picture

Status: Needs work » Needs review
FileSize
58.42 KB

All right, should just have the tests to fix.

SebCorbin found out that variable_get was called during install, before DB was set, Added a check for the MAINTENANCE_MODE . Apologies for blaming testbot :)

Status: Needs review » Needs work

The last submitted patch, core-js-explicit-dependencies-1737148-14.patch, failed testing.

nod_’s picture

Status: Needs work » Needs review
FileSize
73.01 KB

Less test failures.

Wim Leers’s picture

Status: Needs review » Needs work

Quick comments review. Quite a lot of comments to explain tricky edge cases, great! :)

+++ b/core/includes/common.incundefined
@@ -3946,23 +3900,50 @@ function drupal_get_js($scope = 'header', $javascript = NULL, $skip_alter = FALS
+  // Don't add those settings if there is no other JS on the page.

Remove "those", no?

+++ b/core/includes/common.incundefined
@@ -3946,23 +3900,50 @@ function drupal_get_js($scope = 'header', $javascript = NULL, $skip_alter = FALS
+    // Sort the JavaScript so that it appears in the correct order.

Sort the JavaScript files so that they appear in the correct order.

+++ b/core/includes/common.incundefined
@@ -3946,23 +3900,50 @@ function drupal_get_js($scope = 'header', $javascript = NULL, $skip_alter = FALS
+    // later Ajax request can be rendered using the same theme.

s/Ajax/AJAX/

+++ b/core/includes/common.incundefined
@@ -3946,23 +3900,50 @@ function drupal_get_js($scope = 'header', $javascript = NULL, $skip_alter = FALS
+    // Skip if no files were added to the page or jQuery.extend() will overwrite

s/or/otherwise/

+++ b/core/includes/common.incundefined
@@ -3946,23 +3900,50 @@ function drupal_get_js($scope = 'header', $javascript = NULL, $skip_alter = FALS
+    // that drupal_get_js() is running, so add the setting to this output as well

s/setting/settings/

+++ b/core/modules/color/color.moduleundefined
@@ -246,6 +244,7 @@ function theme_color_scheme_form($variables) {
+  // TODO transform to add library

s/TODO/@todo/

nick_schuch’s picture

Status: Needs work » Needs review
FileSize
73.03 KB

Updated comments as per #17.

Status: Needs review » Needs work

The last submitted patch, core-js-explicit-dependencies-1737148-18.patch, failed testing.

pounard’s picture

You don't include settings if they are no other JS on the page, but what about arbitrary inline JS or potential external JS being included?

nod_’s picture

well, it just works™

Everything is managed by libraries so to get settings, you need to drupal_add_library the drupal library, otherwise, no drupal.js and no Drupal.settings. The rest works. But yeah we still have to keep drupal_add_js around for now. That said you can add this type of JS in a library hook too.

pounard’s picture

All right, thanks.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
2.9 KB
73.02 KB

It seems the last patch was made by editing the previous patch directly, and the grammar fixes were made to both the deletions and insertions. Which doesn't work.

Straight reroll with interdiff against #16.

Status: Needs review » Needs work

The last submitted patch, drupal-1737148-23.patch, failed testing.

nod_’s picture

Status: Needs work » Needs review
FileSize
74.08 KB

Green :D

All right changed a couple of things:

- drupal.settings needs to be added as a dependency explicitly as well, that's the same mess otherwise.
- there is a js_alter hook in system to add the default configuration for the JS setting data, it's messy to try to do that in drupal_add_js

This fixes #1279226: jQuery and Drupal JavaScript libraries and settings are output even when no JS is added to the page the right way.

Status: Needs review » Needs work

The last submitted patch, core-js-explicit-dependencies-1737148-25.patch, failed testing.

nod_’s picture

Status: Needs work » Needs review
FileSize
19.42 KB

Here you go

nod_’s picture

Should get rid of the warnings at least, added an isset() in locale_library_info()

I moved things around too much, the ajax thing is broken.

Status: Needs review » Needs work

The last submitted patch, core-js-explicit-dependencies-1737148-28.patch, failed testing.

Wim Leers’s picture

+++ b/core/includes/common.incundefined
@@ -3946,23 +3888,52 @@ function drupal_get_js($scope = 'header', $javascript = NULL, $skip_alter = FALS
+    // Don't add settings if there is no other JS on the page.

s/JS/JavaScript/

+++ b/core/includes/common.incundefined
@@ -3946,23 +3888,52 @@ function drupal_get_js($scope = 'header', $javascript = NULL, $skip_alter = FALS
+    if (!empty($items['settings'])) {

The comment says "if there is no other JavaScript". But here it's checking for other JavaScript *settings* specifically.
I think the comment is right and the code is wrong. The original code also just checked for $items not being empty, not $items['settings'].

However, if I look at drupal.ajax, which is where ajaxPageState would be used (primarily), then I see that it explicitly lists a dependency on drupal.settings. That means this check should also work.

I'd love a clarification here :)

+++ b/core/includes/common.incundefined
@@ -3946,23 +3888,52 @@ function drupal_get_js($scope = 'header', $javascript = NULL, $skip_alter = FALS
+      // files removed below are still considered "used" and prevented from being

"removed below"? Not within this function; so it's better to refer to the specific function in which this is happening.

+++ b/core/includes/common.incundefined
@@ -3946,23 +3888,52 @@ function drupal_get_js($scope = 'header', $javascript = NULL, $skip_alter = FALS
+      // Skip if no files were added to the page otherwise jQuery.extend() will overwrite

Wrap at col 80.

+++ b/core/modules/system/lib/Drupal/system/Tests/Common/JavaScriptTest.phpundefined
@@ -68,13 +68,7 @@ class JavaScriptTest extends WebTestBase {
-    $this->assertTrue(array_key_exists('core/misc/jquery.js', $javascript), t('jQuery is added when a file is added.'));
-    $this->assertTrue(array_key_exists('core/misc/drupal.js', $javascript), t('Drupal.js is added when file is added.'));

Maybe we should assertFalse ()these?

+++ b/core/modules/system/lib/Drupal/system/Tests/Common/JavaScriptTest.phpundefined
@@ -139,6 +136,7 @@ class JavaScriptTest extends WebTestBase {
+    drupal_add_library('system', 'drupal.settings');
     // Only the second of these two entries should appear in Drupal.settings.
     drupal_add_js(array('commonTest' => 'commonTestShouldNotAppear'), 'setting');
     drupal_add_js(array('commonTest' => 'commonTestShouldAppear'), 'setting');

So drupal.settings gets added because we're adding settings that would otherwise not appear.

Can't we automatically add drupal_add_library() whenever drupal_add_js(array(), 'setting') is called?

Or do we intentionally not want to do this to enforce "good behavior"?

+++ b/core/modules/system/lib/Drupal/system/Tests/Common/JavaScriptTest.phpundefined
@@ -496,14 +507,14 @@ class JavaScriptTest extends WebTestBase {
+    // Note: This test installs tracker module.

s/tracker/language/? :)

+++ b/core/modules/system/system.moduleundefined
@@ -1212,6 +1212,51 @@ function _system_batch_theme() {
+  // Drupal specific javascript.

Drupal-specific JavaScript.

+++ b/core/modules/system/system.moduleundefined
@@ -1212,6 +1212,51 @@ function _system_batch_theme() {
+  $libraries['drupal.settings'] = array(
+    'title' => 'Drupal Settings',
+    'version' => VERSION,
+    'js' => array(
+      array(
+        'data' => array(
+          'basePath' => base_path(),
+          'scriptPath' => $scriptPath,
+          'pathPrefix' => $pathPrefix,
+          'currentPath' => current_path(),
+        ),
+        'type' => 'setting',
+        'scope' => 'header',
+        'group' => JS_SETTING,
+        'every_page' => TRUE,
+        'weight' => 0,
+        'browsers' => array(),
+      ),
+    ),
+    'dependencies' => array(
+      array('system', 'jquery'),
+      array('system', 'drupal'),
+    ),
+  );

I LOVE THIS!

+++ b/core/modules/system/system.moduleundefined
@@ -1831,12 +1924,106 @@ function system_library_info() {
+  // Add configuration only if there are settings added to the page.

I think you should extend this comment and state that this is ONLY necessary for JS that is added through drupal_add_js(), i.e. for JS that is not added as a lib, because JS libs should specify a dependency on drupal.settings, in which case this hook implementation is not necessary at all.

+++ b/core/modules/taxonomy/taxonomy.moduleundefined
@@ -1703,3 +1703,27 @@ function taxonomy_entity_query_alter($query) {
+   * Implements hook_library_info().

Unnecessary/unwanted leading spaces.

larowlan’s picture

Assigned: Unassigned » larowlan

tackling Wim's comments and last two failing tests.

larowlan’s picture

Attached patch fixes three of the failing tests.
Adds new util function drupal_get_content_request_type which is wrapper to DIC to get the current request type.
Previous patch didn't load/update $settings['ajaxPageState'] because no other js settings existed for the ajax framework tests, during lazy load.
New patch adds a check to see if it's an ajax request, if so $settings['ajaxPageState'] is always added in drupal_get_js (i.e. of course we need to load javascript in an ajax response).
Interdiff attached too.

larowlan’s picture

Status: Needs work » Needs review

Letting the test-bot have a look.
Note there is still an issue with the lazy loading of scripts.
You can see response.data =

"<script type="text/javascript" src="http://d8.taco/core/modules/system/system.js?m92uip"></script>↵"

In the ajax insert command, however it is adding an empty <div> to the page instead of the script in it's own right.
The problem lines are in ajax.js around here:

    // We don't know what response.data contains: it might be a string of text
    // without HTML, so don't rely on jQuery correctly iterpreting
    // $(response.data) as new HTML rather than a CSS selector. Also, if
    // response.data contains top-level text nodes, they get lost with either
    // $(response.data) or $('<div></div>').replaceWith(response.data).
    var new_content_wrapped = $('<div></div>').html(response.data);
    var new_content = new_content_wrapped.contents();

After this runs, new_content_wrapped only has an empty text child element: i.e. new_content_wrapped.html() = '<div></div>'

larowlan’s picture

Assigned: larowlan » Unassigned

Status: Needs review » Needs work

The last submitted patch, js-deps-1737148.32.patch, failed testing.

nod_’s picture

Status: Needs review » Needs work
Issue tags: -mobile, -JavaScript clean-up

The last submitted patch, core-js-explicit-dependencies-1737148-36.patch, failed testing.

nod_’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +mobile, +JavaScript clean-up

The last submitted patch, core-js-explicit-dependencies-1737148-36.patch, failed testing.

larowlan’s picture

re #33 is a known issue, true but without this patch the lazy loading of script tests work so I think we're missing something

nod_’s picture

Status: Needs work » Needs review
FileSize
74.89 KB

Yeah got to find out what it's about, it seems to work fine for me. The script is actually added to the page.

In the meantime, reroll.

Status: Needs review » Needs work

The last submitted patch, core-js-explicit-dependencies-1737148-41.patch, failed testing.

Wim Leers’s picture

Everything I said in my review #30 was addressed, except for the items below.

+++ b/core/modules/system/lib/Drupal/system/Tests/Common/JavaScriptTest.phpundefined
@@ -68,13 +68,7 @@ class JavaScriptTest extends WebTestBase {
-    $this->assertTrue(array_key_exists('core/misc/jquery.js', $javascript), t('jQuery is added when a file is added.'));
-    $this->assertTrue(array_key_exists('core/misc/drupal.js', $javascript), t('Drupal.js is added when file is added.'));

Maybe we should assertFalse() these?

+++ b/core/modules/system/lib/Drupal/system/Tests/Common/JavaScriptTest.phpundefined
@@ -139,6 +136,7 @@ class JavaScriptTest extends WebTestBase {
+    drupal_add_library('system', 'drupal.settings');
     // Only the second of these two entries should appear in Drupal.settings.
     drupal_add_js(array('commonTest' => 'commonTestShouldNotAppear'), 'setting');
     drupal_add_js(array('commonTest' => 'commonTestShouldAppear'), 'setting');

So drupal.settings gets added because we're adding settings that would otherwise not appear.

Can't we automatically add drupal_add_library() whenever drupal_add_js(array(), 'setting') is called?

Or do we intentionally not want to do this to enforce "good behavior"?

nod_’s picture

I expclicitly don't want that. It's was part of the reason why the drupal_add_js code was weird. If you want settings, declare it.

Wim Leers’s picture

#46: Okay :) I personally agree, I'm just trying to approach it from all angles. But, in that case, a small explanatory comment would be nice, because you won't see this happening in many locations (since everything/as much as possible should move towards hook_library()).

Regarding the test failures:

  • It seems this is caused by the fact that the necessary tables don't exist at all. Locally, I can reproduce one of the two failures on qa.drupal.org, but I also have two failures from SimpleTest itself: Failed to find test tables to drop.
  • When I revert this patch, I still get the two failures within SimpleTest itself

Hence it seems the problem lies in Locale module/SimpleTest module. The discrepancy between what I see on a fresh local install vs. what qa.drupal.org says is actually worrisome, I think. I've requeued the testing to hopefully get the same result.

Wim Leers’s picture

Status: Needs work » Needs review
Issue tags: -mobile, -JavaScript clean-up

Status: Needs review » Needs work

The last submitted patch, core-js-explicit-dependencies-1737148-41.patch, failed testing.

nod_’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +mobile, +JavaScript clean-up

The last submitted patch, core-js-explicit-dependencies-1737148-41.patch, failed testing.

Wim Leers’s picture

nod_: do you have any more ideas? Does my research from #47 give you any ideas what might be causing this?

larowlan’s picture

I too grappled with the locale fails.
If you do exactly what the test is doing in the UI, it passes - you get the exported file.
But the test is getting the 'nothing to export' message from line 248 of locale.bulk.inc.
Perhaps it's a profile thing, if I get time today I'll try and install the testing profile and test in the UI.

nod_’s picture

Tried with the testing profile installed, get "Nothing to export" doing the thing from the UI.

larowlan’s picture

Status: Needs work » Needs review
FileSize
939 bytes
76.23 KB

Too damn efficient - the earlier patches resulted in no calls to drupal_add_js so _locale_parse_js_file() never ran, so locales_source was empty.
New patch fetches admin/config/regional/language (which includes tabledrag js so db is at least primed) before exporting PO file.

nod_’s picture

haha awesome. Now we just need to clean it up or are you still seeing the issue with the JS files added during ajax calls?

Wim Leers’s picture

Status: Needs review » Needs work

@larowlan: GREAT job! :)

Final review before marking as RTBC. Looks very good :)

+++ b/core/includes/common.incundefined
@@ -533,6 +533,35 @@ function drupal_get_destination() {
+function drupal_get_request_content_type() {

@@ -3946,23 +3917,54 @@ function drupal_get_js($scope = 'header', $javascript = NULL, $skip_alter = FALS
+    // this is an ajax request.

s/ajax/AJAX/

+++ b/core/includes/common.incundefined
@@ -3946,23 +3917,54 @@ function drupal_get_js($scope = 'header', $javascript = NULL, $skip_alter = FALS
+      // Provide the page with information about the theme that's used, so that a

Just beyond col 80?

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleExportTest.phpundefined
@@ -114,6 +114,10 @@ class LocaleExportTest extends WebTestBase {
+    // Load an admin page with JavaScript so drupal_add_library fires at least

s/drupal_add_library/drupal_add_library()/

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleExportTest.phpundefined
@@ -114,6 +114,10 @@ class LocaleExportTest extends WebTestBase {
+    // once and _locale_parse_js_file() gets to run at least once so that
+    // locales_source table gets populated with something.

s/so that locales_source/so that the locales_source/

+++ b/core/modules/simpletest/simpletest.moduleundefined
@@ -566,3 +566,25 @@ function simpletest_mail_alter(&$message) {
+function simpletest_library_info() {

Missing the "Implements hook_library_info()." comment.

+++ b/core/modules/system/system.moduleundefined
@@ -1216,6 +1216,51 @@ function _system_batch_theme() {
+  // Drupal specific JavaScript.

s/Drupal specific/Drupal-specific/

+++ b/core/profiles/testing/testing.infoundefined
@@ -2,6 +2,5 @@ name = Testing
-hidden = TRUE

I think this is an accidental change? We *don't* want to change this!

Albert Volkman’s picture

Status: Needs work » Needs review
FileSize
3.36 KB
75.85 KB

Re-roll with @Wim Leers requested fixes.

larowlan’s picture

Thanks Wim and Albert, good catch on the hidden flag on the testing profile.

tim.plunkett’s picture

Status: Needs review » Needs work

This looks almost RTBC! Just some clean-up points.

+++ b/core/includes/common.incundefined
@@ -533,6 +533,35 @@ function drupal_get_destination() {
+ * @return string
+ *   The request type

This should be @return string|false, and the description should clarify what FALSE signifies. It should also end with a trailing full stop

+++ b/core/includes/common.incundefined
@@ -533,6 +533,35 @@ function drupal_get_destination() {
+    if ($container->has('request') &&
+        $container->has('content_negotiation')) {

Combine this into one line

+++ b/core/modules/locale/locale.moduleundefined
@@ -374,7 +374,7 @@ function locale_js_alter(&$javascript) {
       if (!in_array($filepath, $parsed)) {
@@ -431,8 +431,7 @@ function locale_library_info_alter(&$libraries, $module) {

@@ -431,8 +431,7 @@ function locale_library_info_alter(&$libraries, $module) {
     // locale.datepicker.js should be added in the JS_LIBRARY group, so that
     // this attach behavior will execute early. JS_LIBRARY is the default for
     // hook_library_info_alter(), thus does not have to be specified explicitly.
-    $datepicker = drupal_get_path('module', 'locale') . '/locale.datepicker.js';
-    $libraries['jquery.ui.datepicker']['js'][$datepicker] = array();
+    $libraries['jquery.ui.datepicker']['dependencies'][] = array('locale', 'drupal.locale.datepicker');
     $libraries['jquery.ui.datepicker']['js'][] = array(
       'data' => array(
         'jquery' => array(
@@ -882,6 +881,38 @@ function _locale_rebuild_js($langcode = NULL) {

@@ -882,6 +881,38 @@ function _locale_rebuild_js($langcode = NULL) {
 }
 
 /**
+ * Implements hook_library_info().
+ */
+function locale_library_info() {
+  $libraries['drupal.locale.admin'] = array(
+    'title' => 'Locale',
+    'version' => VERSION,
+    'js' => array(
+      drupal_get_path('module', 'locale') . '/locale.admin.js' => array(),

This would make sense to have locale_library_info() directly before locale_library_info_alter() in the code.

Albert Volkman’s picture

Status: Needs work » Needs review
FileSize
3.15 KB
75.92 KB

Thanks @tim.plunkett

Wim Leers’s picture

Status: Needs review » Needs work

Final review. Three style nitpicks. One actual — yet minor — regression.

+++ b/core/modules/locale/locale.pages.incundefined
@@ -255,8 +255,8 @@ function locale_translate_edit_form($form, &$form_state) {
-  $form['#attached']['js'] = array(
-    $path . '/locale.admin.js',
+  $form['#attached']['library'] = array(
+    array('locale', 'drupal.locale.admin')
   );

We should change this from ['library'] = array(array('foo')) to ['library'][] = array('foo').

+++ b/core/modules/statistics/statistics.moduleundefined
@@ -103,10 +103,8 @@ function statistics_permission() {
 function statistics_node_view(Node $node, $view_mode) {
   if (!empty($node->nid) && $view_mode == 'full') {
-    $node->content['#attached']['js'] = array(
-      drupal_get_path('module', 'statistics') . '/statistics.js' => array(
-        'scope' => 'footer'
-      ),
+    $node->content['#attached']['library'] = array(
+      array('statistics', 'drupal.statistics'),
     );
     $settings = array('nid' => $node->nid);
     $node->content['#attached']['js'][] = array(
@@ -462,3 +460,23 @@ function statistics_preprocess_block(&$variables) {

@@ -462,3 +460,23 @@ function statistics_preprocess_block(&$variables) {
     $variables['attributes']['role'] = 'navigation';
   }
 }
+
+/**
+ * Implements hook_library_info().
+ */
+function statistics_library_info() {
+  $libraries['drupal.statistics'] = array(
+    'title' => 'Statistics',
+    'version' => VERSION,
+    'js' => array(
+      drupal_get_path('module', 'statistics') . '/statistics.js' => array(),
+    ),
+    'dependencies' => array(
+      array('system', 'jquery'),
+      array('system', 'drupal'),
+      array('system', 'drupal.settings'),
+    ),
+  );
+

'scope' => 'footer' was lost here.

We should change this from ['library'] = array(array('foo')) to ['library'][] = array('foo').

+++ b/core/modules/translation/translation.moduleundefined
@@ -116,8 +116,8 @@ function translation_permission() {
-  $form['#attached']['js'] = array(
-    drupal_get_path('module', 'translation') . '/translation.js',
+  $form['#attached']['library'] = array(
+    array('translation', 'drupal.translation'),

We should change this from ['library'] = array(array('foo')) to ['library'][] = array('foo').

larowlan’s picture

Status: Needs work » Needs review
FileSize
2.65 KB
75.52 KB

Fixing issues flagged at #62

nod_’s picture

That is very awesome :)

I'm very tempted to RTBC it :p It'd be great to have that committed this week, this is the most important frontend performance-related patch ever.

Please give credit to SebCorbin first for this patch he did the hard work of making all the library_info declarations.

pounard’s picture

SebCorbin++ nod_++ RTBC?

nod_’s picture

And the other people involved++ :)

Just need confirmation the new drupal_get_request_content_type function is done correctly, I don't know is that's using the container properly or not. Looks like it but I can't say.

pounard’s picture

I don't like this function, I'm asking myself why is that in the global scope, and will it help other pieces of API than the JS/CSS helpers? It seems to be a wrapper on top of the Request to fetch something that the Request object should probably capable of giving it us in a more direct way, without the need of a wrapper.

nod_’s picture

can you try it out please?

/me don't even know where to start looking.

pounard’s picture

Right now, I can't, I'm @work and I don't have a complete environement for this, but I manage to find some time in the weekend I'd try. EDIT: The first place to start is maybe open a support request with the WSCCI tag explaining that you wrote this wrapper function and asking if it exists a better way to fetch this value. I'm thinking that the WSCCI team will probably answer you quickly.

larowlan’s picture

pounard, are you saying this should be a public method on the request object?
If so I can tackle but won't be for a day or so.

larowlan’s picture

Fwiw I started out without the wrapper but the amount of logic required in the if statement was unwieldly so figured a global wrapper might be useful.

pounard’s picture

I'm not saying that it must be a method of the Request object, but that there may be a more direct way to obtain it, that's why I propose a support request to the WSCCI team and not a feature request.

Wim Leers’s picture

I'll try to talk to @Crell about this.

Wim Leers’s picture

Status: Needs review » Needs work

I talked to @Crell. First of all he said that we should *never* use static caching for Symfony stuff.

He told us we should do the following:

$container = drupal_container();
$type = $container->get('content_negotiation')->getContentType($container->get('request'));

But soon, we'll be able to do the following:

$type = drupal_container()->get('content_negotiation')->getContentType();

So we should add a @todo to make sure we do that clean-up later (but @Crell first has to make that possible first).

nod_’s picture

All right should be an easy one :) someone reroll?, that'll take it very very close to rtbc :)

SebCorbin’s picture

Assigned: Unassigned » SebCorbin
Status: Needs work » Needs review
FileSize
1.95 KB
77.49 KB

Status: Needs review » Needs work

The last submitted patch, js-deps-1737148-76.patch, failed testing.

SebCorbin’s picture

Status: Needs work » Needs review
FileSize
966 bytes
77.59 KB

Damn, I forgot about the install state...

nod_’s picture

wow, someone rtbc that stuff, it's the most important patch for JS. Ever. (funnily enough, there is no js changes :p)

pounard’s picture

Status: Needs review » Reviewed & tested by the community

RTBC.

Wim Leers’s picture

RTBC HELL YEAH!

tim.plunkett’s picture

I've also reviewed this many times and think its the right approach. RTBC++

SebCorbin’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
77.47 KB

Re-rolled as http://drupalcode.org/project/drupal.git/commitdiff/34e4ef48118d1a4d9155... modified toolbar.module

Also removed unused $module_path variable in toolbar.module

tim.plunkett’s picture

Can you post an interdiff?

SebCorbin’s picture

FileSize
645 bytes
77.34 KB

@tim.plunkett here's a re-roll patch with a patch interdiff against #78 (no changes except for the settings that conflicted)

SebCorbin’s picture

FileSize
596 bytes
74.54 KB

And here's a new one, with unused $module_path removed in toolbar.module, and already stated dependency to jquery.cookie.
interdiff is against #85

nod_’s picture

Reroll ok for me.

nod_’s picture

Status: Needs review » Reviewed & tested by the community
sun’s picture

Status: Reviewed & tested by the community » Needs work

Awesome patch! I really like it.

I reviewed this patch in-depth and found a couple of issues with it. I did not read comments on this issue, in order to try to stay as ignorant for reviewing the final implementation.

+++ b/core/modules/block/block.admin.inc
@@ -82,8 +82,8 @@ function block_admin_display_prepare_blocks($theme) {
+  $form['#attached']['library'][] = array('system', 'drupal.tableheader');
+  $form['#attached']['library'][] = array('block', 'drupal.block');

Let's remove the "drupal." prefix from registered libraries. This was introduced, because System module was more or less the only module in core that ships with any libraries and we wanted to separate Drupal stuff in the /misc folder from jQuery and other stuff.

However, in this larger scope the "drupal." prefix doesn't really make sense. I think that we should only use the "drupal." prefix for stuff in the /misc directory. For everything else it is unnecessary.

+++ b/core/modules/block/block.module
@@ -1085,3 +1085,22 @@ 
+function block_library_info() {

+++ b/core/modules/book/book.module
@@ -1454,3 +1454,23 @@ 
+function book_library_info() {

The new hook_library_info() implementations have been added to the bottom of .module files, whereas it is (not a standard but) common practice to put the "central" info hook implementations to the top.

I do not want to hold up this patch on that, but it would be great if we could quickly adjust that before commit - otherwise, let's do a follow-up patch/issue.

+++ b/core/modules/block/block.module
@@ -1085,3 +1085,22 @@ function block_language_delete($language) {
+    'dependencies' => array(
+      array('system', 'jquery'),
+      array('system', 'drupal'),
+    ),

+++ b/core/modules/book/book.module
@@ -1454,3 +1454,23 @@ function book_menu_subtree_data($link) {
+    'dependencies' => array(
+      array('system', 'jquery'),
+      array('system', 'drupal'),
+      array('system', 'drupal.form'),
+    ),

drupal_add_library() resolves nested dependencies already, so I don't quite understand why we're specifying the full stack of dependencies instead of just the "highest" dependency?

Removing the deeper dependencies doesn't affect how this patch works, so I'm happy if we clean this up in a follow-up issue.

+++ b/core/modules/locale/locale.module
@@ -374,7 +374,7 @@ function locale_js_alter(&$javascript) {
   foreach ($javascript as $item) {
-    if ($item['type'] == 'file') {
+    if (isset($item['type']) && $item['type'] == 'file') {

'type' should always be set for every JS item. I do not see where and why this would be changed in this patch. Is this additional check for 'type' being set an artifact of earlier debugging or approaches?

+++ b/core/modules/system/system.module
@@ -1216,6 +1216,51 @@ function _system_batch_theme() {
+  // url() generates the script and prefix using hook_url_outbound_alter().
+  // Instead of running the hook_url_outbound_alter() again here, extract
+  // them from url().
+  // @todo Make this less hacky: http://drupal.org/node/1547376.
+  $scriptPath = $GLOBALS['script_path'];
+  $pathPrefix = '';
+  url('', array('script' => &$scriptPath, 'prefix' => &$pathPrefix));
+  // Drupal settings.
+  $libraries['drupal.settings'] = array(
+    'title' => 'Drupal Settings',
+    'version' => VERSION,
+    'js' => array(
+      array(
+        'data' => array(
+          'basePath' => base_path(),
+          'scriptPath' => $scriptPath,
+          'pathPrefix' => $pathPrefix,
+          'currentPath' => current_path(),
+        ),
+        'type' => 'setting',
+        'scope' => 'header',
+        'group' => JS_SETTING,
+        'every_page' => TRUE,
+        'weight' => 0,
+        'browsers' => array(),
+      ),
+    ),
+    'dependencies' => array(
+      array('system', 'jquery'),
+      array('system', 'drupal'),
+    ),
+  );

This cannot be contained in hook_libraries_info(), since it is incompatible with caching of the registered libraries. hook_libraries_info() is a static declaration/registration of available libraries provided/shipped by modules and not supposed to change on every request.

It looks like this change actually belongs into #1279226: jQuery and Drupal JavaScript libraries and settings are output even when no JS is added to the page instead.

+++ b/core/modules/system/system.module
@@ -1357,13 +1444,16 @@ function system_library_info() {
-  $libraries['farbtastic'] = array(
+  $libraries['jquery.farbtastic'] = array(
     'title' => 'Farbtastic',
     'website' => 'http://code.google.com/p/farbtastic/',

The farbtastic library name change is irrelevant for this issue and should be reverted. I don't think our code is actually the code of that Google code repo (development on that seems to have stopped in 2009/2010), although I could be mistaken. In any case, this rename seems out of scope for this issue.

+++ b/core/modules/system/system.module
@@ -1835,12 +1928,109 @@ function system_library_info() {
+function system_js_alter(&$javascript) {
+  // Add configuration only if there are settings added to the page. This is
+  // only necessary for JavaScript that is added through drupal_add_js(),
+  // JavaScript that is added as a library would nominate its dependency on
+  // drupal.settings.
+  if (!empty($javascript['settings'])) {
+    $javascript['settings'] += array(
+      'type' => 'setting',
+      'scope' => 'header',
+      'group' => JS_SETTING,
+      'every_page' => TRUE,
+      'weight' => 0,
+      'browsers' => array(),
+    );
+  }
+}

Lazy-injecting the base information for settings in a hook_js_alter() implementation doesn't fly for me. I guess this change is the cause for the isset($item['type']) check in locale_js_alter(), which is ultimately going to be only one of many consequences. This handling of defaults has to be carried out by drupal_get_js().

This change also seems to belong into #1279226: jQuery and Drupal JavaScript libraries and settings are output even when no JS is added to the page instead.

nod_’s picture

Status: Needs work » Needs review

Well some reading could have helped.

1) The drupal. prefix is here for a reason, it's because all this stuff lives under the Drupal object, Drupal.tableDrag, Drupal.ajax and so on. Notice how the html5 shiv doesn't have it. JS will be out of misc and in the system module very soon after this patch we can't rely on that.

jquery.farbtastic, under the $.fn.farbtastic, same thing.

2) bottom of files, fair enough, I'd rather follow up issues, it's a lot of files.

3) No, explicit dependencies, the actual script is using and the jQuery object and the Drupal object so it should depend on both even if drupal.settings depends on both as well, otherwise it'll be impossible to build AMD/commonjs ever and will make things much harder to take apart in contrib. There is nothing to clean-up here it's the whole point of the patch.

4) 'type', indeed it comes from the _js_alter() but this is needed since drupal_add_js does way too much, the other issue about js on every pages comes from drupal_add_js doing way too much and putting defaults everywhere. So well, if you have a better idea you're welcome.

5) About caching, maybe. Still works for me™ I can put that with the rest of the ajaxPageState in drupal_get_js if that suits you better. even if it's an empty array() this library definition needs to stay.

6) the js_alter hook, well, couldn't make it work another way If you have a better solution feel free to update the patch.

This patch is aimed at simplifying how we process js, the _js_alter, the wierd things in settings will be gone once we make that work #1751602: Use Assetic to package JS and CSS files but this patch needs to get in before we can start on that. So yeah, not great but works really well.

nod_’s picture

Changed the settings thing, and removed the isset from local.module, let's see what the testbot says.

Status: Needs review » Needs work

The last submitted patch, core-js-explicit-dependencies-1737148-91.patch, failed testing.

nod_’s picture

Status: Needs work » Needs review
FileSize
74.54 KB

Tried a few other things: can't make it work. I don't know how to do it another way, feel free to fix the patch if it doesn't suit you, there are several others for which it's good enough for now.

re-up #86.

nod_’s picture

Assigned: Unassigned » SebCorbin

This is holding up a lot of follow-up work, that will eventually lead to fixing the feedback from #89.

nod_’s picture

Assigned: SebCorbin » Unassigned
Status: Needs review » Reviewed & tested by the community

grr double post.

catch’s picture

Assigned: SebCorbin » catch

This issue is great. I'd really, really like to review it before it goes in though (but can't do so today), so assigning to me. This shouldn't stop webchick or Dries reviewing it too.

sun’s picture

Status: Reviewed & tested by the community » Needs review

I understand that you want to move forward. However, the parts about settings that have been outlined in #89 really do not look acceptable to me. We need to find a better solution for that. I don't think that it is proper to treat Drupal.settings as a library -- the amount of additionally required adjustments for them in this patch pretty much prove that settings do not work like a library. It's not really clear to me why those changes are part of this patch in the first place.

Can we just do the files to libraries conversion here?

On the other issues:

The drupal. prefix is here for a reason, it's because all this stuff lives under the Drupal object, Drupal.tableDrag, Drupal.ajax and so on.

That's a very custom interpretation of the server-side library names. I don't see a problem with it, but we need to make sure that this gets documented.

That said, some scripts/libraries may extend a parent object in multiple ways, so I'm relatively sure that it won't always be possible to determine a unique name à la drupal.foo or jquery.ui.bar. I wonder whether the concept will lead to inconsistencies.

No, explicit dependencies, the actual script is using and the jQuery object and the Drupal object so it should depend on both even if drupal.settings depends on both as well, otherwise it'll be impossible to build AMD/commonjs ever and will make things much harder to take apart in contrib.

I do not think that this is the case. drupal_add_library() already resolves all dependencies of dependencies, which only takes a couple lines of code. The very same can be done by a dependency resolving function for AMD/commonjs. If we wanted to, we could resolve and prepare the dependencies of all libraries within drupal_get_library() already.

However, having to manually specify all nested dependencies sounds very cumbersome and prone to errors to me. That's a typical job for computers, not for humans.

nod_’s picture

Because not all scripts needs Drupal.settings, we have to add it as a library, for now.

If the processing wasn't trying to be too smart and add default settings all over the place when it's not needed (meaning that jquery and drupal get added to all pages all the time, yes that's the reason behind #1279226: jQuery and Drupal JavaScript libraries and settings are output even when no JS is added to the page) we wouldn't need it. Then to fix that properly we'll need to change the internal API of this thing, which is going to be a huge mess to clean up, making this big patch a huge patch. Which is why the API change of js processing will be handled in #1751602: Use Assetic to package JS and CSS files issue to clean all that up and hopefully make the settings dependency obsolete.

Show me that the current patch breaks stuff and I'll change the way it's done somehow (I tried, I really tried to make it pretty, I just can't). I don't want to have an API change in this patch that will push it away that much further.

I do not think that this is the case. drupal_add_library() already resolves all dependencies of dependencies,

That's not the point. Try out AMD, you'll see the problem with what you're saying right away, read the AMD issue, look at the AMD sandbox. If you need to access "it" from your JS code, you need to declare "it" as a dependency, regardless of the rest of if it's a dependency of something else you're using.

By all means, show me a reproducible way of how this badly breaks or improve the patch to make it better while keeping the same API. Until then I really can't do much more than push for this exact patch to get in so that we can get on with the rest of the clean-up.

Wim Leers’s picture

  1. Regarding Drupal.settings as a library. I think it is valid to regard Drupal.settings as a library, for two reasons:
    1. It is impossible to magically know which JS files need Drupal.settings. So we need some way to figure out which JS depends on it. Why not use the existing dependency system for that?
    2. It is *more* than just settings: it depends both on Drupal.js and jQuery having loaded before it.
  2. Regarding specifying "all" dependencies. As nod_ outlined, you should not see it as specifying "all things to load", but rather "all things you need access to". Instead of looking at it from a PHP POV, or a "general programmer POV", you have to look at it from a JS POV. However, this should absolutely be documented very clearly and explicitly.
catch’s picture

I agree that settings feels like a genuine library, it depends on libraries, libraries depend on it.

+++ b/core/includes/common.incundefined
@@ -3952,23 +3894,59 @@ function drupal_get_js($scope = 'header', $javascript = NULL, $skip_alter = FALS
+    // Don't add settings if there is no other JavaScript on the page, unless
+    // this is an AJAX request.
+    // @todo Clean up container call.
+    $container = drupal_container();
+    if ($container->has('request') && $container->has('content_negotiation')) {
+      $type = $container->get('content_negotiation')->getContentType($container->get('request'));

Ouch, but it has a @todo.

+++ b/core/modules/system/system.moduleundefined
@@ -1835,12 +1928,109 @@ function system_library_info() {
 /**
+ * Implements hook_js_alter().
+ */
+function system_js_alter(&$javascript) {
+  // Add configuration only if there are settings added to the page. This is
+  // only necessary for JavaScript that is added through drupal_add_js(),
+  // JavaScript that is added as a library would nominate its dependency on
+  // drupal.settings.
+  if (!empty($javascript['settings'])) {
+    $javascript['settings'] += array(
+      'type' => 'setting',
+      'scope' => 'header',
+      'group' => JS_SETTING,
+      'every_page' => TRUE,
+      'weight' => 0,
+      'browsers' => array(),
+    );
+  }

Do we want to deprecate both this, and direct usage of drupal_add_js(), and then remove this again? I'm wondering if there should be a follow-up patch to make it _drupal_add_js() to indicate it's an internal function. Also wondering whether anything in core is actually using #attached['js'] now but I assume that needs to stay for external js or similar.

moshe weitzman’s picture

Yes, I would love to rename to _drupal_add_js and make it internal only ... We do use #attached['js'] and AFAIK, it remains the preferred way to add js to the page or else you break render cache. Eventually, we should deprecate drupal_add_library() and drupal_add_css() as well (unless SCOTCH changes everything).

nod_’s picture

So we asked crell about this piece of code, he told us to put the todo since we can't currently do that but it'll be possible to make it shorter soon, once some patches gets in, so… blame Crell :)

Yep, so I don't want to put API changes in this patch, so that's the only way I was able to do that. That's right, core don't use #attach js anywhere.

Yeah there are a few things API-related that will need to be taken care of to clean up the alter thing. Making a new internal function to separate and simplify what drupal_add_js is currently doing, we can actually keep that around to add implicity dependencies on jquery, drupal, and drupal.setting for every script added by drupal_add_js just so you know, we don't horribly break contrib.

I guess we can make a intermediate followup between this issue and the Assetic issue.

Wim Leers’s picture

The @todo can only be solved when WSCCI is further along.

Ideally: #attached should be the only way to add/remove CSS/JS/libs. drupal_add_js(), if we keep it, should indeed become a private/internal function. But those are outside of the scope of this patch (as nod_ says: no API changes in this patch).

Besides that, I think the only unaddressed question here is whether it's acceptable to have Drupal.settings be specified as a library. Conceptually, it makes sense, as @catch and I have outlined above. However, @sun makes a good point: part of the concept of libs is also that they're not tied to the current page request. However, I feel that is a minor issue in comparison with the major improvements this patch brings. I think it would be acceptable to create a follow-up issue for that aspect?

So, what is still blocking this patch from going to RTBC status?

cosmicdreams’s picture

Status: Needs review » Reviewed & tested by the community

What would that follow up be? to create a new construct for page specific libraries?

nod_’s picture

Follow-up: #1762204: Introduce Assetic compatibility layer for core's internal handling of assets. The summary still needs some work but that's mostly it.

sun’s picture

My two major concerns are:

1) The actual settings are not declarative, they are dynamic, by design. Info hooks are not supposed to contain highly dynamic data. In fact, this patch revealed that we're needlessly re-computing all library info on every single page request and missing a cache for hook_library_info() currently. In addition to that, system_js_alter() is an alter hook that injects default properties, which is architecturally wrong.

2) It is not clear how modules are supposed to add settings. It looks like they need to add the drupal.settings library first and then add the settings. This feels odd. Especially if you consider that (further) settings might be added to a page in a different (code) spot than the library.

I'd really prefer to take that settings part out of this patch and discuss it in a separate issue. It's really just that part that bugs me — all of the other js-to-library changes are looking great.

nod_’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
3.55 KB
74.26 KB

That's fair enough. the library def stays because we need the dependencies but it doesn't add anything else.

Let's see what the testbot says.

sun’s picture

Thanks! RTBC if bot comes back green.

Status: Needs review » Needs work

The last submitted patch, core-js-explicit-dependencies-1737148-107.patch, failed testing.

nod_’s picture

Haha, that really scared me, it's failing because it can't write to disk somehow. I'm asking around to see what's up before retesting.

nod_’s picture

Status: Needs work » Needs review
FileSize
74.25 KB

same as #107, with correcter spelling in comment.

Status: Needs review » Needs work

The last submitted patch, core-js-explicit-dependencies-1737148-111.patch, failed testing.

nod_’s picture

Status: Needs work » Needs review
FileSize
74.52 KB

All right had a few issues with the tests, all green (local at least). Hope you have some space now testbot, here is the patch.

Jelle_S’s picture

Status: Needs review » Needs work

Just thinking ahead for documentation etc. Is there still a valid use case for drupal_add_js once this patch gets in? Since all contrib modules are encouraged to use Drupal's behaviors (and thus have a dependency to drupal.js), shouldn't they define their script in hook_library_info as well?

Jelle_S’s picture

Status: Needs work » Needs review

cross-post...

Status: Needs review » Needs work
Issue tags: -mobile, -JavaScript clean-up

The last submitted patch, core-js-explicit-dependencies-1737148-113.patch, failed testing.

nod_’s picture

Status: Needs work » Needs review
Issue tags: +mobile, +JavaScript clean-up
Jelle_S’s picture

On a sidenote:
Shouldn't we change drupal_add_js to do

drupal_add_library('system', 'drupal.settings');

when $options['type'] is setting so that we can just do this:

drupal_add_js(array('dummy' => 'value'), 'setting');

in stead of:

drupal_add_library('system', 'drupal.settings');
drupal_add_js(array('dummy' => 'value'), 'setting');

Just a small change dat improves DX (imho)

Status: Needs review » Needs work

The last submitted patch, core-js-explicit-dependencies-1737148-113.patch, failed testing.

Jelle_S’s picture

1 fail on file unmanagedDelete test... Bot still acting out?

One more retest...

Jelle_S’s picture

Status: Needs work » Needs review
nod_’s picture

(while waiting for the bot) #118 that'll be doing what drupal_add_js has been doing, implicitly adding things so I'm not really in favor of this.

Jelle_S’s picture

True, but it seems to me that when you're adding js settings, you'll be adding js files as well anyway... But I can see your point.

catch’s picture

Assigned: catch » Unassigned

Unassigning me since I was able to look at this, looks much better with the request stuff outside the hook_library_info() yes.

nod_’s picture

Status: Needs review » Reviewed & tested by the community

from #108: Rock & roll.

Dave Reid’s picture

Issue tags: +Needs manual testing

This seems like the type of issue that could use a couple rounds of manual testing, correct?

nod_’s picture

Well I guess, it's been tested to death by me at least and I'm sure about a couple of other people that tested it as well.

webchick’s picture

Title: Explicitly declare all JS dependencies, don't use drupal_add_js » Change notice: Explicitly declare all JS dependencies, don't use drupal_add_js
Priority: Major » Critical
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

Cool, this patch seems like a great start to reducing our mobile JS footprint. Looks like everyone's concerns from above have been addressed as well.

Committed and pushed to 8.x. Thanks!

In general, I agree with manual testing (particularly in 7.x), but while we're in code thaw, as long as basic testing has been done, I think we'll catch any JS regressions pretty fast once patches like this get committed.

This definitely needs a change notice.

nod_’s picture

There is a change notice at https://drupal.org/node/1764252, feel free to make it better or tell me how to.

I'd like to thank SebCorbin for all his work that lead to this patch, he's been working in the shadows to make core use AMD (and he succeeded! Check out the AMD sandbox).

I've never seen that many people in one of my JS-related issue, thanks to all of you :)

Now, go enjoy the speed (makes a lot more difference without toolbar activated)!

webchick’s picture

Title: Change notice: Explicitly declare all JS dependencies, don't use drupal_add_js » Explicitly declare all JS dependencies, don't use drupal_add_js
Priority: Critical » Major
Status: Active » Fixed

Awesome, thanks for the change notice! And yay for faster Drupal. :)

Mark Theunissen’s picture

I went to read the change description but I don't know what AMD is. Perhaps a clearer explanation of that this means:

"This change will make possible the use of core JS in the context of AMD by contrib."

Otherwise, great work!!

SebCorbin’s picture

AMD : Asynchronous Module Definition

More info http://github.com/amdjs/amdjs-api/wiki/AMD and #1542344: Use AMD for JS architecture

Now Drupal will also be modular in the front-end :)

tim.plunkett’s picture

sun’s picture

c960657’s picture

Follow-up: This introduced a regression in the OpenID module. This is included in the latest patch for #1538462: Cannot log in with OpenID due to "required" attribute (another issue blocking usage of OpenID).

Dave Reid’s picture

I'm curious if anyone had tested how this works with themes that use scripts[] = myfile.js and the fact that themes cannot implement basic hooks (so no hook_libraries). I mean I guess a theme can use themename_libraries_alter() but this is adding extra complexity for users that normally don't need to touch PHP files. Is there a follow-up for this at all?

nod_’s picture

maybe not a direct follow-up but very related #1762204: Introduce Assetic compatibility layer for core's internal handling of assets where this'll turn drupal_add_js into a legacy function adding drupal.js, jquery.once, drupalSettings and jQuery as dependency for the script. Then it's just a matter of making scripts[] use drupal_add_js to add files.

sun’s picture

Status: Fixed » Closed (fixed)

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

larowlan’s picture

xjm’s picture

Untagging. Please remove the "Needs change notification" tag when the change notice task is complete.

xjm’s picture

Issue summary: View changes

drupal_add_js in color module

penyaskito’s picture

Landed to the change record by google search, and edited it for using render arrays.

Please verify this is correct:
https://drupal.org/node/1764252/revisions/view/2735445/6898303