Problem/Motivation

Inspired by Github's approach to removing jQuery from the front-end, add the eslint jquery plugin to Drupal core's eslint config. This will help identify and prevent new jQuery usages and make it easier to inventory existing ones.

Steps to reproduce

Proposed resolution

  • Install the eslint jquery plugin that Github used for their jQuery removal efforts.
  • Add rule-specific /* eslint-disable */ declarations to files still using jQuery, so they can still pass tests
  • Determine if additional eslint checks are needed beyond those provided by the eslint jquery plugin, and add those as well.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

In order to limit Drupal core's dependency on jQuery for forward-compatibility with future versions, the eslint core-js-passing config now includes the eslint-plugin-jquery plugin.

Currently, only a small number of the available rules in eslint-plugin-jquery are enabled; these are the rules that check for jQuery features not used by core. As core eliminates uses of a given jQuery feature (typically because modern ES6 JavaScript has native replacements), additional eslint-plugin-jquery rules can be enabled. This will prevent those jQuery features from being reintroduced.

Issue fork drupal-3191023

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:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

bnjmnm created an issue. See original summary.

bnjmnm’s picture

This adds the eslint plugin. The /* eslint-disable */ declarations, needed for tests to pass, are applied to entire files. There are pros and cons of adding them to individual usages instead:
Pros of adding to individual usages

  • Easier to prevent new jQuery usages to get added
  • A more accurate assessment of the jQuery usage in core

Cons of adding to individual usages

  • More difficult to see jQuery usage in a file at a glance
  • Potentially nontrivial increase in JS filesize (albeit in files that aren't loaded by the browser)
  • Some files would become very difficult to read as they make heavy use of jQuery
  • Countless issues-in-progress would be disrupted by potentially difficult rerolls.
  • The advantages of /* eslint-disable */ declarations could still be enjoyed at a later date. If this is done after first taking care of frequently-used/easily-addressed jQuery usages, most of the cons listed above are less of a concern.

So my thought is to do it on the file-level now and move to individual instances once overall jQuery use is reduced.

nod_’s picture

That looks great :D

I'm not a fan of eslint disable in each file. we don't work file by file anymore, so we're going to fix 1 type of problem at a time. I would add the settings to legacy-passing eslint conf and disable all necessary rules instead. That way we can have an issue to get rid of the find usage or sizzle selectors, by deleting one line in the eslint config.

bnjmnm’s picture

Status: Active » Needs review
FileSize
3.04 KB

This patch changes the approach to what was suggested in #3. No interdiff as that would just be noise.

Although the approach in #2 won't be used, it provided me with a way to see how many files use a given piece of jQuery functionality. A table with that information can be found here: #3179551: Provide no-library equivalents of common/useful jQuery functions.

lauriii’s picture

Should we have another config that enables all applicable rules for statistics in the .eslintrc.json?

nod_’s picture

We already have 3 different eslint config, not in favor of adding another one, we could get rid of eslint.legacy.json though

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

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

nod_’s picture

Status: Needs review » Reviewed & tested by the community

Adding another eslintrc file seems fine, after all it's under the core/ directory. And it's better to have it than not have it.

Would make tracking down jquery usage very simple (sizzle selectors especially) so +1 on this.

Still applies.

xjm’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record, +Needs release note

Love it!

Can we add a change record explaining this change? It's also technically a new coding standard so gets a release note mention.

bnjmnm’s picture

Issue summary: View changes
bnjmnm’s picture

Issue summary: View changes
bnjmnm’s picture

Issue summary: View changes

bnjmnm’s picture

Status: Needs work » Needs review
bnjmnm’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs change record, -Needs release note

Switching back to RTBC since the switch away from RTBC earlier was for RN/CR and those have been added + no code has changed since then.

xjm’s picture

Issue summary: View changes

Thanks @bnjmnm!

Tweaked the release note a little to help explain why we want less jQuery, and added the same adjustments to the CR.

xjm’s picture

Hiding patch files since there is a more recent MR.

  • xjm committed 4855e96 on 9.3.x
    Issue #3191023 by bnjmnm, nod_: Add eslint rules to check for jQuery...
xjm’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +9.3.0 release notes

Tested this out by adding:

diff --git a/core/misc/autocomplete.es6.js b/core/misc/autocomplete.es6.js
index c747986d14..6efd3969f0 100644
--- a/core/misc/autocomplete.es6.js
+++ b/core/misc/autocomplete.es6.js
@@ -41,6 +41,8 @@
       result.push($.trim(current));
     }
 
+    $.globalEval("console.log('Hi')");
+
     return result;
   }

Then:

[mandelbrot:core | Mon 19:49:59] $ yarn run build:js -- --file misc/autocomplete.es6.js 
yarn run v1.22.4
warning From Yarn 1.0 onwards, scripts don't require "--" for options to be forwarded. In a future version, any explicit "--" will be forwarded as-is to the scripts.
$ cross-env BABEL_ENV=legacy node ./scripts/js/babel-es6-build.js --file misc/autocomplete.es6.js
[19:50:22] 'misc/autocomplete.es6.js' is being processed.
[19:50:23] 'misc/autocomplete.es6.js' is finished.
✨  Done in 1.68s.

Then attempted to commit that and got:

Checking core/misc/autocomplete.es6.js

yarn run v1.22.4
$ cross-env BABEL_ENV=legacy node ./scripts/js/babel-es6-build.js --check --file /Users/xjm/git/maintainer/core/misc/autocomplete.es6.js
[19:50:41] '/Users/xjm/git/maintainer/core/misc/autocomplete.es6.js' is being checked.
✨  Done in 0.77s.

/Users/xjm/git/maintainer/core/misc/autocomplete.es6.js
  44:5  error  $.globalEval is not allowed  jquery/no-global-eval

✖ 1 problem (1 error, 0 warnings)

Works great!

Committed to 9.3.x and published the CR. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.