Style error information

https://eslint.org/docs/rules/no-use-before-define

Remark

Code Improve. Less harmful.

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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

drpal created an issue. See original summary.

GrandmaGlassesRopeMan’s picture

Status: Active » Needs review
FileSize
154.93 KB

- This patch is quite large, 154.93 KB

borisson_’s picture

Status: Needs review » Reviewed & tested by the community

I ran yarn lint:core-js-passing before and after applying the patch and got the same result.

dawehner’s picture

I went through the patch in phpstorm and tried to imagine possible problems (Altering "global" state). This patch improves the situation in these cases!

  1. +++ b/core/modules/ckeditor/js/ckeditor.admin.es6.js
    @@ -215,6 +215,7 @@
              */
             function shutdown() {
    +          // eslint-disable-next-line no-use-before-define
               dialog.close(action);
    
    @@ -346,6 +347,7 @@
               $(event.target).remove();
             },
           });
    +
           // A modal dialog is used because the user must provide a button group
    

    I'm curious, what is the reason behind that?

  2. +++ b/core/modules/editor/js/editor.admin.es6.js
    @@ -248,6 +275,7 @@
    +          // eslint-disable-next-line no-use-before-define
               if (findPropertyValuesOnTag(universe, tag, property, propertyValues, allowing)) {
    

    I just checked this one ... we are calling A in function B and B in function A.

  3. +++ b/core/modules/quickedit/js/editors/formEditor.es6.js
    @@ -189,17 +189,17 @@
    -      function cleanUpAjax() {
    -        Drupal.quickedit.util.form.unajaxifySaving(formSaveAjax);
    -        formSaveAjax = null;
    -      }
    -
           // Create an AJAX object for the form associated with the field.
           let formSaveAjax = Drupal.quickedit.util.form.ajaxifySaving({
             nocssjs: false,
             other_view_modes: fieldModel.findOtherViewModes(),
           }, $submit);
     
    +      function cleanUpAjax() {
    +        Drupal.quickedit.util.form.unajaxifySaving(formSaveAjax);
    +        formSaveAjax = null;
    +      }
    +
    

    This is much nicer here. We now modify a value in a function which is used afterwards. 👍

+++ b/core/themes/seven/js/responsive-details.es6.js
@@ -15,6 +15,7 @@
       const $details = $(context).find('details').once('responsive-details');
+      const $summaries = $details.find('> summary');
 
       if (!$details.length) {

Should we move $summaries after the if ($!$details.length)?I know it shouldn't be an issue, but i think checking the result of $details first, still makes sense.

GrandmaGlassesRopeMan’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
154.8 KB
1.34 KB

@dawehner -

#4.1 - This one is a bit difficult to see, function shutdown() { //... } is actually part of function closeDialog(action, form) { //... } which is used in creation of the actual dialog.

#4.2 - Yes, without some major rewriting, it wasn't worth it.

#4.3 - 👍

#4.4(?) - Good point. I guess it could potentially throw an error.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Thank you @drpal!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/misc/states.es6.js
    @@ -22,6 +22,49 @@
    +  /**
    +   * These are helper functions implementing addition "operators" and don't
    +   * implement any logic that is particular to states.
    +   */
    

    This doesn't really make sense here anymore. It was a kind of divider of the file. I think we should just remove it.

  2. +++ b/core/modules/quickedit/js/editors/formEditor.js
    index ba742033fc..eaacc3b6c5 100644
    --- a/core/modules/quickedit/js/quickedit.es6.js
    
    --- a/core/modules/quickedit/js/quickedit.es6.js
    +++ b/core/modules/quickedit/js/quickedit.es6.js
    

    This one is really not easy to review.

  3. Should we open a followup to fix the bits where we had to ignore to not rewrite the logic?
GrandmaGlassesRopeMan’s picture

Status: Needs work » Needs review
FileSize
154.66 KB
437 bytes

@alexpott

#7.1 - Yep.
#7.2 - Yeah, sorry. The changes to quickedit.es6.js are moving a very large chunk of the file to the bottom from the top.
#7.3 - I don't really know. Some of the instances are minor, but having a followup for someone to investigate might be nice.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Thank you @drpal for fixing it!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Seems as though a recent change has introduced a new problem.

/Volumes/devdisk/dev/drupal/core/modules/big_pipe/js/big_pipe.es6.js
  23:24  error  'mapTextContentToAjaxResponse' was used before it was defined  no-use-before-define
alexpott’s picture

Status: Needs work » Needs review
FileSize
173.93 KB
327.75 KB

Moved the method.

alexpott’s picture

dawehner’s picture

Does someone mind to not add the yarn-error log?

alexpott’s picture

Whoopsie. Interdiff back to #8. Sorry for the noise.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Thank you alex

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 7d2f3a3 and pushed to 8.6.x. Thanks!

  • alexpott committed 7d2f3a3 on 8.6.x
    Issue #2946603 by drpal, alexpott, dawehner: JS codestyle: no-use-before...

Status: Fixed » Closed (fixed)

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