Because all script dependencies are/should be declared, the 'weight' key in library files declaration is now useless.

State of the patch:
Everything seems to be working
Was a problem with dependencies in contextual module

Needs testing :)

Files: 
CommentFileSizeAuthor
#33 Module Test.jpg48.26 KBamol_tatkare
#33 Module Views Overlay.jpg69.83 KBamol_tatkare
#33 Module Views Overlay 2.jpg37.31 KBamol_tatkare
#32 core-js-lib-diet-1945262-32.patch11.9 KBpenyaskito
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,219 pass(es).
[ View ]
#32 core-js-lib-diet-1945262.interdiff.29-32.txt996 bytespenyaskito
#29 1945262.patch12.12 KBRobLoach
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,173 pass(es), 0 fail(s), and 99 exception(s).
[ View ]
#28 1945262.patch16.11 KBRobLoach
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,177 pass(es), 0 fail(s), and 99 exception(s).
[ View ]
#25 core-js-lib-diet-1945262-25.patch9.8 KBpenyaskito
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,192 pass(es).
[ View ]

Comments

nod_’s picture

Issue tags:+JavaScript clean-up

forgot tags

RobLoach’s picture

Yup.... Dependencies should determine asset order, not "weight".

Wim Leers’s picture

Status:Needs review» Needs work

This broke at least two tricky edge cases:

  1. contextual.toolbar.js wants to hide its pencil icon in the toolbar when the overlay is open. Including when the URL fragment indicates that the overlay should open, meaning that the drupalOverlayOpen event is triggered and Drupal.behaviors.contextualToolbar.attach() must already have been executed. The removal of the weights broke that. Note that we may not introduce a dependency here because contextual.toolbar.js does not depend on overlay-parent.js, but instead it listens to it in case it exists.
  2. tour.js adjusts its tour icon in the toolbar (and the corresponding tour tips) when the overlay is open. Analogous additional info here.

I think the only way to solve this is by setting up the listener outside of the Drupal behaviors, so that it runs earlier. I hope I'm wrong :)

nod_’s picture

Status:Needs work» Needs review
StatusFileSize
new11.76 KB
PASSED: [[SimpleTest]]: [MySQL] 56,760 pass(es).
[ View ]

Reroll.

Looks like contextual links work. Tour is still broken.

Wim Leers’s picture

Title:hook_library_info() diet» hook_library_info() diet (or: get rid of weights in favor of dependencies)

I had no idea what this issue was about — it's witty, but this is hopefully a bit more clear :)

nod_’s picture

Title:hook_library_info() diet (or: get rid of weights in favor of dependencies)» hook_library_info() diet, get rid of weights in favor of dependencies

You're right, wasn't clean. Little small adjustment to make it easier on the eyes.

Wim Leers’s picture

Issue tags:+front-end performance

This is blocked on a mechanism that uses a DAG (Directed Acyclic Graph) to determine the order of the assets. We can then work around the problem in #3 by having the ability to mark a library to be loaded before another one, and having the DAG take that into account.

(As per a call with sdboyer and a discussion with nod_.)

Related: #1014086-114: Consider using image derivative style generation of CSS and JS aggregates.

sun’s picture

Let's revisit this — I think we were able to remove some of the weights in the libraries.yml patch already, but there are certainly lots of remaining instances.

YesCT’s picture

Issue tags:-front-end performance+frontend performance

changing to use the more common tag, so the less common one can be deleted, so it does not show up in the auto complete and confuse people.

Wim Leers’s picture

nod_’s picture

StatusFileSize
new7.45 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,099 pass(es).
[ View ]

Reroll, There are two cases of conditional dependencies. I think the only problem could be with views-contextual but doesn't seems broken.

Anyway, last patch was before yml files so here it is.

RobLoach’s picture

Status:Needs review» Reviewed & tested by the community

Tested, clicked around Views. Works well.

<script src="http://localhost:8000/core/assets/vendor/jquery/jquery.min.js?v=2.1.3"></script>
<script src="http://localhost:8000/core/assets/vendor/domready/ready.min.js?v=1.0.7"></script>
<script src="http://localhost:8000/core/misc/drupal.js?v=8.0.0-dev"></script>
<script src="http://localhost:8000/core/assets/vendor/underscore/underscore-min.js?v=1.7.0"></script>
<script src="http://localhost:8000/core/assets/vendor/backbone/backbone-min.js?v=1.1.2"></script>
<script src="http://localhost:8000/core/assets/vendor/jquery-once/jquery.once.js?v=1.2.3"></script>
<script src="http://localhost:8000/core/modules/contextual/js/contextual.js?v=8.0.0-dev"></script>
<script src="http://localhost:8000/core/modules/contextual/js/models/StateModel.js?v=8.0.0-dev"></script>
webchick’s picture

Status:Reviewed & tested by the community» Fixed

Oh, nice! Less code, same functionality.

Committed and pushed to 8.0.x. Thanks!

  • webchick committed dc5ef57 on 8.0.x
    Issue #1945262 by nod_: Replace custom weights with dependencies in...
catch’s picture

Could someone cross-link the issue that actually removes weight support for js? I can't find it atm.

Wim Leers’s picture

Status:Fixed» Active

Please revert this commit; it broke at least /admin/config/development/testing. Hence, this wasn't sufficiently manually tested. Is it possible that #12 only tested the JS in the Views module?

  • webchick committed 94c7316 on
    Revert "Issue #1945262 by nod_: Replace custom weights with dependencies...
webchick’s picture

Status:Active» Needs work
Issue tags:+Needs manual testing

Sorry about that! Sometimes my enthusiasm for a small RTBC queue gets the better of me. :) Reverted.

swentel’s picture

StatusFileSize
new26.37 KB

yes, you can't filter and all checkboxes are gone ...

penyaskito’s picture

Status:Needs work» Needs review
StatusFileSize
new8.38 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,205 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new959 bytes

Removed simpletest_js_alter(). Simpletest form works again.

penyaskito’s picture

AttachedAssetsTest::testAlter() has a @see for simpletest_js_alter(). We should do something about it if we remove it.

penyaskito’s picture

btw, after this change the only hook_alter_js() implementation in core is locale :_)

Status:Needs review» Needs work

The last submitted patch, 20: core-js-lib-diet-1945262-20.patch, failed testing.

penyaskito’s picture

simpletest depends on drupal.tableselect, so I guess we should just remove the test.

penyaskito’s picture

Status:Needs work» Needs review
StatusFileSize
new9.8 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,192 pass(es).
[ View ]
new1.41 KB

Done that.

RobLoach’s picture

Rather than removing the test, it might be better to add a mock that tests the hook_js_alter() functionality. It's in core, and it should be tested.

penyaskito’s picture

Status:Needs review» Needs work

Needs work then.

RobLoach’s picture

Status:Needs work» Needs review
StatusFileSize
new16.11 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,177 pass(es), 0 fail(s), and 99 exception(s).
[ View ]
RobLoach’s picture

StatusFileSize
new12.12 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,173 pass(es), 0 fail(s), and 99 exception(s).
[ View ]

Wrong file, sorry. Here's the correct patch.

The last submitted patch, 28: 1945262.patch, failed testing.

Status:Needs review» Needs work

The last submitted patch, 29: 1945262.patch, failed testing.

penyaskito’s picture

Status:Needs work» Needs review
StatusFileSize
new996 bytes
new11.9 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,219 pass(es).
[ View ]

@RobLoach: Thanks for the test! I changed it so the module is only enabled for testAlter. I could have added a check for jquery in the test module, but this looks cleaner to me.

amol_tatkare’s picture

StatusFileSize
new37.31 KB
new69.83 KB
new48.26 KB

I have tested the issue on views and test module. Patch #32 is working fine.

Please find attached screen shot.

amol_tatkare’s picture

Issue tags:+#DCM2015

Adding Issue tag as #DCM2015. This patch was tested in Code Sprint.

penyaskito’s picture

Issue tags:+JavaScript
RobLoach’s picture

+++ b/core/modules/system/src/Tests/Common/AttachedAssetsTest.php
@@ -389,21 +389,23 @@ function testRenderDifferentWeight() {
+    $this->enableModules(['attached_assets_test']);

Elegant solution :-) . What other places should we manually test before this goes RTBC?

penyaskito’s picture

There isn't any other hook_js_alter() but locale, and that one already fails in HEAD and is being worked on #2419897: Javascript translations are loaded in the wrong order due to missing dependencies.
So I guess we are good.
Aside of that, I don't know the *official* procedure for manually testing JS patches. IMHO there is no problem with RTBCing this one.

Wim Leers’s picture

Status:Needs review» Needs work

This patch makes several comment lies. We either need to remove those comments by verifying that the ordering requirements that used to exist no longer apply, or we can't remove all weights just yet…

  1. +++ b/core/modules/contextual/contextual.libraries.yml
    @@ -3,14 +3,14 @@ drupal.contextual-links:
         # Ensure to run before contextual/drupal.context-toolbar.
         # Core.
    -    js/contextual.js: { weight: -2 }
    +    js/contextual.js: {}

    This comment is now a lie. And I'm afraid this means we'll have to keep this one for now.

  2. +++ b/core/modules/simpletest/simpletest.module
    @@ -49,18 +49,6 @@ function simpletest_theme() {
    -function simpletest_js_alter(&$javascript, AttachedAssetsInterface $assets) {
    -  // Since SimpleTest is a special use case for the table select, stick the
    -  // SimpleTest JavaScript above the table select.
    -  $simpletest = drupal_get_path('module', 'simpletest') . '/simpletest.js';
    -  if (array_key_exists($simpletest, $javascript) && array_key_exists('core/misc/tableselect.js', $javascript)) {
    -    $javascript[$simpletest]['weight'] = $javascript['core/misc/tableselect.js']['weight'] - 1;
    -  }

    Why can this simply be removed?

    This comment says simpletest.js must run *before* tableselect.js. But the library definition says the opposite:

    drupal.simpletest:
      version: VERSION
      js:
        simpletest.js: {}
      css:
        component:
          css/simpletest.module.css: {}
      dependencies:
        - core/jquery
        - core/drupal
        - core/drupalSettings
        - core/jquery.once
        - core/drupal.tableselect
        - core/drupal.debounce

    Which is it?

  3. +++ b/core/modules/views/views.libraries.yml
    @@ -21,7 +21,7 @@ views.contextual-links:
         # Ensure to run before contextual/drupal.contextual-links.
    -    js/views-contextual.js: { weight: -10 }
    +    js/views-contextual.js: {}

    This comment is now also a lie.

penyaskito’s picture

Thanks for the review @Wim!

I'm not sure how to proceed.

1. and 3.) I tested diverse cases of contextual links with different contextual items (nodes, blocks, views) + inline editing using the standard profile and found no errors. It worked as expected and they were included in the same order than previously. Maybe is just *luck*.

2) simpletest.js is included after tableselect.js, but it works as expected (which makes me ask: is *included* order the same than *executed* order? I guess yes and the order is not relevant anymore, but clarification would help).

How can we verify that the order is not relevant anymore? Git log and check the changes on the files?

In the meantime, while testing 2) I found a different problem. We have e.g testing groups Action and action, and clicking on the checkboxes has the desired behavior. But if we click on the labels, always the same group is checked/unchecked. The problem relies on having the same lowercased element id in both checkboxes. If we think about keeping the casing is works for my browser (Google Chrome), but we should not rely on that as per http://www.w3.org/TR/html401/struct/links.html#h-12.2.1 it is invalid to have the same id even with different casing.
I will create an issue for that tomorrow, too late here...

Wim Leers’s picture

#39: manually tested both of these.

  1. This indeed works fine in HEAD, but it is not guaranteed. This is why we need "before" and "after" relationships: contextual.toolbar.js must run after contextual.js. But it does NOT depend on contextual.js: if contextual.js hasn't run, then it short-circuits itself: if there are no contextual links on the page, then contextual.js won't have been loaded, and there is no need to show the corresponding toolbar tab (the pencil icon) in the toolbar.
    This need is discussed several times in #1762204: Introduce Assetic compatibility layer for core's internal handling of assets but apparently it doesn't have its own dedicated issue.
    Due to lack of strong guarantees, I'm suggesting to keep this weight declaration in place for now. Until we have the ability to declare "before" and "after" relationships.
  2. How can we verify that the order is not relevant anymore? Git log and check the changes on the files?

    Exactly.

    Also: oops, I was clearly confused. SimpleTest's JS depends on tableselect.js is equivalent with setting the tablselect.js weight lower! My bad. So this change is actually fine already :) (That hook_js_alter() implementation should have been removed when it was converted into a library, but that was clearly forgotten.)

  3. This is only working correctly thanks to an implementation detail of contextual.js.

    This case is similar to the one in point 1, but instead of needing an "after" relationship, this one needs a "before" relationship.

Therefore, I propose to remove the changes I quoted in #38.1 and #38.3. The rest we can keep. That brings down the total number of "weight" usages to *two*. That'll make actually removing it easier, and a follow-up issue to add support for "before" and after" simpler too.

Pinging catch for his thoughts on adding "before" and "after" besides only "dependencies" to asset library declarations.

nod_’s picture

Status:Needs work» Postponed
Related issues:+#2367655: Control the list of behaviors run by Drupal.attachBehaviors

We can't do that, otherwise we end up with contextual.js before jquery, drupaljs and the rest and things crash. Need to remove all weights or none.

Wen we remove all the things, we can implement behavior order execution by extending #2367655: Control the list of behaviors run by Drupal.attachBehaviors and having something declared in the Drupal.behaviors objects.

nod_’s picture