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

Steps to reproduce

Proposed resolution

Remaining tasks

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

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#15 D94x-partial_revert-3239132-15.patch896 bytesjoseph.olstad

Issue fork drupal-3239132

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 !1245

hooroomoo’s picture

hooroomoo
they/them
Location nyc🗽
commented
Status: Active » Needs review
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

Just one question, otherwise this looks good to go

nod_’s picture

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

Another duplicate trim to remove, otherwise it looks good :)

hooroomoo’s picture

hooroomoo
they/them
Location nyc🗽
commented
Status: Needs work » Needs review

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

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented
Status: Needs review » Reviewed & tested by the community

All of the feedback from @nod_ and @larowlan has been addressed. I'm pleased that this was used as an opportunity to clean up some unnecessary uses as opposed to making replacements for something that was unnecessary to begin with. Those reviews were quite thorough, but I did an additional check of each trim() change to confirm is was the correct approach and everything looks good!

nod_’s picture

nod_
Primary language French
Location Lille
commented

RTBC+1 by the way :)

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

Crediting nod_ and myself for reviews and bnjmnm for mentoring and review

  • larowlan committed 25c55e3 on 9.3.x authored by hooroomoo 
    Issue #3239132 by hooroomoo, larowlan, nod_, bnjmnm: Refactor (if...
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Reviewed & tested by the community » Fixed

Fixed, thanks!

Status: Fixed » Closed (fixed)

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

joseph.olstad’s picture

joseph.olstad
Primary language French
commented

This change causes a javascript error as follows on the node/edit and node/add forms, possibly elsewhere:
Uncaught TypeError: callback is not a function drupalGetSummary https://localhost/core/misc/form.js?v=9.3.15:11

Solution I found is to revert this:

diff --git a/core/misc/form.js b/core/misc/form.js
index 4064c1ddf6..bc6f0524b1 100644
--- a/core/misc/form.js
+++ b/core/misc/form.js
@@ -8,7 +8,7 @@
 (function ($, Drupal, debounce) {
   $.fn.drupalGetSummary = function () {
     var callback = this.data('summaryCallback');
-    return this[0] && callback ? $.trim(callback(this[0])) : '';
+    return this[0] && callback ? callback(this[0]).trim() : '';
   };
 
   $.fn.drupalSetSummary = function (callback) {

so change it back to :
return this[0] && callback ? $.trim(callback(this[0])) : '';

when I did this, the error went away.

joseph.olstad’s picture

joseph.olstad
Primary language French
commented
StatusFileSize
new D94x-partial_revert-3239132-15.patch896 bytes

patch

joseph.olstad’s picture

joseph.olstad
Primary language French
commented

#3291587: Regression fix for (if feasible) uses of the jQuery trim function to use vanillaJS

joseph.olstad’s picture

joseph.olstad
Primary language French
commented

I'm still seeing this regression in D10.0.10
#3291587: Regression fix for (if feasible) uses of the jQuery trim function to use vanillaJS

joseph.olstad’s picture

joseph.olstad
Primary language French
commented

ok I have the steps to reproduce the regression introduced by the commit made in comment #3239132-11: Refactor (if feasible) uses of the jQuery trim function to use vanillaJS, it can be reproduced with core alone.

in your content type, allow the "Provide a menu link" option provided by the core menu_ui module.

When editing content that does not have the checkbox checked for "Provide a menu link" the callback is not a valid callback, it is "Not in menu"

The "Not in menu" functionality comes from the core menu_ui module.

The regression is in core, only need core to reproduce, it's not even caused by contrib. exposed by contrib module entity_translation_unified_form doing something a bit unconventional but necessary.

Patch 18 works

the $.trim() function in jQuery does more than the ES6 trim() function and it's encapsulated by jQuery.

Basically the es6 trim function is not as good as the jQuery $.trim() function.

I've spent a significant amount of time investigating ways to get this working with es6 without jQuery however the approach by jQuery is radically different because the trim function takes a parameter of item we want to trim.

Not only is the approach more sophisticated with jQuery (different) it also trims more than just whitespace, it also trims tab and newline characters whereas the es6 trim function only handles whitespace.

any chained functions I tried just didn't seem to cut the mustard. The encapsulated approach by jQuery is more advanced and I haven't had a chance to figure it out.

joseph.olstad’s picture

joseph.olstad
Primary language French
commented

Fixed in
#3291587: Regression fix for (if feasible) uses of the jQuery trim function to use vanillaJS