Contrib.social

Fix eslint in 2.x branch

Open on Drupal.org →
Created on 9 December 2023, 7 months ago
Updated 9 April 2024, 3 months ago

Problem/Motivation

While I was updating the 2.x branch to use Drupal.org's GitLab CI → for automated linting and testing, the new eslint lints started identifying a bunch of issues with the JavaScript and/or YAML in the project. At time-of-writing, the most recent set of lints were in job 462178.

I tried automatically fixing the issues identified, but there were some that were non-trivial to fix, and my JS isn't good enough to the best way to fix these, so I have left the file as-is (i.e.: without committing the automatic fixes). Here are the lints left over after the automatic fixes:

web/modules/contrib/multiselect/js/multiselect.js
    5:2   warning  Unexpected unnamed function                     func-names
   34:37  warning  Unexpected unnamed function                     func-names
   40:9   warning  Unexpected unnamed function                     func-names
   46:25  warning  Unexpected unnamed function                     func-names
   54:44  warning  Unexpected unnamed function                     func-names
   62:43  warning  Unexpected unnamed function                     func-names
   70:35  warning  Unexpected unnamed function                     func-names
   79:38  warning  Unexpected unnamed function                     func-names
  103:23  warning  Unexpected unnamed function                     func-names
  104:13  warning  Unexpected unnamed function                     func-names
  116:32  warning  Unexpected unnamed function                     func-names
  117:16  error    Use the rest parameters instead of 'arguments'  prefer-rest-params
  118:13  warning  Unexpected unnamed function                     func-names
  129:29  warning  Unexpected unnamed function                     func-names
  130:16  error    Use the rest parameters instead of 'arguments'  prefer-rest-params
  131:13  warning  Unexpected unnamed function                     func-names
  137:9   warning  Unary operator '--' used                        no-plusplus
  147:23  warning  Unexpected unnamed function                     func-names
  148:18  error    Use the rest parameters instead of 'arguments'  prefer-rest-params
  149:13  warning  Unexpected unnamed function                     func-names
  164:26  warning  Unexpected unnamed function                     func-names
  165:22  error    Use the rest parameters instead of 'arguments'  prefer-rest-params
  166:13  warning  Unexpected unnamed function                     func-names
  169:24  error    Expected '===' and instead saw '=='             eqeqeq

✖ 24 problems (5 errors, 19 warnings)

Steps to reproduce

To reproduce this locally...

  1. Download and install Drupal 10.1.
  2. Make sure that JS packages are installed (one of which is eslint):
    1. cd web/core
    2. yarn install
    3. cd ../..
  3. Install the multiselect module and dependencies, and get it ready for development:
    1. composer require --prefer-install=source "drupal/multiselect"
    2. cd web/modules/contrib/multiselect
    3. git fetch --all -t -p
    4. git checkout 2.x
    5. git pull
    6. (follow the git instructions for this issue)
  4. Set up eslint config files like the GitLab CI job does. Note this ignores the files so they won't accidentally get committed. We probably don't want to commit them so that Drupal core's JS standards can evolve upstream:
    1. cp ../../../core/.prettierrc.json . && echo ".prettierrc.json" | tee -a .git/info/exclude
    2. echo '*.yml' > .prettierignore && echo ".prettierignore" | tee -a .git/info/exclude
  5. Run eslint - this will show all 94 problems, including ones that can be automatically fixed: ../../../core/node_modules/.bin/eslint --no-error-on-unmatched-pattern --ignore-pattern="*.es6.js" --resolve-plugins-relative-to=../../../core --ext=.js,.yml .: this should match what you currently see in GitLab CI, i.e.: in job 462178
  6. Ask eslint to fix the problems it can fix safely: ../../../core/node_modules/.bin/eslint --no-error-on-unmatched-pattern --ignore-pattern="*.es6.js" --resolve-plugins-relative-to=../../../core --ext=.js,.yml . --fix : this will do what it can and write out the problems it can't fix, which will probably match what I pasted in the Problem/Motivation section above.
  7. (fix the errors)
  8. Make sure to run eslint one more time before committing: ../../../core/node_modules/.bin/eslint --no-error-on-unmatched-pattern --ignore-pattern="*.es6.js" --resolve-plugins-relative-to=../../../core --ext=.js,.yml .

Proposed resolution

Fix the errors.

Remaining tasks

  2. Write a patch or merge request.
  3. Review and feedback
  4. RTBC and feedback
  5. Commit

User interface changes

None.

API changes

None.

Data model changes

None.

✨ Feature request
Status

Fixed

Version

2.0

Component

Code

Created by

🇨🇦Canada mparker17 UTC-4

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @mparker17
  • Comment 7 months ago →
    🇨🇦Canada mparker17 UTC-4

    Whoops I made a small typo in the issue summary.

  • Assigned to Bobik
  • Comment 5 months ago →
    🇺🇦Ukraine Bobik
  • Merge request !5Issue #3273331: Fix eslint in 2.x branch → (Merged) created by Unnamed author
  • Pipeline finished with Failed
    5 months ago
    #77600
  • Pipeline finished with Failed
    5 months ago
    Total: 177s
    #77895
  • Pipeline finished with Failed
    5 months ago
    Total: 150s
    #77938
  • Pipeline finished with Success
    5 months ago
    Total: 145s
    #80646
  • Pipeline finished with Canceled
    5 months ago
    Total: 55s
    #80651
  • Pipeline finished with Success
    5 months ago
    Total: 202s
    #80652
  • Pipeline finished with Success
    5 months ago
    Total: 146s
    #80654
  • Pipeline finished with Success
    5 months ago
    Total: 144s
    #80660
  • Pipeline finished with Success
    5 months ago
    #80669
  • Pipeline finished with Success
    5 months ago
    Total: 144s
    #80672
  • Pipeline finished with Success
    5 months ago
    Total: 143s
    #80673
  • Issue was unassigned.
  • Status changed to Needs review 5 months ago
  • Comment 5 months ago →
    🇺🇦Ukraine Bobik

    I've fixed the issue. Please check it.

  • Comment 5 months ago →
    System Message
  • Status changed to Fixed 5 months ago
  • Comment 5 months ago →
    🇨🇦Canada mparker17 UTC-4

    Code review looks good; manual testing works; automated tests work. Looks good to me! Thanks very much @bobi-mel!

  • Comment 5 months ago →
    🇺🇦Ukraine Bobik

    Thanks for the really fast review.

  • Comment 5 months ago →
    🇩🇪Germany rgpublic Düsseldorf 🇩🇪 🇪🇺

    Um, this commit changed jQuery.fn.selectAll to an arrow function. Due to this change, the selectAll function doesn't seem to work anymore it seems - causing JS errors. I guess the reason is that the semantics of "this" has been changed inadvertently. Using the arrow function, "this" is now pointing to a window and not to the JQuery object anymore.

  • Status changed to Needs work 5 months ago
  • Comment 5 months ago →
    🇨🇦Canada mparker17 UTC-4

    @rgpublic, thanks for noticing this issue, and reporting it here! (JavaScript scope resolution is hard!)

    I will move this ticket back to "Needs work" so that we fix it before the next release.

  • Assigned to Bobik
  • Comment 5 months ago →
    🇺🇦Ukraine Bobik

    I'll review and correct these remarks and correct as soon as possible

  • Merge request !7Issue #3407697: Fix eslint in 2.x branch (follow-up) → (Merged) created by Unnamed author
  • Pipeline finished with Success
    5 months ago
    #82738
  • Pipeline finished with Success
    5 months ago
    #82744
  • Issue was unassigned.
  • Status changed to Needs review 5 months ago
  • Comment 5 months ago →
    🇺🇦Ukraine Bobik

    HI @mparker17 @rgpublic
    I have reviewed your remarks and agree with them. I haven't checked whether the data is saved after the form is saved. I fixed it and tested everything works as expected.
    @mparker17
    No CTA only IFU
    I found that the jQuery.fn.removeOption() function is never called, so it probably needs to be removed, I suggest creating a separate issue for this for a deeper investigation.

  • Comment 3 months ago →
    System Message
  • Status changed to Fixed 3 months ago
  • Comment 3 months ago →
    🇨🇦Canada mparker17 UTC-4

    The follow-up has been merged; thanks @rgpublic and @bobi-mel!

  • Comment 3 months ago →
    System Message

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

contrib.social Blog FAQ Discussions
Production build 0.69.0 2024