Style error information

https://eslint.org/docs/rules/prefer-rest-params

Remark

- Code refactoring. Review it carefully :)

How to Review

## 1. Apply Patch
## 2. Review Code Changes
## 3. Confirm no Code Standard Errors
yarn & yarn lint:core-js-passing
## 4.1 If `NO` errors, mark the issue as `Reviewed & tested by the community` (Don't be shy, we're all friendly)
## 4.2 If `HAS` errors, fix it and upload a new patch (Just do it and you can!!!)

Background

- #2912962: Step 1 JS codestyle: [meta] Fix JS coding standards in core

- We adapted the airbnb coding standard (#2815077: Adopt airbnb javascript style guide v14.1 as new baseline javascript coding standards for Drupal 8 core and contrib), but we are not fully compliant to it yet.

More Information

- Using ES6 in Core
https://www.drupal.org/node/2815083

- To find JS code standard errors stats
cd core/ && yarn & yarn lint:core-js-stats

CommentFileSizeAuthor
#13 2915534-13.patch3.92 KBGrandmaGlassesRopeMan
#9 2915534-9.patch3.92 KBGrandmaGlassesRopeMan
#6 2915534-6.patch3.9 KBGrandmaGlassesRopeMan
#6 interdiff-2915534-4-6.txt873 bytesGrandmaGlassesRopeMan
#4 2915534-4.patch3.86 KBGrandmaGlassesRopeMan
#4 interdiff-2915534-2-4.txt941 bytesGrandmaGlassesRopeMan
#2 2915534-2.patch3.83 KBGrandmaGlassesRopeMan
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

drpal created an issue. See original summary.

GrandmaGlassesRopeMan’s picture

GrandmaGlassesRopeMan
he/him
Primary language English
Location 🌎
CreditAttribution: GrandmaGlassesRopeMan at Acquia commented
FileSize
2915534-2.patch3.83 KB

Status: Needs review » Needs work

The last submitted patch, 2: 2915534-2.patch, failed testing. View results

GrandmaGlassesRopeMan’s picture

GrandmaGlassesRopeMan
he/him
Primary language English
Location 🌎
CreditAttribution: GrandmaGlassesRopeMan at Acquia commented
Status: Needs work » Needs review
FileSize
interdiff-2915534-2-4.txt941 bytes
2915534-4.patch3.86 KB
nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ as a volunteer commented

ca do this no?
return Drupal.theme[func](...args);

GrandmaGlassesRopeMan’s picture

GrandmaGlassesRopeMan
he/him
Primary language English
Location 🌎
CreditAttribution: GrandmaGlassesRopeMan at Acquia commented
FileSize
interdiff-2915534-4-6.txt873 bytes
2915534-6.patch3.9 KB

Thanks @nod_! 🤘

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented
Status: Needs review » Reviewed & tested by the community 
+++ b/core/misc/debounce.es6.js
@@ -29,9 +29,8 @@
-  return function () {
+  return function (...args) {
     const context = this;
-    const args = arguments;
     const later = function () {

+++ b/core/misc/debounce.js
@@ -9,8 +9,11 @@ Drupal.debounce = function (func, wait, immediate) {
   return function () {
+    for (var _len = arguments.length, args = Array(_len), _key = 0; _key < _len; _key++) {
+      args[_key] = arguments[_key];
+    }

Its a bit sad that babel copies everything manually, but I guess its not really something to worry about.

+++ b/core/misc/drupal.es6.js
@@ -559,10 +559,9 @@ window.Drupal = { behaviors: {}, locale: {} };
    *   but also a complex object.
    */
-  Drupal.theme = function (func) {
-    const args = Array.prototype.slice.apply(arguments, [1]);
+  Drupal.theme = function (func, ...args) {
     if (func in Drupal.theme) {
-      return Drupal.theme[func].apply(this, args);
+      return Drupal.theme[func](...args);
     }
   };

Nice simplification

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 6: 2915534-6.patch, failed testing. View results

GrandmaGlassesRopeMan’s picture

GrandmaGlassesRopeMan
he/him
Primary language English
Location 🌎
CreditAttribution: GrandmaGlassesRopeMan at Acquia commented
Status: Needs work » Needs review
FileSize
2915534-9.patch3.92 KB
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented
Status: Needs review » Reviewed & tested by the community

B2RTBC

GrandmaGlassesRopeMan’s picture

GrandmaGlassesRopeMan
he/him
Primary language English
Location 🌎
CreditAttribution: GrandmaGlassesRopeMan at Acquia commented

🤘

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer and at Dazzle commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Patch no longer applies, because of error: core/.eslintrc.passing.json: patch does not apply.

GrandmaGlassesRopeMan’s picture

GrandmaGlassesRopeMan
he/him
Primary language English
Location 🌎
CreditAttribution: GrandmaGlassesRopeMan at Acquia commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
2915534-13.patch3.92 KB

Reroll death spiral continues.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer and at Dazzle commented
Status: Needs review » Reviewed & tested by the community

Patch applies, back to rtbc - I'm assuming the bot will confirm.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented

Here's why we do this: https://github.com/airbnb/javascript#es6-rest

Crediting @nod_ for their review.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed

Committed cf52b41 and pushed to 8.5.x. Thanks!

  • alexpott committed cf52b41 on 8.5.x 
    Issue #2915534 by drpal, nod_: JS codestyle: prefer-rest-params

Status: Fixed » Closed (fixed)

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