JS codestyle: no-multi-assign

1. +++ b/core/misc/displace.es6.js
   @@ -73,7 +73,8 @@
   +    Drupal.displace.offsets = offsets
   

   I know its not needed but afaik we use semicolons in all our JS.

2. +++ b/core/misc/states.js
   @@ -259,7 +261,8 @@
   +    this.pristine = state;
   +    this.name = state;
   

   In the other case we would have used this.name = this.pristine

3. +++ b/core/modules/ckeditor/js/ckeditor.admin.es6.js
   @@ -34,11 +34,12 @@
   -        const model = Drupal.ckeditor.models.Model = new Drupal.ckeditor.Model({
   +        const model = new Drupal.ckeditor.Model({
              $textarea,
              activeEditorConfig: JSON.parse($textarea.val()),
              hiddenEditorConfig: drupalSettings.ckeditor.hiddenCKEditorConfig,
            });
   +        Drupal.ckeditor.models.Model = model;
   

   In this case for example we use model just once. Could we not just get rid of it or do you think this would be out of scope here? At least for me a changed line feels better than a new line.

4. +++ b/core/modules/contextual/js/contextual.toolbar.es6.js
   @@ -22,7 +22,7 @@
   -    const model = contextualToolbar.model = new contextualToolbar.StateModel({
   +    const model = new contextualToolbar.StateModel({
          // Checks whether localStorage indicates we should start in edit mode
          // rather than view mode.
          // @see Drupal.contextualToolbar.VisualView.persist
   @@ -31,6 +31,8 @@
   
   @@ -31,6 +31,8 @@
          contextualCollection: Drupal.contextual.collection,
        });
    
   +    contextualToolbar.model = model;
   

   This is another one of those cases.

Thank you matt!

In reviewing the patch I was wondering why this is important... https://github.com/airbnb/javascript#variables--no-chain-assignment

After applying the patch I get an error:

yarn run build:js -- --check --file modules/ckeditor/js/ckeditor.admin.es6.js
yarn run v1.1.0
warning From Yarn 1.0 onwards, scripts don't require "--" for options to be forwarded. In a future version, any explicit "--" will be forwarded as-is to the scripts.
$ node ./scripts/js/babel-es6-build.js "--check" "--file" "modules/ckeditor/js/ckeditor.admin.es6.js"
[09:07:37] 'modules/ckeditor/js/ckeditor.admin.es6.js' is being checked.
[09:07:37] SyntaxError: modules/ckeditor/js/ckeditor.admin.es6.js: Unexpected token, expected , (45:16)
error Command failed with exit code 1.

In JS, sometimes, it's very easy to overlook. To check out the example code below:
http://jsbin.com/bayajepule/1/edit?js,console

@droplet thanks - I didn't complete my thought on #6 - the link https://github.com/airbnb/javascript#variables--no-chain-assignment also explains very nicely why this is important.

Ha, yeah they are not just braindead changes.

1. +++ b/core/misc/tabledrag.es6.js
   @@ -1107,13 +1107,16 @@
        if (document.all) {
   -      scrollY = this.scrollY = !de.scrollTop ? b.scrollTop : de.scrollTop;
   +      scrollY = !de.scrollTop ? b.scrollTop : de.scrollTop;
   +      this.scrollY = scrollY;
        }
        else {
   -      scrollY = this.scrollY = window.pageYOffset ? window.pageYOffset : window.scrollY;
   +      scrollY = window.pageYOffset ? window.pageYOffset : window.scrollY;
   +      this.scrollY = scrollY;
        }
   

   In this one, couldn't we just have one line that does this.scrollY = scrollY after the conditional? Since it's in both code paths identically.

2. +++ b/core/modules/quickedit/js/editors/plainTextEditor.es6.js
   @@ -30,10 +30,12 @@
          if ($fieldItems.length) {
   -        $textElement = this.$textElement = $fieldItems.eq(0);
   +        $textElement = $fieldItems.eq(0);
   +        this.$textElement = $textElement;
          }
          else {
   -        $textElement = this.$textElement = this.$el;
   +        $textElement = this.$el;
   +        this.$textElement = $textElement;
          }
   

   Same here for this.$textElement.

I'd love to have @xjm's eagle eyes on code reviews.

+++ b/core/modules/quickedit/js/editors/plainTextEditor.es6.js
@@ -27,16 +27,9 @@
       // changes.
-      let $textElement;
       const $fieldItems = this.$el.find('.quickedit-field');
-      if ($fieldItems.length) {
-        $textElement = $fieldItems.eq(0);
-        this.$textElement = $textElement;
-      }
-      else {
-        $textElement = this.$el;
-        this.$textElement = $textElement;
-      }
+      const $textElement = $fieldItems.length ? $fieldItems.eq(0) : this.$el;
+      this.$textElement = $textElement;

+1 for making this sane code overall

Crediting @xjm for the review in #11.

+++ b/core/misc/states.es6.js
@@ -12,7 +12,7 @@
-  const states = Drupal.states = {
+  const states = {

Needs docs update on this changes I think. And reverse the order seems more sense? So `states` is caching `Drupal.states` locally.

+++ b/core/modules/quickedit/js/editors/formEditor.es6.js
@@ -95,10 +95,11 @@
-      const $formContainer = this.$formContainer = $(Drupal.theme('quickeditFormContainer', {
+      const $formContainer = $(Drupal.theme('quickeditFormContainer', {
...
+      this.$formContainer = $formContainer;

same as above, reverse the order?

- toolbar.es6.js
- plainTextEditor.es6.js
has same issues.

+++ b/core/misc/states.js
@@ -259,7 +261,8 @@
-    this.pristine = this.name = state;
+    this.pristine = state;
+    this.name = state;

+++ b/core/misc/tabledrag.es6.js
@@ -1107,14 +1107,16 @@
-    const windowHeight = this.windowHeight = window.innerHeight || (de.clientHeight && de.clientWidth !== 0 ? de.clientHeight : b.offsetHeight);
+    const windowHeight = window.innerHeight || (de.clientHeight && de.clientWidth !== 0 ? de.clientHeight : b.offsetHeight);
+    this.windowHeight = windowHeight;

Why not use `this.windowHeight` directly? ( I will file another issue for the OR condition, I think this is outdated! It's for IE6~8 )

I crossposted a commit with @droplet - I reverted because of that.

Re the last part of @droplet's review in #16 I pondered this too but since removing outdated code paths for ie6-8 is not in scope I didn't mention it. I also didn't ask for a follow-up because doing that for all our existing JS is likely to an enormous issue and one that should have very careful scoping in order to be successful.

Why not use `this.windowHeight` directly? ( I will file another issue for the OR condition, I think this is outdated! It's for IE6~8 )

I totally think we should use this.... only in all those instances. What about going through the code in a follow up and clean all those places up?

I think no. 2 reasons:

1. this is a simple change
2. we should have 100% confidence to make this change. (I would wonder why not? Is that one of http://jsbin.com/bayajepule/1/edit?js,console issues)

One patch to final, no new introduced issue :)

explains why I did it:

1. +++ b/core/misc/displace.es6.js
   @@ -30,7 +30,7 @@
   -  let offsets = {
   +  const offsets = {
   

   with this change, we know it will be Object, not string assign. (Needs follow-up to fix docs so that has a better editor hints)

2. +++ b/core/misc/displace.es6.js
   @@ -73,12 +73,11 @@
   -    offsets = calculateOffsets();
   -    Drupal.displace.offsets = offsets;
   +    Drupal.displace.offsets = calculateOffsets();
   

   no helps for performance

3. +++ b/core/misc/states.es6.js
   @@ -7,20 +7,16 @@
   -   * Having the local states variable allows us to use the States namespace
   -   * without having to always declare "Drupal.states".
   ...
   -  const states = {
   

   I don't think we need a doc to explain everything. Code speaks itself already.

   only have to explain unexpected behaviors (e.g. may fall into #7 examples)

1. +++ b/core/misc/displace.es6.js
   @@ -30,7 +30,7 @@
       *
       * @type {Drupal~displaceOffset}
       */
   -  let offsets = {
   +  const offsets = {
   

   At least for me having this const up here and then merged as default on the other end of the file is a bit confusing. Maybe that's just me

2. +++ b/core/misc/states.es6.js
   @@ -7,19 +7,17 @@
       *
   -   * Having the local states variable allows us to use the States namespace
   -   * without having to always declare "Drupal.states".
   -   *
   

   Right, but this change is also simply not needed. We talked about the change and the core committer will stumble upon it as well, I'll promise you :) Still I agree with you that the comment is not needed actually.

I agree with @drpal that not having an additional variable is the ideal future. I personally think smaller steps are better, but I refer to the JS maintainers, @droplet and @drpal to make this call.

OK. I just give up :) Not worth to make a argue here for this little thing.

And the reason I trying to do a little more because I see @xjm comments and you're agreed :p

I re-upload @drpal' #13.

Thanks All!

Sorry, I forgot about it. Without my changes, it still needs a follow-up for the docs I pointed in #22. With #13 changes, the docs don't correct.

Thanks everyone -- I agree with keeping the scope of this constrained to things that can be reviewed in the immediate context of the patch. That way we can review and confirm the changes are correct without having to understand the whole file.

Removing totally unnecessary local variables is also useful though, as is removing dead code or unnecessary docs. I agree that those things can happen in a followup issue. Shall we file issues for those things and reference them here?

Meanwhile, committed to 8.5.x. Thanks!

Thanks.

1️⃣ #2918471: Remove local variables and replace them by this. or Drupal.states.