Selector: Inline Sizzle into the selector module #4395

mgol · 2019-05-13T21:07:38Z

Summary

This PR removes Sizzle from jQuery. The first commit inlines Sizzle mostly as-is, the subsequent commit fixes ESLint violations; later commits mostly remove workarounds for old browsers.

Currently the PR reduces the gzipped jQuery size by 1201 bytes compared to master.

I'm opening it as a draft as I still need to backport Sizzle tests. It's ready for initial review, though (reviewing by commit might be easier).

I've checked that all the tests pass in Chrome 74 & IE 11. All Sizzle tests are also passing for jQuery.find except for the Sizzle.noConflict one - but jQuery.find has always stripped noConflict out of its internal Sizzle version.

Checklist

All authors have signed the CLA at https://cla.js.foundation/jquery/jquery
New tests have been added to show the fix or feature works (Sizzle tests ported in Selector: Port Sizzle tests to jQuery #4406)
Grunt build and unit tests pass locally with these changes
~~If needed, a docs issue/PR was created at https://github.com/jquery/api.jquery.com~~

dmethvin

👏 👏 👏
I like the progressive commits and the approach seems good, I'll leave approval to the Sizzleers!

timmywil · 2019-05-20T15:47:49Z

This looks like it's on the right track. I'd like to see it with the tests migrated.

mgol · 2019-05-20T19:41:10Z

I submitted a draft PR porting Sizzle tests in #4406. I'll be adding more commits to it but it might be easier for you to review commit by commit even now instead of all at once when I'm done.

mgol · 2019-05-27T20:05:37Z

@jquery/core During my work on #4406, I've reminded myself we support many more custom selectors than just the ones listed in the 3.4.0 blog post as deprecated. We listed: :first, :last, :eq, :even, :odd, :lt, :gt, and :nth but we also support non-standard (linked to their API pages):

:contains - no easy replacement?
[attr!="value"] - can be replaced by users to :not([attr="value"])
:selected - can be replaced with :checked which natively supports selected options
:input - no easy replacement other than listing all the matching elements?
and probably others, these are just the ones I had to deal with so far.

What do we plan to do with those selectors? Do we want to implement internal replacement (where possible, :contains is out, I think)?

dmethvin · 2019-05-27T20:36:24Z

Some of this can be handled by Migrate, or at least the easy cases of them. I wonder if we want any of custom selectors in 4.0 or should just have new custom methods by similar names where needed?

It would be useful to have very clear instructions on how to convert from old to new, even if it means making new core methods to simplify things. So $("div:contains(h1) > p") is hard to rewrite unless we create a new method so it would be possible to say $("div").contains("h1").children("p").

mgol · 2019-05-29T09:19:29Z

I wonder if we want any of custom selectors in 4.0 or should just have new custom methods by similar names where needed?

(porting some info from the Gitter discussion) We still need to maintain our own custom parsing just to reliably apply the ID hack to have a correct scoping mechanism. When we have parsing, adding some re-mapping may not be that hard in many of those cases (like :selected), so we can try. :contains is a bit different here.

timmywil · 2019-05-29T13:59:44Z

Porting a little more from the Gitter conversation, :contains will be much easier to support than POS selectors. I think we can revisit which selectors we really want to keep, but I expect we'll be able to support all but POS in the upcoming rewrite.

timmywil · 2019-05-29T14:00:49Z

As far as this PR is concerned, I think it's good to go as a first step.

test/data/testinit.js

Krinkle · 2019-06-26T23:28:17Z

Also, if not already, ensure all entries in AUTHORS.txt are present in the jQuery one.

mgol · 2019-06-27T08:51:45Z

Also, if not already, ensure all entries in AUTHORS.txt are present in the jQuery one.

@Krinkle Good point. I'll need to sort them into the jQuery list based on comparing their first contribution to Sizzle with other first contributions to jQuery, I guess.

mgol · 2019-07-01T21:52:05Z

OK, I think this is ready to review. I removed the draft status.

mgol · 2019-07-03T17:13:05Z

@Krinkle AUTHORS.txt updated.

This commit removes Sizzle from jQuery, inlining its code & removing obsolete workarounds where applicable. The selector-native module has been removed. Further work on the selector module may decrease the size enough that it will no longer be necessary. If it turns out it's still useful, we'll reinstate it but the code will look different anyway as we'll want to share as much code as possible with the existing selector module. The Sizzle AUTHORS.txt file has been merged with the jQuery one - people are sorted by their first contributions to either of the two repositories. The commit reduces the gzipped jQuery size by 1469 bytes compared to master.

This commit removes Sizzle from jQuery, inlining its code & removing obsolete workarounds where applicable. The selector-native module has been removed. Further work on the selector module may decrease the size enough that it will no longer be necessary. If it turns out it's still useful, we'll reinstate it but the code will look different anyway as we'll want to share as much code as possible with the existing selector module. The Sizzle AUTHORS.txt file has been merged with the jQuery one - people are sorted by their first contributions to either of the two repositories. The commit reduces the gzipped jQuery size by 1460 bytes compared to master. Closes gh-4395

Due to a faulty IE 8 workaround removal, the fast path qSA mode was skipped when jQuery's find was called on an element node - i.e. in most cases. 😱 Ref jquerygh-4395

Due to a faulty IE 8 workaround removal, the fast path qSA mode was skipped when jQuery's find was called on an element node - i.e. in most cases. 😱 Ref gh-4395 Closes gh-4452

mgol added this to the 4.0.0 milestone May 13, 2019

mgol added Needs review Selector labels May 13, 2019

mgol self-assigned this May 13, 2019

mgol requested review from dmethvin, gibson042 and timmywil May 13, 2019 21:09

dmethvin reviewed May 13, 2019

View reviewed changes

mgol mentioned this pull request May 14, 2019

Explanation of why we support ancient browsers jquery/sizzle#389

Closed

timmywil approved these changes May 29, 2019

View reviewed changes

mgol force-pushed the sizzle-removal branch from fef05cc to ca028f3 Compare June 26, 2019 20:06

mgol mentioned this pull request Jun 26, 2019

home page of sizle: documentation link is broken jquery/sizzle#447

Closed

Krinkle reviewed Jun 26, 2019

View reviewed changes

test/data/testinit.js Outdated Show resolved Hide resolved

mgol marked this pull request as ready for review July 1, 2019 21:49

3imed-jaberi approved these changes Jul 16, 2019

View reviewed changes

mgol force-pushed the sizzle-removal branch from e910c17 to b1ef60b Compare July 29, 2019 17:25

mgol force-pushed the sizzle-removal branch from b1ef60b to 90997bd Compare July 29, 2019 19:00

mgol changed the title ~~Remove Sizzle~~ Selector: Inline Sizzle into the selector module Jul 29, 2019

mgol merged commit 30e1bfb into jquery:master Jul 29, 2019

mgol deleted the sizzle-removal branch July 29, 2019 19:14

mgol removed the Needs review label Jul 29, 2019

This was referenced Jul 29, 2019

Selector: Allow whitespace after hex escapes in CSS identifiers jquery/sizzle#450

Closed

IE & Edge crash with "Permission denied" on the contents() for <frame /> test #4441

Closed

GulajavaMinistudio mentioned this pull request Jul 30, 2019

Stop adding accessors to their parent GulajavaMinistudio/jquery#94

Merged

4 tasks

mgol mentioned this pull request Aug 8, 2019

Selector: Bring back querySelectorAll shortcut usage #4452

Merged

2 tasks

lock bot locked as resolved and limited conversation to collaborators Jan 25, 2020

Selector: Inline Sizzle into the selector module #4395

Selector: Inline Sizzle into the selector module #4395

Uh oh!

Conversation

mgol commented May 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

Uh oh!

dmethvin left a comment

Choose a reason for hiding this comment

Uh oh!

timmywil commented May 20, 2019

Uh oh!

mgol commented May 20, 2019

Uh oh!

mgol commented May 27, 2019

Uh oh!

dmethvin commented May 27, 2019

Uh oh!

mgol commented May 29, 2019

Uh oh!

timmywil commented May 29, 2019

Uh oh!

timmywil commented May 29, 2019

Uh oh!

Uh oh!

Krinkle commented Jun 26, 2019

Uh oh!

mgol commented Jun 27, 2019

Uh oh!

mgol commented Jul 1, 2019

Uh oh!

mgol commented Jul 3, 2019

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

5 participants

mgol commented May 13, 2019 •

edited

Loading