[2/2] JS codestyle: no-restricted-syntax

Some more cleanup.

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

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

+++ 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.

Reserved. Making some cleanups

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.

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)

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

#2 removes the self ref.

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

✗
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.

"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...

- reroll from #26.

- some more updates.

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

@dawehner Lets see how this goes with the tests.

@dawehner So #41 is it...

FYI part 1 introduced an unexpected regression see #2941106: Site email address in the install profile form is no longer copied to the user email address.

#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

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

cc. @dawehner

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.

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.

- 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.

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.

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?

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.

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

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

+++ 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.

Something like the patch attached.

@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().

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.

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

@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!