Updated: Comment #12

Problem/Motivation

This issue was opened because of performance considerations. $.each() is - depending on the browsers javascript engine - around 70% slower than a for loop (benchmark : http://jsperf.com/jquery-each-vs-for-loop). So changing the behavior will result in better frontend performance.

Since this patch touches Drupal.attachBehaviors that would result in a big impact for frontend performance.

$.each() also changes the loop scope and does not filter properties which causes bugs for example in autocomplete.

Proposed resolution

As a proposed resolution $.each() would be replaced by filtered for-loops.

Remaining tasks

none

User interface changes

none

API changes

none

- #1428524: Replace all $.each() with filtered for loop
- #1920888: Add some JS to make the dependent groups always checked (and readonly) when the master group is checked translatable
- #432962: Add option to disable password strength checking

Original report by [nod_]

This was fixed a year ago #1428524: Replace all $.each() with filtered for loop but a few slipped by me since then (please tag your issues with "JavaScript" properly if it has anything to do with a javascript file).

Reintroduced in:
#1920888: Add some JS to make the dependent groups always checked (and readonly) when the master group is checked translatable
#432962: Add option to disable password strength checking
And the views merge.

Patch soon.

Files: 
CommentFileSizeAuthor
#8 core-js-reremove-each-2057371-8.patch5.67 KBJelle_S
PASSED: [[SimpleTest]]: [MySQL] 58,429 pass(es).
[ View ]
#3 core-js-reremove-each-2057371-3.patch5.67 KBphiit
PASSED: [[SimpleTest]]: [MySQL] 57,969 pass(es).
[ View ]
#3 interdiff-2057371-1-3.txt693 bytesphiit
#1 core-js-reremove-each-2057371-1.patch5.68 KBnod_
PASSED: [[SimpleTest]]: [MySQL] 57,571 pass(es).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new5.68 KB
PASSED: [[SimpleTest]]: [MySQL] 57,571 pass(es).
[ View ]

Reviewing the patch I found there are 70 instances of '.each(' found using grep before applying the patch. After applying I still found 64 instances of '.each('.

I used the grep on core/modules and the command was a bit of a hack to figure out the correct amount but here goes:
grep -c -r --include=*.js ".each(" ./ | cut -d ":" -f 2 | awk '{s+=$1} END {print s}'

First part prints each file and the amount of instances found, the second part cuts so that only the number remains and the last one equals together the amount.

Am I doing something wrong here? Should the patch only apply to certain components?

edit: Figured out the problem on IRC. Changes are in order as far as I can understand and all the instances of $.each are replaced with a for loop.

StatusFileSize
new693 bytes
new5.67 KB
PASSED: [[SimpleTest]]: [MySQL] 57,969 pass(es).
[ View ]

Patched the extra newline.

great stuff, thanks for looking though the patch :)

We still need some help reviewing the content translation admin interface with this patch.

After discussing this issue on IRC, nod_ pointed out that this issue only concerns replacing .each() with this kind of structure: $.each(theArray, function () {});

This fix should be enough if the content translation admin interface is reviewed.

Issue tags:+D8MI

tagging d8mi

Status:Needs review» Needs work
Issue tags:+Needs reroll

Tagging for reroll.

StatusFileSize
new5.67 KB
PASSED: [[SimpleTest]]: [MySQL] 58,429 pass(es).
[ View ]

Reroll, I'll check the content translation admin interface as well.

Status:Needs work» Needs review
Issue tags:-Needs reroll

Everything on admin/config/regional/content-language seems to work as expected.

Status:Needs review» Reviewed & tested by the community

Also tested the content-language page and it works as before.

Also did a search and all remaining instances of $.each( I could find were in vendor code...

RTBCing

Assigned:Unassigned» dasrecht

working on it : updating the summary

Issue summary:View changes

updated summary

+1 on reroll too. Thanks :)

Status:Reviewed & tested by the community» Needs review

looks like manual testing was done in #10 and #11 for the content translation settings, but not the views or password checker.

@dasrecht is doing that now. :) at the sprint.

Status:Needs review» Reviewed & tested by the community
  1. +++ b/core/modules/user/user.js
    @@ -75,9 +75,8 @@ Drupal.behaviors.password = {
    +      passwordInput.on('input', passwordCheck);
    +      confirmInput.on('input', passwordCheck);

    tested, works as before

  2. +++ b/core/modules/views/js/ajax_view.js
    @@ -12,9 +12,12 @@
    +    var ajaxViews = drupalSettings.views.ajaxViews;
    +    for (var i in ajaxViews) {
    +      if (ajaxViews.hasOwnProperty(i)) {
    +        Drupal.views.instances[i] = new Drupal.views.ajaxView(ajaxViews[i]);
    +      }
    +    }

    ok tested with a little help of nod_ and it works :-)

    How to test : Create a View, Enable Ajax:on use a pager or exposed filters. The Javascript is ran when you visit the page.

Issue tags:-Needs manual testing

Manual testing has been completed.

Assigned:dasrecht» Unassigned

Move it to unassigned.

Status:Reviewed & tested by the community» Fixed

Committed and pushed to 8.x. Thanks!

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

Issue summary:View changes

Removed the Remaining Testing Task done in #10 and #11