Problem/Motivation
As mentioned in the parent issue #3238306: [META] Where possible, refactor existing jQuery uses to vanillaJS to reduce jQuery footprint, we are working towards reducing our jQuery footprint. One of the ways to accomplish this is to reduce the number of jQuery features used in Drupal core. We have added eslint rules that identify specific features and fail tests when those features are in use.
There are (or will be) individual issues for each jQuery-use eslint rule. This one is specific to jquery/no-closest, which targets the jQuery closest function.
Steps to reproduce
Proposed resolution
Remaining tasks
- In
core/.eslintrc.jquery.jsonChange"jquery/no-closest": 0,to"jquery/no-closest": 2,to enable eslint checks for uses of jQuery.closest(). With this change, you'll be able to see uses of the undesirable jQuery feature by runningyarn lint:core-js-passingfrom thecoredirectory - Add the following lines to
core/scripts/dev/commit-code-check.shso the DrupalCI testing script can catch this jQuery usage on all files, not just those which have changed
# @todo Remove the next chunk of lines before committing. This script only lints # JavaScript files that have changed, so we add this to check all files for # jQuery-specific lint errors. cd "$TOP_LEVEL/core" node ./node_modules/eslint/bin/eslint.js --quiet --config=.eslintrc.passing.json . CORRECTJQS=$? if [ "$CORRECTJQS" -ne "0" ]; then # No need to write any output the node command will do this for us. printf "${red}FAILURE ${reset}: unsupported jQuery usage. See errors above." STATUS=1 FINAL_STATUS=1 fi cd $TOP_LEVEL # @todo end lines to removeAdd the block about 10 lines before the end of the file, just before
if [[ "$FINAL_STATUS" == "1" ]] && [[ "$DRUPALCI" == "1" ]]; then, then remove it once all the jQuery uses have been refactored. - If it's determined to be feasible, refactor those uses of jQuery
.closest()to use Vanilla (native) JavaScript instead.
User interface changes
API changes
Data model changes
Release notes snippet
| Comment | File | Size | Author |
|---|---|---|---|
| #25 | 3239531-nr-bot.txt | 90 bytes | needs-review-queue-bot |
| #21 | 3239531-nr-bot.txt | 90 bytes | needs-review-queue-bot |
| #10 | Pipelines.png | 346.04 KB | ahsannazir |
Issue fork drupal-3239531
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #3
Theresa.GrannumComment #8
ahsannazir commentedComment #10
ahsannazir commentedCan someone look into the MR and confirm why Tests Pipelines are failing. Seems like few test cases are failing but not sure why
Comment #11
ahsannazir commentedComment #13
shweta__sharma commentedChanges seems fine in the MR
Comment #14
smustgrave commentedComment #15
smustgrave commentedBut moving to NW for the eslint rule change.
Comment #17
ahsannazir commentedComment #19
smustgrave commentedLeft a comment.
Comment #20
ahsannazir commentedComment #21
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #22
ahsannazir commentedComment #23
nod_havent' finished but it'll need more work
Comment #24
shubh_ commentedComment #25
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #26
nod_Comment #27
smustgrave commentedAppears to have javascript test failures.
Comment #30
utkarsh_33 commentedFixed tests.
Comment #31
smustgrave commentedStill needs subsystem thumbs up believe
But applied the MR and did some light testing of checking random features (views, layout builder popups, etc) and didn't notice anything.
The 2 threads appear to be addressed (I believe)
So to keep from stalling going to mark as tests are also showing fine.
Comment #32
nod_First of all thank you all for the hard work on this one, I've had to push a number of patches like this and I know how hard it is to keep up.
I'm going to try and refocus the jQuery removal work and to do that I need to take a few decisions. I'm going to close this issue for a few reasons:
There is one line to change and we have to make many, many other changes for this to work
I ported the credits to #3464044: Credit for work on the reduce jQuery issues, which I will mark as fixed as soon as I go through all the others impacted jQuery issues.