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.

CommentFileSizeAuthor
#8 core-js-reremove-each-2057371-8.patch5.67 KBJelle_S
#3 core-js-reremove-each-2057371-3.patch5.67 KBphiit
#3 interdiff-2057371-1-3.txt693 bytesphiit
#1 core-js-reremove-each-2057371-1.patch5.68 KBnod_
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented
Status: Active » Needs review
FileSize
core-js-reremove-each-2057371-1.patch5.68 KB
phiit’s picture

phiit CreditAttribution: phiit commented

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.

phiit’s picture

phiit CreditAttribution: phiit commented
FileSize
interdiff-2057371-1-3.txt693 bytes
core-js-reremove-each-2057371-3.patch5.67 KB

Patched the extra newline.

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented
Issue tags: +JavaScript, +Novice, +Needs manual testing

great stuff, thanks for looking though the patch :)

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

phiit’s picture

phiit CreditAttribution: phiit commented

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.

YesCT’s picture

YesCT CreditAttribution: YesCT commented
Issue tags: +D8MI

tagging d8mi

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented
Status: Needs review » Needs work
Issue tags: +Needs reroll

Tagging for reroll.

Jelle_S’s picture

Jelle_S
Primary language Dutch
Location Antwerp, Belgium
CreditAttribution: Jelle_S commented
FileSize
core-js-reremove-each-2057371-8.patch5.67 KB

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

Jelle_S’s picture

Jelle_S
Primary language Dutch
Location Antwerp, Belgium
CreditAttribution: Jelle_S commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
Jelle_S’s picture

Jelle_S
Primary language Dutch
Location Antwerp, Belgium
CreditAttribution: Jelle_S commented

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

seutje’s picture

seutje CreditAttribution: seutje commented
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

dasrecht’s picture

dasrecht CreditAttribution: dasrecht commented
Assigned: Unassigned » dasrecht

working on it : updating the summary

dasrecht’s picture

dasrecht CreditAttribution: dasrecht commented
Issue summary: View changes

updated summary

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented

+1 on reroll too. Thanks :)

YesCT’s picture

YesCT CreditAttribution: YesCT commented
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.

dasrecht’s picture

dasrecht CreditAttribution: dasrecht commented
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.

dasrecht’s picture

dasrecht CreditAttribution: dasrecht commented
Issue tags: -Needs manual testing

Manual testing has been completed.

dasrecht’s picture

dasrecht CreditAttribution: dasrecht commented
Assigned: dasrecht » Unassigned

Move it to unassigned.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

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

Anonymous’s picture

(not verified) CreditAttribution: commented
Issue summary: View changes

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