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-prop, which targets the jQuery prop function.

Steps to reproduce

In core/.eslintrc.jquery.json Change "jquery/no-prop": 0, to "jquery/no-prop": 2, to enable eslint checks for uses of jQuery prop(). 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.

Proposed resolution

Replace usage of jQuery.prop with vanilla js alternatives.

Remaining tasks

  • Review MR !4266
  • Close MR !1252

User interface changes

N/A

API changes

N/A

Data model changes

N/A

Release notes snippet

N/A

CommentFileSizeAuthor
#26 3238882-nr-bot.txt90 bytesneeds-review-queue-bot
#18 3238882-nr-bot.txt4.87 KBneeds-review-queue-bot
#11 3238882-nr-bot.txt90 bytesneeds-review-queue-bot

Issue fork drupal-3238882

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:

About issue forks

Comments

hooroomoo created an issue. See original summary.

hooroomoo opened merge request !1252

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.

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

mstrelan opened merge request !4266

mstrelan’s picture

mstrelan commented
Issue summary: View changes
Status: Active » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Refactor LGTM. Applied locally and didn't notice anything that broke.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new 3238882-nr-bot.txt90 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 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.

mstrelan’s picture

mstrelan commented
Status: Needs work » Reviewed & tested by the community

Back to RTBC as per #10.

quietone’s picture

quietone commented
Status: Reviewed & tested by the community » Needs work

I'm triaging RTBC issues. I read the IS which has no proposed resolution (although it is obvious by the title) but has remaining tasks.

I followed the steps in the first item of the remaining tasks and found that there are ✖ 42 problems (42 errors, 0 warnings) . Based on that, this is not fixing all the usages of prop. I looked at the Meta to see if this is part of a set of issues to fix prop but found that this is the only issue. I presume then, that this is to fix all usages?

Setting to needs work for an issue summary update and for the work on the other instances of prop in core. Although, if there is some reason they cannot be changed, then that should be documented in the IS.

Thanks

mstrelan’s picture

mstrelan commented
Issue summary: View changes
Status: Needs work » Needs review

@quietone which MR did you check? Should be all fixed up in !4266. We should close !1252 as it's against 9.3.x when we still compiled es6 files.

I've updated the issue summary to clarify the status.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Restoring status.

Verified locally MR 4266 still applies cleanly to 11.x

quietone closed merge request !1252

quietone’s picture

quietone commented

@mstrelan, I don't recall which MR I used. However, I repeated the steps with !4266 and didn't get any errors. I also closed the other MR. Thanks for the help.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new 3238882-nr-bot.txt4.87 KB

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch 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.

mstrelan’s picture

mstrelan commented
Status: Needs work » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Rebase seems good, reran nightwatch but was random failure.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Reviewed & tested by the community » Needs work

Added some comments/suggestions to the MR.

ahsannazir’s picture

ahsannazir commented

Will work on suggestions

ahsannazir’s picture

ahsannazir commented
Status: Needs work » Needs review
nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Needs review » Needs work

There are a couple of things to fix. I'm worried about some of the replacement that could lead to regression (aria-pressed, autofocus)

Having a bunch of .each all over the place is not ideal but there are no better way to deal with this.

shubh_’s picture

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

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3238882-nr-bot.txt90 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.

shubh_’s picture

shubh_ commented
Status: Needs work » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Re-ran javascript test failure twice and it failed both. So seems to be related to the change.

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

utkarsh_33’s picture

utkarsh_33 commented
Status: Needs work » Needs review

Fixed the test.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Left some comments on the MR.

@nod_ I tagged you in one if there's a change we want to keep?

Saved credit for ahsannazir and Utkarsh_33 for keeping this moving.

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Needs work » 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
  • Every time I review this MR, I find a new bug
  • Sometimes the jQuery code is simply more readable, I do not think this change is a net positive:
        const editSubmit = $(
      '.field-config-edit-form [data-drupal-selector="edit-submit"]',
    )[0];
    if (editSubmit) {
      editSubmit.disabled = true;
    }

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.