Style error information

https://eslint.org/docs/rules/no-restricted-syntax

Remark

Code Improve. Less hamful

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
yarn & yarn lint:core-js-stats

Valuable Follow-up

- core/modules/quickedit/js/quickedit.js throws JS error. It's not in testcases. It might requred `t` function to translate the string.

CommentFileSizeAuthor
#68 2917234-62.patch17.77 KBalexpott
#65 2917234-65.patch17.36 KBalexpott
#65 62-65-interdiff.txt2.27 KBalexpott
#62 2917234-62.patch17.77 KBalexpott
#62 61-62-interdiff.txt1.17 KBalexpott
#61 2917234-61.patch16.89 KBalexpott
#61 59-61-interdiff.txt1.79 KBalexpott
#59 2917234-59.patch16.84 KBalexpott
#59 57-59-interdiff.txt2.08 KBalexpott
#57 interdiff-2917234-56-57.txt1 KBdrpal
#57 2917234-57.patch17.05 KBdrpal
#56 2917234-56.patch16.92 KBalexpott
#56 54-56-interdiff.txt1.74 KBalexpott
#54 2917234-54.patch16.54 KBalexpott
#54 53-54-interdiff.txt1.69 KBalexpott
#53 interdiff-2917234-51-53.txt979 bytesdrpal
#53 2917234-53.patch16.89 KBdrpal
#51 interdiff-2917234-41-51.txt3.57 KBdrpal
#51 2917234-51.patch17.03 KBdrpal
#44 interdiff-2917234-41-43.txt1.31 KBdrpal
#43 2917234-43.patch20.05 KBdrpal
#41 2917234-41.patch20.18 KBdrpal
#41 interdiff-2917234-39-41.txt1.4 KBdrpal
#39 2917234-39.patch19.97 KBdrpal
#39 interdiff-2917234-37-39.txt1.32 KBdrpal
#37 2917234-37.patch19.9 KBdrpal
#37 interdiff-2917234-35-37.txt3.68 KBdrpal
#35 2917234-35.patch19.72 KBdrpal
#35 interdiff-2917234-32-35.txt12.63 KBdrpal
#32 2917234-32.patch9.57 KBdrpal
#26 2917234-26.patch9.55 KBdawehner
#22 2917234-hard-22.patch14.89 KBdawehner
#16 interdiff.patch15.92 KBdroplet
#16 2917234-16.patch92.83 KBdroplet
#14 interdiff-2917234.txt1.24 KBdawehner
#14 2917234-14.patch89.41 KBdawehner
#12 2917234-12.patch89.34 KBdawehner
#12 interdiff-2917234.txt4.21 KBdawehner
#9 interdiff-2917234-4-9.txt2.53 KBdrpal
#9 2917234-9.patch89.32 KBdrpal
#7 2917234-6.patch80.64 KBdawehner
#4 interdiff-2917234-2-4.txt45.36 KBdrpal
#4 2917234-4.patch89.8 KBdrpal
#2 2917234-2.patch43.26 KBdawehner
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

dawehner created an issue. See original summary.

dawehner’s picture

dawehner’s picture

Note: not all of them are converted yet.

drpal’s picture

Some more cleanup.

Status: Needs review » Needs work

The last submitted patch, 4: 2917234-4.patch, failed testing. View results

drpal’s picture

Oh, so it looks like the first patch did not contain the transpiled code. This might explain some of the errors.

dawehner’s picture

Status: Needs work » Needs review
FileSize
80.64 KB
  1. +++ b/core/modules/color/preview.es6.js
    @@ -47,23 +47,19 @@
    +          delta = [];
    +          Object.keys(color_start).forEach((colorStartKey) => {
    +            delta[colorStartKey] = (color_end[colorStartKey] - color_start[colorStartKey]) / (settings.gradients[gradientsKey].vertical ? height[gradientsKey] : width[gradientsKey]);
    +          });
    

    This could be a .map, do we care? I guess it would no longer be the minimal change? ... Note: Given that this is inside a foreach the for rule still kinda applies. Can we extract the function above first?

  2. +++ b/core/modules/filter/filter.filter_html.admin.es6.js
    @@ -181,7 +181,7 @@
    -      for (tag in editorRequiredTags) {
    +      Object.keys(editorRequiredTags).forEach((tag) => {
             // If userAllowedTags does not contain a rule for this editor-required
    ...
             if (!_.has(userAllowedTags, tag)) {
    

    I guess we could get rid of the if here?

  3. +++ b/core/modules/editor/js/editor.admin.es6.js
    @@ -567,14 +567,10 @@
    +      Object.keys(Drupal.filterConfiguration.statuses).some((filterID) => {
    +        const filterStatus = Drupal.filterConfiguration.statuses[filterID];
    +        return !filterStatusAllowsFeature(filterStatus, feature)
    +      });
     
           return true;
    

    This is wrong. You need to check the return value to be able to return false

Note: The patch I'm uploading is #3 with actual JS build. Sorry, for missing that.

Status: Needs review » Needs work

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

drpal’s picture

I think this may fix some of the issues. @dawehner, sorry, I didn't fix anything

drpal’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 9: 2917234-9.patch, failed testing. View results

dawehner’s picture

Status: Needs work » Needs review
FileSize
4.21 KB
89.34 KB

This is particularly awful.

drpal’s picture

+++ b/core/misc/states.es6.js
@@ -261,13 +261,15 @@
+        for (let i = 0; i < constraintsIds.length; i++) {

Store the length outside the for loop so we don't have to compute it each pass.

dawehner’s picture

droplet’s picture

Assigned: Unassigned » droplet

Reserved. Making some cleanups

droplet’s picture

Assigned: droplet » Unassigned
FileSize
92.83 KB
15.92 KB

wow. A huge patch. I wonder how much time @dawehner spent on it!!!

I haven't finished my review but clean up the vars scope that will help for reviewing I think. Hope I haven't messed it.

dawehner’s picture

+++ b/core/modules/color/color.es6.js
@@ -14,9 +14,6 @@
     attach(context, settings) {
-      let i;
-      let j;
-      let colors;
       // This behavior attaches by ID, so is only valid once on a page.
       const form = $(context).find('#system-theme-settings .color-form').once('color');

I try to understand this for example. You seem to no longer do the minimal changes. Everything beyond the minimal changes will make life harder for other reviewers and core committers and ultimately for you as well.

droplet’s picture

code like this creates a new `no-shadow` issues.

let i;
Object.keys(settings.gradients).forEach((i) => {

Most for-in loop, the var defined outside the block. I moved it back to block scope. (Including to remove unused var)

droplet’s picture

It's very risky to assume for-in and Object.keys is a 1:1 changes and scan the patch online only. (this style, @dawehner patch is easier to review)

But, if you applied offline and start to read the code (to check if there's any extra if-else around the code) and var scope..etc. Then, I think my extra change would help you to understand it better (& faster).

Thanks

dawehner’s picture

It's very risky to assume for-in and Object.keys is a 1:1 changes and scan the patch online only. (this style, @dawehner patch is easier to review)

That is a strong point. The different semantic of var and let/const is dangerous quite often.
I really try to think: What would the core committers say, simply to avoid some slowdown for our overall process.

  1. +++ b/core/misc/states.es6.js
    @@ -122,18 +121,11 @@
    -      let state;
    ...
           Object.keys(dependeeStates).forEach((i) => {
    -        state = dependeeStates[i];
    +        let state = dependeeStates[i];
    

    +1

  2. +++ b/core/misc/states.es6.js
    @@ -122,18 +121,11 @@
    -      const self = this;
    -
    -      function stateEventHandler(e) {
    -        self.update(e.data.selector, e.data.state, e.value);
    -      }
    
    @@ -146,7 +138,9 @@
    +        $(selector).on(`state:${state}`, { selector, state }, (e) => {
    +          this.update(e.data.selector, e.data.state, e.value);
    +        });
    

    This change seems not necessary even it is the right change on the longrun.

  3. +++ b/core/modules/filter/filter.filter_html.admin.es6.js
    @@ -135,16 +135,16 @@
    -      let featureName;
    -      let feature;
    -      let featureRule;
    -      let filterRule;
    -      let tag;
           const editorRequiredTags = {};
    ...
           Object.keys(newFeatures).forEach((featureName) => {
    -        feature = newFeatures[featureName];
    +        const feature = newFeatures[featureName];
    +        let featureRule;
    +        let filterRule;
    +        let tag;
    +
             for (let f = 0; f < feature.length; f++) {
               featureRule = feature[f];
    

    Can't we move those even into the for loop itself? I don't see them used outside of it.

droplet’s picture

#2 removes the self ref.

I will give up if it slows down the process. Sorry for the noise.

dawehner’s picture

Title: JS codestyle: no-restricted-syntax » [2/2] JS codestyle: no-restricted-syntax
Status: Needs review » Postponed
FileSize
14.89 KB

@xjm asked for splitting up this issue into reviewable chunks. #2925064: [1/2] JS codestyle: no-restricted-syntax now contains the easy changes.

xjm’s picture

dawehner’s picture

Status: Postponed » Needs review

Status: Needs review » Needs work

The last submitted patch, 22: 2917234-hard-22.patch, failed testing. View results

dawehner’s picture

Status: Needs work » Needs review
FileSize
9.55 KB

Here is my try to reroll this patch.

Status: Needs review » Needs work

The last submitted patch, 26: 2917234-26.patch, failed testing. View results

webchick’s picture


testImageDimensions
fail: [Other] Line 216 of core/modules/image/src/Tests/ImageDimensionsTest.php:
Value 40 is equal to value 41.

Hm. Nothing in this patch touches image styles; let's try that again.

webchick’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 26: 2917234-26.patch, failed testing. View results

webchick’s picture

Status: Needs work » Needs review

"exception: [Warning] Line 177 of core/lib/Drupal/Core/Cache/ApcuBackend.php:
apcu_store(): Unable to allocate memory for pool."

Sounds like a testbot issue...

drpal’s picture

- reroll from #26.

dawehner’s picture

+++ b/core/modules/editor/js/editor.admin.es6.js
@@ -567,17 +567,14 @@
+      const disallowedFeature = Object.keys(Drupal.filterConfiguration.statuses)
+        // We use some to do an early return when filterStatusAllowsFeature
+        // returns false.
+        .some((filterID) => {
           const filterStatus = Drupal.filterConfiguration.statuses[filterID];
-          if (!(filterStatusAllowsFeature(filterStatus, feature))) {
-            return false;
-          }
-        }
-      }
-
-      return true;
+          return !filterStatusAllowsFeature(filterStatus, feature);
+        });
+      return !disallowedFeature;

Reading through this bit again I'm quite convinced that this can be replaced by Object.keys(...).every(return filterStatusAllowsFeature)

Right now we have basically something like: NOT (NOT A OR NOT B). Using https://en.wikipedia.org/wiki/De_Morgan%27s_laws NR 2 we end up with NOT (NOT (A AND B)) which is A and B.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

drpal’s picture

- some more updates.

Status: Needs review » Needs work

The last submitted patch, 35: 2917234-35.patch, failed testing. View results

drpal’s picture

Status: Needs work » Needs review
FileSize
3.68 KB
19.9 KB

- the one where we don't break table drag.

Status: Needs review » Needs work

The last submitted patch, 37: 2917234-37.patch, failed testing. View results

drpal’s picture

Status: Needs work » Needs review
FileSize
1.32 KB
19.97 KB

Status: Needs review » Needs work

The last submitted patch, 39: 2917234-39.patch, failed testing. View results

drpal’s picture

Status: Needs work » Needs review
FileSize
1.4 KB
20.18 KB
dawehner’s picture

+++ b/core/misc/states.es6.js
@@ -260,18 +252,23 @@
-        // eslint-disable-next-line no-restricted-syntax
-        for (const n in constraints) {
-          if (constraints.hasOwnProperty(n)) {
-            result = ternary(result, this.checkConstraints(constraints[n], selector, n));
-            // False and anything else will evaluate to false, so return when
-            // any false condition is found.
-            if (result === false) {
-              return false;
-            }
+        if (Object.keys(constraints).every((constraint) => {
+          result = ternary(
+            result,
+            this.checkConstraints(
+              constraints[constraint],
+              selector,
+              constraint,
+            ),
+          );
+          if (result === false) {
+            return false;
           }
+        })) {
+          return false;
         }

I stared at that code now for a while. If result is true before, we return true, just as before. When result was false before, but one of the conditions is false, we return false. To be honest I'm a bit confused why we don't jut always return result in the every() function.

drpal’s picture

@dawehner Lets see how this goes with the tests.

drpal’s picture

FileSize
1.31 KB
dawehner’s picture

Let's hope for the best :P

Status: Needs review » Needs work

The last submitted patch, 43: 2917234-43.patch, failed testing. View results

drpal’s picture

@dawehner So #41 is it...

dawehner’s picture

Status: Needs work » Reviewed & tested by the community

yeah, let's go with #41. To be honest the existing code is far from being easy to understand either, given its local state is using something from far above.

alexpott’s picture

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

#41 makes yarn lint:core-js-passing fail with:

/Volumes/devdisk/dev/sites/drupal8alt.dev/core/misc/states.es6.js
  258:57  error  Expected to return a value at the end of arrow function  array-callback-return

✖ 1 problem (1 error, 0 warnings)

Also the changes to color.es6.js appear to be out-of-scope for fixing JS codestyle: no-restricted-syntax - they seem to be fixing no-shadow

drpal’s picture

Status: Needs work » Needs review
FileSize
17.03 KB
3.57 KB

- removes changes to color.es6.js
- fixes expected return.

Status: Needs review » Needs work

The last submitted patch, 51: 2917234-51.patch, failed testing. View results

drpal’s picture

Status: Needs work » Needs review
FileSize
16.89 KB
979 bytes

cc. @dawehner

alexpott’s picture

I think we can simplify the logic here if we just accept that we should check all the constraints. If we want the early return we could use .some and invert the logic but that would be head-scratching.

Status: Needs review » Needs work

The last submitted patch, 54: 2917234-54.patch, failed testing. View results

alexpott’s picture

Status: Needs work » Needs review
FileSize
1.74 KB
16.92 KB

Wow there is something very strange going on inside checkConstraints

Patch attached passes the failing test and uses .some to continue to have the performance hack of not checking all the constraints if one evals to false.

drpal’s picture

- ternary() was slightly too long, broke it up into multiple lines.
- fixed multi-line comment format.

@alexpott - I think this looks really good. 👍for RTBC if this patch works.

dawehner’s picture

I'm a bit confused. Isn't every and some just two side of the same spectrum given that we want an early return?
To be honest I'm not sure that the some/every really increases readability. An ordinary for loop could be better overall.

alexpott’s picture

Well the problem is caused by

  function ternary(a, b) {
    if (typeof a === 'undefined') {
      return b;
    }
    else if (typeof b === 'undefined') {
      return a;
    }

    return a && b;
  }

And the fact that if both are undefined it returns undefined as the function description says: Bitwise AND with a third undefined state.. Ternary not always returning FALSE or TRUE is pretty magic.

Ah this leads me a more elegant solution. All the problems are happening because of the initial state of result being undefined and checkConstraints also returning undefined.

drpal’s picture

Just a very minor nit. Otherwise, this looks great, and awesome debug work @alexpott.

+++ b/core/misc/states.es6.js
@@ -261,18 +253,16 @@
+        // This constraint is an object (AND). The ternary function will return
+        // true if the result of checking the constraint is undefined.

Can we switch this to the multi-line comment format?

alexpott’s picture

So this is the only usage of ternary() - given that I think we should just inline the logic and not use it. Because calling ternary with one argument as true is pretty weird.

alexpott’s picture

Oh and we can actually remove ternary() because it is no longer used and it is not part of our javascript API.

drpal’s picture

Status: Needs review » Reviewed & tested by the community

Alright, awesome. Tests are green, and the logic here makes a lot more sense.

alexpott’s picture

+++ b/core/misc/tabledrag.es6.js
@@ -412,23 +410,18 @@
-    // eslint-disable-next-line no-restricted-syntax
-    for (const delta in tableSettingsGroup) {
-      if (tableSettingsGroup.hasOwnProperty(delta)) {
-        const targetClass = tableSettingsGroup[delta].target;
-        if (field.is(`.${targetClass}`)) {
-          // Return a copy of the row settings.
-          const rowSettings = {};
-          // eslint-disable-next-line no-restricted-syntax
-          for (const n in tableSettingsGroup[delta]) {
-            if (tableSettingsGroup[delta].hasOwnProperty(n)) {
-              rowSettings[n] = tableSettingsGroup[delta][n];
-            }
-          }
-          return rowSettings;
-        }
+    return Object.keys(tableSettingsGroup).map((delta) => {
+      const targetClass = tableSettingsGroup[delta].target;
+      let rowSettings;
+      if (field.is(`.${targetClass}`)) {
+        // Return a copy of the row settings.
+        rowSettings = {};
+        Object.keys(tableSettingsGroup[delta]).forEach((n) => {
+          rowSettings[n] = tableSettingsGroup[delta][n];
+        });
       }
-    }
+      return rowSettings;
+    }).filter(rowSetting => rowSetting)[0];

I've stared at this for considerable amounts of time and whilst I think the return value is always going to be the same I'm pretty sure that we're doing quite a lot of unnecessary looping here because we've removed the early return. But what feels more icky is that the new code is harder to understand. Could we change the flow here to first determine the and once we have that delta use it to return the rowSettings. This way we wouldn't have to filter.

alexpott’s picture

Something like the patch attached.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 65: 2917234-65.patch, failed testing. View results

drpal’s picture

@alexpott Can we do any kind of profiling to see if any of these loops actually cause some sort of performance impact? I personally don't think the new code from #62 is all that complicated, and actually a bit more understandable since we dropped ternary().

alexpott’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
17.77 KB

Looks like we can't use es6 functions like .find() yet. Let's go back to #62 - at least #65 proves we have test coverage.

@drpal the change in #65 is about the tabledrag changes not the states changes. I agree the loss of the early return in states is no biggie.

drpal’s picture

@alexpott - Looks like no .find() support in IE. Eventually we should have a polyfill discussion around handling these.

dawehner’s picture

Browsers are so hard

Big +1 for the new bits though.
I think @alexpott should not have tried to use .find. This issue is not about endless refactoring, because well, we could do that forever. We should focus on not doing that.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

@dawehner the reason I tried to improve it was because the new formulation in tabledrag is harder to understand than before as pointed out in #64 - I agree we shouldn't be endlessly refactoring but refactoring into less readable code should be avoided where possible - which is what I was trying to do.

So we've got @dawehner's and @drpal's and I've reviewed the code changes extensively plus we've proved there is test coverage. Time to get this done.

Committed b4064da and pushed to 8.6.x. Thanks!

  • alexpott committed b4064da on 8.6.x
    Issue #2917234 by drpal, alexpott, dawehner, droplet: [2/2] JS codestyle...