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-sizzle, which targets the selector extensions provided by Sizzle .
Steps to reproduce
Proposed resolution
Remaining tasks
- In
core/.eslintrc.jquery.jsonChange"jquery/no-sizzle": 0,to"jquery/no-sizzle": 2,to enable eslint checks for uses of Sizzle. 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 Sizzle to use Vanilla (native) JavaScript instead.
User interface changes
API changes
Data model changes
Release notes snippet
| Comment | File | Size | Author |
|---|
Issue fork drupal-3239042
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:
Comments
Comment #2
nod_I think we'll need to split this one into several tasks, see related issues :)
Comment #5
xjmThis is at least major based on info from @nod_.
Comment #9
Sam Phillemon commentedComment #11
Sam Phillemon commentedI am facing issues with the PHP unit tests. I am new to this and I have fixed some of them but I am unable to fix the remaining. Can someone please help with it?
Comment #12
guptahemant commentedComment #13
guptahemant commentedComment #14
Sam Phillemon commentedComment #15
needs-review-queue-bot commentedThe 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.
Comment #17
omkar-pd commentedComment #18
needs-review-queue-bot commentedThe 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.
Comment #19
omkar-pd commentedComment #20
needs-review-queue-bot commentedThe 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.
Comment #21
shubh_ commentedComment #22
needs-review-queue-bot commentedThe 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.
Comment #23
Sam Phillemon commentedComment #24
needs-review-queue-bot commentedThe 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.
Comment #25
Sam Phillemon commentedComment #26
nod_Thanks for the work!
:visible will be tricky to get right so I'd rather leave that in for now, we can always fix it in a follow-up. In some places there is a bit too much of code rewrite, if we just need to update the CSS selector it's better to just do that and not try to replace addClass and that kind of things.
Comment #27
Sam Phillemon commentedSo I have made the suggested changes mentioned and also kept the ':visible' as it is for now.
Comment #29
shubh_ commentedlooks like some javascript test failure
Comment #32
yash.rode commentedComment #33
yash.rode commentedHi, I also tried providing the
{"allowOther": true}option tono-sizzle. But couldn't find any way to do so. The other way I can think of doing this is -- adding inline comment to Eslint disable for the lines which uses:visibleand get rid of those once we figure out a solution for:visible?Comment #35
nod_That's my bad, the eslint configuration change is not possible I was looking at the wrong js package for documentation.
Comment #44
smustgrave commentedClosed as dup #1751388: Selectors clean-up: filter module