Refactor (if feasible) uses of the jQuery data function to use Vanilla/native

Created on 27 September 2021, about 3 years ago
Updated 5 July 2024, 6 months ago

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

Steps to reproduce

Proposed resolution

Remaining tasks

See #9 for questions about direction.

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

User interface changes

API changes

Data model changes

Release notes snippet

📌 Task
Status

Needs work

Version

11.0 🔥

Component
Javascript 

Last updated 3 days ago

Created by

🇺🇸United States Theresa.Grannum Boston

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

Merge Requests

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • First commit to issue fork.
  • Status changed to Needs work over 1 year ago
  • 🇦🇺Australia mstrelan

    Might need some guidance from the big brains on this one. jQuery's data function has two purposes, one for getting and setting data attributes on elements and another for getting and setting arbitrary bits of data in an internal cache for an element. It seems the former can be addressed using the dataset property (as per ajax.js) whereas the latter may can use a WeakMap object (as per tabbingmanager.js). Is this the right approach?

  • 🇳🇿New Zealand quietone

    Since this needs some input on direction, I am adding the 'Needs architectural review' tag and changing to NR. Added and item to the remaining tasks to get answers to those questions.

  • 🇮🇳India shubh511

    checking this issue..

  • Merge request !69353239535-Refactored-no-data → (Open) created by shubh511
  • Pipeline finished with Failed
    10 months ago
    Total: 356s
    #112610
  • Pipeline finished with Failed
    10 months ago
    #112616
  • Pipeline finished with Failed
    10 months ago
    #112643
  • 🇫🇷France nod_ Lille

    To reply to #9 Let's first make the changes for the getting/setting data attributes using dataset, let's leave the second use case alone (like vertical tabs) because it'll need something else than a straight replacement of the jquery call (probably some new architecture for these things).

    @shubh511 I think you're still working on it but please make sure to test the code before setting this to Needs review, as-is the code is not going to work, jquery doesn't have a setAttribute function. Also with data attributes let's use element.dataset.someThing = 'value'; instead of element.setAttribute('data-some-thing', 'value');

  • 🇮🇳India shubh511

    raised a MR for this issue but the problem is that it is failing pipeline and when fixing one test case will result in failing another test case and viceversa..

  • Assigned to kostyashupenko
  • 🇷🇺Russia kostyashupenko Omsk

    Gonna kill this task

  • 🇷🇺Russia kostyashupenko Omsk

    Still on it

  • Pipeline finished with Failed
    9 months ago
    Total: 1161s
    #129203
  • Pipeline finished with Failed
    9 months ago
    Total: 172s
    #129215
  • Pipeline finished with Failed
    9 months ago
    Total: 183s
    #129217
  • Issue was unassigned.
  • 🇷🇺Russia kostyashupenko Omsk

    There are still several existing data() methods:

    % yarn lint:core-js-passing
    
    core/misc/ajax.js
      1104:17  error  Prefer WeakMap to $.data  jquery/no-data
      1111:31  error  Prefer WeakMap to $.data  jquery/no-data
      1580:7   error  Prefer WeakMap to $.data  jquery/no-data
    
    core/misc/autocomplete.js
      224:13  error  Prefer WeakMap to $.data  jquery/no-data
    
    core/misc/dialog/dialog.ajax.js
       59:31  error  Prefer WeakMap to $.data  jquery/no-data
       60:13  error  Prefer WeakMap to $.data  jquery/no-data
      105:24  error  Prefer WeakMap to $.data  jquery/no-data
    
    core/misc/tabbingmanager.js
      297:25  error  Prefer WeakMap to $.data        jquery/no-data
      302:9   error  Prefer WeakMap to $.data        jquery/no-data
      314:25  error  Prefer WeakMap to $.data        jquery/no-data
      332:13  error  Prefer WeakMap to $.removeData  jquery/no-data
      340:13  error  Prefer WeakMap to $.data        jquery/no-data
    
    ✖ 12 problems (12 errors, 0 warnings)
    

    1. regarding ajax.js and 3 remaining errors - i think all 3 errors can be fixed, but need to research (i just don't have available time anymore for this, so skipped)

    2. regarding autocomplete and dialog - there are some data methods which are still exist, which are related to the plugins itself (jquery autocomplete, jquery dialog). I didn't touch those 2 files at all, i think the better way is to wait for replacement of jquery autocomplete in core, and replacement of jquery dialog to native dialog element functionality.

    3. regarding tabbingmanager - i didn't understand how to reproduce it in Drupal for making local tests. From what i saw in tabbingmanager.js - this file is fixable in terms of replacing jquery data() method

    4. Also i prefer to use getAttribute() instead of dataset, coz getAttribute works seems at least 2x faster https://www.measurethat.net/Benchmarks/Show/14432/0/getattribute-vs-dataset

  • Sam Phillemon made their first commit to this issue’s fork.

  • Pipeline finished with Canceled
    8 months ago
    Total: 102s
    #149851
  • Pipeline finished with Failed
    8 months ago
    Total: 194s
    #149857
  • First commit to issue fork.
  • Status changed to Needs review 8 months ago
  • 🇮🇳India gauravvvv Delhi, India
  • Pipeline finished with Failed
    8 months ago
    Total: 194s
    #156011
  • Status changed to Needs work 8 months ago
  • 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 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.

  • First commit to issue fork.
  • Pipeline finished with Failed
    6 months ago
    Total: 562s
    #203442
  • Pipeline finished with Failed
    6 months ago
    Total: 633s
    #203451
  • Pipeline finished with Failed
    6 months ago
    Total: 167s
    #210506
  • Pipeline finished with Failed
    6 months ago
    Total: 512s
    #210521
  • Pipeline finished with Failed
    6 months ago
    Total: 527s
    #213717
  • Status changed to Needs review 6 months ago
  • Status changed to Needs work 6 months ago
  • 🇺🇸United States smustgrave

    Trying to keep up with these but seems the javascript tests are failing repeatedly (re-ran 3 times)

  • Pipeline finished with Failed
    6 months ago
    Total: 192s
    #218490
  • Pipeline finished with Failed
    6 months ago
    Total: 178s
    #218493
  • Pipeline finished with Failed
    6 months ago
    Total: 538s
    #218530
Production build 0.71.5 2024