Problem/Motivation

We can now add selectors using jQuery: https://api.jquery.com/category/selectors/jquery-selector-extensions/

At the same time, jQuery itself recommends optimizing them:

https://learn.jquery.com/performance/optimize-selectors/

Therefore, an important step is to use only vanilla selectors and deprecate the Sizzle style.

Proposed resolution

Similar to
https://git.drupalcode.org/project/drupal/-/blob/11.x/core/misc/dialog/d...

We'll add a temporary function that will still work but throw messages if the selector is invalid.
https://developer.mozilla.org/en-US/docs/Web/API/Document/querySelector#...

CommentFileSizeAuthor
#44 3568777-nr-bot_pqzuh4j9.txt91 bytesneeds-review-queue-bot
#26 3568777-nr-bot_crs6jb_c.txt91 bytesneeds-review-queue-bot
#24 3568777-nr-bot_pn7bd_a8.txt91 bytesneeds-review-queue-bot

Issue fork drupal-3568777

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

finnsky created an issue. See original summary.

finnsky opened merge request !14440

finnsky changed the visibility of the branch 3568777-deprecate-sizzle-jquery to hidden.

finnsky’s picture

finnsky commented
Status: Active » Needs review
nicxvan’s picture

nicxvan commented
Status: Needs review » Needs work

There are a bunch of failures still.

How do we know which ones are supposed to be deprecated and are there any that could flow through this code path that should not be deprecated?

finnsky’s picture

finnsky commented
Status: Needs work » Needs review
finnsky’s picture

finnsky commented
Issue summary: View changes
finnsky’s picture

finnsky commented

Rebased

I don't think there's any point in constantly fixing the Asset Size Test. I'm leaving the task in Needs Review even with a failed test. This test can be fixed before committing.

finnsky’s picture

finnsky commented

Fixed Asset Size Test

smustgrave’s picture

smustgrave commented

Wondering if this should be pushed to 13, we found out removing existing deprecation from ajax.js is breaking things so may wan to be safe.

finnsky’s picture

finnsky commented

Looking at the review dynamics, you can safely postpone it until 15 :)

smustgrave’s picture

smustgrave commented

Why 15?

finnsky’s picture

finnsky commented

Sorry! Sad joke.

I have several similar MRs. And they're being reviewed very slowly.

They're not really needed right now, but without them, it seems like it would be difficult to complete, say, a replacement for the jQuery UI Dialog.

nicxvan’s picture

nicxvan commented

That can certainly be frustrating!

I read the attached docs and https://www.scaler.com/topics/jquery/sizzle-selector/

And with the refactor I understand what is happening here well enough to review properly now I think.

I have one more comment, I think we can return in the try and fall back to jquery in the catch after setting the deprecation message.

That way we don't duplicate the selector.

Also there is a performance test conflict, once those two issues are resolved I think this is ready.

finnsky’s picture

finnsky commented

I added a comment and removed the unnecessary function.
We should return the same expected jQuery selectors. We'll just make sure the syntax is no longer sizzle.
In version 12, this will allow us to switch to vanilla ones.

finnsky’s picture

finnsky commented

@nicxvan

This is context of current idea
https://git.drupalcode.org/project/drupal/-/merge_requests/8751#note_387911

smustgrave’s picture

smustgrave commented

@nicxvan thoughts? I don't think I'm a good resource for javascript review. Also the current javascript deprecation removals we are doing is breaking so much.

nicxvan’s picture

nicxvan commented

This needs a rebase, but I think this is ready once that is done

Sorry I missed 16, but that answers my last question.

Just a note, I'm not super strong with JS either, but this seems useful.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Thanks @nicxvan

finnsky’s picture

finnsky commented
Status: Needs work » Needs review
nicxvan’s picture

nicxvan commented

I'm so sorry, I think this needs a change record with a link to it in the notice right?

I created a stub cr but I think you might need to add examples.

One other thing I realized, we should create a follow up issue with some notes about how to clean this up since it's a bit out of the ordinary e.g delete deprecateSizzleSelector and probably remove jquery from those calls.

Ping me in slack when this is ready for review again.

finnsky’s picture

finnsky commented
Related issues: +#3582096: Remove Sizzle selector compatibility helper and jQuery selector fallback in Drupal 12
finnsky’s picture

finnsky commented

Added follow up https://www.drupal.org/project/drupal/issues/3582096
and updated CR.

Please review!

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3568777-nr-bot_pn7bd_a8.txt91 bytes

The 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.

finnsky’s picture

finnsky commented
Status: Needs work » Needs review
needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3568777-nr-bot_crs6jb_c.txt91 bytes

The 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.

finnsky’s picture

finnsky commented
Status: Needs work » Needs review
nicxvan’s picture

nicxvan commented

I reviewed this again, I think it's ready, but there is another performance test conflict.

prudloff’s picture

prudloff commented

Switching entirely to document.querySelector() would also improve security because $() is an XSS sink (if the selector starts with < it parses it as HTML...).

nicxvan’s picture

nicxvan commented
Status: Needs review » Reviewed & tested by the community

Ok the conflict was resolved, I think this is ready now!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

We should have a test that proves this is working and can be caught by our test infra.

finnsky’s picture

finnsky commented
Status: Needs work » Closed (won't fix)

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

nicxvan’s picture

nicxvan commented
Status: Closed (won't fix) » Needs work

Yeah I should have caught that it didn't have an actual test.

finnsky’s picture

finnsky commented
Status: Needs work » Needs review

It was probably a good idea to write a test.
Because with it, I caught a couple of syntax errors in other tests.

nicxvan’s picture

nicxvan commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

Thanks for adding that, I should have noticed it was missing earlier!

It looks great to me!

I can't run test only, but this is a feature request not a bug fix so that will fail anyway since it's a new code path.

This looks ready to me!

Read through the CR again and it looks good as well.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work

I think the current deprecation method

try {
      document.querySelector(selector);
    } catch (error) {
      if (error.name === 'SyntaxError') {
        Drupal.deprecationError({
          message: `Using jQuery-only selector "${selector}" is deprecated in 11.4.0 and is removed from Drupal:12.0.0.`,
        });
      }
    }
    return $(selector);

results in doing multiple dom queries... i.e. document.querySelector(selector); will always be followed by return $(selector); so we do the CSS query twice. I commented on the MR with a suggestion about how to get round this... not sure of the impact.

finnsky’s picture

finnsky commented

@alexpott
Please try this in browser console. Right in this page :)

I have 20%. User will not see it at all. 

(() => {
  const $ = window.jQuery;
  const sel = '#block-system-main';
  const N = 100000;

  const t1 = performance.now();
  for (let i = 0; i < N; i++) { try { document.querySelector(sel); } catch(e){} $(sel); }
  const a = performance.now() - t1;

  const t2 = performance.now();
  for (let i = 0; i < N; i++) $(sel);
  const b = performance.now() - t2;

  console.log(`A (with QS):    ${a.toFixed(1)} ms`);
  console.log(`B (without QS): ${b.toFixed(1)} ms`);
  console.log(`Overhead: ${((a-b)*1e6/N).toFixed(0)} ns per call (${((a-b)/b*100).toFixed(1)}%)`);
})();
finnsky’s picture

finnsky commented

We're not "querying twice". The first document.querySelector(selector)
call is only a syntax probe — to tell whether the input is valid CSS or
Sizzle-only, and decide whether to emit a deprecation. Its result is
discarded: no nodes are returned to the caller, no jQuery object is built
from them, no events/data are attached. The actual query happens once,
inside $(selector).

The probe costs ~100ns per call, and only in dev/tests: in production
deprecations are suppressed via drupalSettings.suppressDeprecationErrors,
so if even those 100ns matter, an early return is enough:

Drupal.deprecateSizzleSelector = function (selector) {
  if (drupalSettings.suppressDeprecationErrors !== false) {
    return $(selector);
  }
  try {
    document.querySelector(selector);
  } catch (error) {
    if (error.name === 'SyntaxError') {
      Drupal.deprecationError({
        message: `Using jQuery-only selector "${selector}" is deprecated in 11.4.0 and is removed from Drupal:12.0.0.`,
      });
    }
  }
  return $(selector);
};

In production this is exactly $(selector); in dev/tests behaviour is
unchanged.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

@finnsky great points... my brain had missed the early 

  if (drupalSettings.suppressDeprecationErrors !== false) {
    return $(selector);
  }

If there are reason this is not written like this:

Drupal.deprecateSizzleSelector = function (selector) {
  if (drupalSettings.suppressDeprecationErrors === false) {
    try {
      document.querySelector(selector);
    } catch (error) {
      if (error.name === 'SyntaxError') {
        Drupal.deprecationError({
          message: `Using jQuery-only selector "${selector}" is deprecated in 11.4.0 and is removed from Drupal:12.0.0.`,
        });
      }
    }
  }
  return $(selector);
};

1 return and less duplicated code? Also I think the deprecation message should link to the CR.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Issue summary: View changes

Fixing link in the issue summary.

finnsky’s picture

finnsky commented

Sorry, let me add that!
You're fine :)

finnsky’s picture

finnsky commented
Status: Needs work » Needs review
needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3568777-nr-bot_pqzuh4j9.txt91 bytes

The 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.

finnsky’s picture

finnsky commented
Status: Needs work » Needs review
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

@finnsky I really like this change - this is a good idea and great work on the tests.

A couple of questions:

  • Should we change the deprecation to removing support in Drupal 13?
  • What does removing support actually mean?
finnsky’s picture

finnsky commented

Should we change the deprecation to removing support in Drupal 13?

I don't know. I think it should have been done at 11 :)

What does removing support actually mean?

Well, this means AJAX in 12 can be rewritten practically without jQuery. This will be very helpful, for example, in Dialog Native and everywhere else.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

Thanks for #47. So the aims are as I thought - rewrite AJAX without jQuery... that's going to happen in 12... so we'll be supporting jQuery for the length of D12 cycle - so practically this deprecation will only be able to be enforced in Drupal 13 right?

finnsky’s picture

finnsky commented

Yes! But we'll be able to write document.querySelector() instead of $() in 12.

Regardless of whether jQuery is present on the page. This has nothing to do with it at all!

This has to do with script waits. The AJAX script and everything related to it now only waits for a string.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

Okay given the tiny changes for core JS I guess deprecating for Drupal 12 is okay and getting this done will probably make it easier to continue to remove jQuery in D12 in other parts of the JS ecosystem.