Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.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-val, which targets the jQuery val function.
Steps to reproduce
Proposed resolution
Remaining tasks
- In
core/.eslintrc.jquery.jsonChange"jquery/no-val": 0,to"jquery/no-val": 2,to enable eslint checks for uses of jQuery.val(). 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
.val()to use Vanilla (native) JavaScript instead.
User interface changes
API changes
Data model changes
Release notes snippet
| Comment | File | Size | Author |
|---|---|---|---|
| #23 | 3239134-23-d10.patch | 98.23 KB | hooroomoo |
| |||
Issue fork drupal-3239134
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:
- 3239134-refactor-if-feasible
compare - 3239134-jquery-val changes, plain diff MR !1269
1 hidden branch
Comments
Comment #5
hooroomooComment #6
hooroomooset back to active due to failed tests
Comment #8
nod_Few things in the MR.
Generally we'll need to be careful because
$(element).val()works even when element is undefined, this is not the case with dom elements. if the element is empty there is no .value property and things crash. Let's see what the tests says.Comment #9
nod_@yogeshmpawar please make sure not to undo another commit during a rebase. You removed part of #3239132: Refactor (if feasible) uses of the jQuery trim function to use vanillaJS in the process :)
Comment #10
yogeshmpawarThanks @nod_ for pointing this. actually while rebasing I have compared this issue code with this drupal core - https://git.drupalcode.org/issue/drupal-3239134/-/blob/9.3.x/core/.eslin... didn't recognise that this is not correct drupal core to compare. will correct it now.
Comment #11
hooroomooComment #12
larowlanLeft a couple of comments on the MR where I think we're replacing the
val(something)setter with[0].valuewhere assuming there is only one match may not be valid - e.g. class based selectorsComment #14
bnjmnmDown to fewer than 10 threads for this gigantic issue, which is incredible progress. I updated those with as helpful feedback as possible.
Comment #15
hooroomooLooked through all usages of nodelist.forEach and added it to the libraries that didn't have it.
Comment #16
bnjmnmI provided a comment for every change made in the 100+ JS files impacted in this issue. Even if there were no requested changes, I documented that the change was reviewed and looks good, usually explaining why it's a safe change that accounts for the differences between jQuery and vanilla (namely jQUery automatically acting on multiple elements in a query, and how it casually reacts to calling a function on empty results). This is too large an MR to assume no-comment === no-problem.
I've run out of nits to pick and I'm making this big chungus RTBC.
Comment #17
nod_Went through all the code & tested most changes, if I didn't comment I didn't find anything wrong with it :)
bunch of small things, 2 or 3 actual bugs/problems found. Which is great given the size of the MR :)
Comment #18
hooroomooComment #19
nod_almost there :)
Comment #20
hooroomooComment #21
nod_Changes looks good, simply removed an unnecessary eslint comment.
All good from me!
Comment #22
lauriiiReviewed the MR in detail and it looks good! Only problem is that to be able to commit this, we need a Drupal 10 version of the MR.
Comment #23
hooroomood10 patch uploaded
Comment #24
nod_10.x patch is all good.
Comment #28
lauriiiCommitted 6808108 and pushed to 10.0.x. Also committed the 9.4.x MR. Thanks!
Comment #29
nod_Little follow-up in #3259381: Follow-up for jQuery val replacement