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.json Change "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 running yarn lint:core-js-passing from the core directory
  • Add the following lines to core/scripts/dev/commit-code-check.sh so 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 remove

    Add 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

Issue fork drupal-3239531

Command icon 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

Theresa.Grannum created an issue. See original summary.

Theresa.Grannum’s picture

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

ahsannazir’s picture

Assigned: Unassigned » ahsannazir
Status: Active » Needs work

ahsannazir’s picture

StatusFileSize
new346.04 KB

Can someone look into the MR and confirm why Tests Pipelines are failing. Seems like few test cases are failing but not sure why

ahsannazir’s picture

Assigned: ahsannazir » Unassigned

shweta__sharma’s picture

Status: Needs work » Needs review

Changes seems fine in the MR

smustgrave’s picture

smustgrave’s picture

Status: Needs review » Needs work

But moving to NW for the eslint rule change.

ahsannazir changed the visibility of the branch 3239531-refactor-closest to hidden.

ahsannazir’s picture

Status: Needs work » Needs review

smustgrave changed the visibility of the branch 3239531-refactor-if-feasible to hidden.

smustgrave’s picture

Status: Needs review » Needs work

Left a comment.

ahsannazir’s picture

Status: Needs work » Needs review
needs-review-queue-bot’s picture

Status: Needs review » Needs work
StatusFileSize
new90 bytes

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

ahsannazir’s picture

Status: Needs work » Needs review
nod_’s picture

Status: Needs review » Needs work

havent' finished but it'll need more work

shubh_’s picture

Status: Needs work » Needs review
needs-review-queue-bot’s picture

Status: Needs review » Needs work
StatusFileSize
new90 bytes

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

nod_’s picture

Status: Needs work » Needs review
smustgrave’s picture

Status: Needs review » Needs work

Appears to have javascript test failures.

Tom Konda made their first commit to this issue’s fork.

Utkarsh_33 made their first commit to this issue’s fork.

utkarsh_33’s picture

Status: Needs work » Needs review

Fixed tests.

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

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

nod_’s picture

Status: Reviewed & tested by the community » Closed (won't fix)

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:

  • This MR is big, it impact a very big number of subsystems and make the code more brittle. jQuery is good at dealing with undefined elements, empty sets and so on, the DOM isn't. We already had regressions from a previous patch with undefined elements
  • The MR is too big to review and make sure there are no regressions (even with the tests we already have), it would create unstability that we don't have to endure
  • Sometimes the jQuery code is simply more readable, I do not think this change is a net positive:
          $(once('filter-guidelines', '.js-filter-guidelines', context))
            .find(':header')
            .hide()
            .closest('.js-filter-wrapper')
            .find('select.js-filter-list')
            .on('change.filterGuidelines', updateFilterGuidelines)
            // Need to trigger the namespaced event to avoid triggering formUpdated
            // when initializing the select.
            .trigger('change.filterGuidelines');
          // Define the closest function for finding the closest ancestor with a given selector
          function closest(element, selector) {
            while (element && !element.matches(selector)) {
              element = element.parentNode;
            }
            return element;
          }
    
          const contextElement = document.querySelector('.js-filter-guidelines');
          const closestWrapper = closest(contextElement, '.js-filter-wrapper');
          if (closestWrapper) {
            const $context = $(contextElement);
            const selectElement = $(closestWrapper).find('select.js-filter-list');
            $context.find(':header').hide();
            selectElement
              .on('change.filterGuidelines', updateFilterGuidelines)
              // Need to trigger the namespaced event to avoid triggering formUpdated
              // when initializing the select.
              .trigger('change.filterGuidelines');
          }
    

    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.