Remove legacy code and align browser support with jQuery compat

[timmywil]

Whether or not we have a branch that supports the same browsers as jQuery master is another issue. I think it may be an appropriate time to bring Sizzle master's browser support in line with jQuery compat. This means removing all legacy code that supports old browsers that jQuery compat will no longer support (including IE6/7).

[mgol]

How does that relate to Sizzle supporting a wide variety of even rolling-release browsers? Would e.g. Sizzle stop supporting Fx/Chrome older than previous minus n (where n is a constant)?

Note also that jQuery & jQuery Compat 3.0.0 drop support for Opera Presto; @gibson042 said Sizzle will maintain support.

[timmywil]

Yes, I am now suggesting that it won't anymore. We can discuss it.

[mgol]

👍 from me; jQuery is the primary consumer of Sizzle anyway. Sizzle is not my baby, though so it's not my choice. ;)

[markelog]

+1

[jzaefferer]

Seems fine to me. We can point out older versions that support older browsers. These aren't maintained anymore, but both Sizzle and jQuery Core are pretty stable projects, so using a slightly older release shouldn't be a problem in most cases.

[gibson042]

Sizzle is basically a customizable (including positional capability) querySelectorAll—or rather, findAll—polyfill. If we toss old browser support, not much externally-visible functionality remains except some edge case fixes, custom selectors, and CSS4-ready :not and :has.

Honestly, I think we'd get more out of improving selector-native so new browsers don't even need Sizzle.

[timmywil]

@gibson042 That's a separate conversation. Bringing browser support in line with jQuery master (previously 2.0) has those implications, but this is only about dropping some legacy code.

But really, I'm not sure what externally-visible functionality that only pertains to old browsers you're talking about.

[gibson042]

But really, I'm not sure what externally-visible functionality that only pertains to old browsers you're talking about.

Correct behavior without a mostly-good native qSA.

[timmywil]

Ah, well, as you know, the code is not all that different when supporting selector extensions, unless we wanted to look into a rewrite where we use matchesSelector for every piece of the selector that we can. I'm up for exploring that, but it wouldn't be the first step.

[gibson042]

My point exactly... as long as we support custom selectors or fix those edge cases, we should also support the wide range of browsers enabled by a few support tests.

[timmywil]

I think there's more code supporting old browsers than a few support tests. But even if there wasn't, this also has an impact on the testing infrastructure. If we support the same browsers as jQuery compat, we no longer need to test as many browsers.

[jzaefferer]

Is this still under discussion or is it decided?

[timmywil]

Yea, we're going to go ahead and align browser support.

[mgol]

This is not written anywhere but this issue conclusion seems incorrect taking into account @gibson042's decision to keep current support and just offer custom builds excluding hacks for specific browsers in the future.

I've been putting up various commits for Sizzle today (mostly updating browsers in Karma & other build stuff) and I must say it was a real pain that Sizzle supports this many browsers. Karma tests are sometimes flakey, BrowserStack doesn't always work with many browsers spawned at the same time (and note you can't completely prevent it as two commits tested at the same time double the amount) so a lot of times something doesn't work as it should, tests also take a lot of time.

While I understand wanting to support more browsers than jQuery in Sizzle - e.g. supporting IE 6-7 & Opera 12.16, and maybe Safari 6.0 and last two Firefox ESRs (which makes 3-4 Firefox versions in total), I don't really see why test on:

1. Chrome 14
2. Firefox 3.6
3. Opera 11.6

and other similarly old browsers. No one uses them today and dropping testing on them would certainly make it easier to contribute.

[gibson042]

There is some redundancy, but also some quirks that are worth covering. I'd like to test only the newest and oldest browsers in normal runs, and carve off the middle into their own grunt task and weekly run à la jQuery (also to be run manually before releasing). Would you be willing to help with that?

[timmywil]

Ultimately, this is up to @gibson042, but I was thinking we'd start by removing any browsers from testing that are not supported in jQuery Compat and then see if there's any code we can remove without causing tests to fail.

[mgol]

There is some redundancy, but also some quirks that are worth covering.

What do you mean by "quirks that are worth covering"? What's the purpose of supporting Chrome 14, Opera 11.6 or Firefox 3.6?

I'd like to test only the newest and oldest browsers in normal runs, and carve off the middle into their own grunt task and weekly run à la jQuery (also to be run manually before releasing). Would you be willing to help with that?

I can help with that. How do you imagine the weekly runs, though? Sizzle is tested entirely on Travis which, AFAIK, has only hooks for commits and you can't run weekly runs on it.

[markelog]

has only hooks for commits and you can't run weekly runs on it.

You can trigger commits through API with some scheduler, that would require a server. But why would you want weekly builds?

Seems pretty unnecessary, ci run is pretty fast already. We can cache the node_modules folder though :-)

[mgol]

Seems pretty unnecessary, ci run is pretty fast already.

It runs for 6-7 minutes if everything goes right (otherwise longer, like 15 minutes) and sometimes gets flakey, triggers BrowserStack API errors etc. It was definitely a pain for me to wait for these runs to finish.

[mgol]

We can cache the node_modules folder though :-)

The main slowdown is from testing, not npm install. I.e. too many browsers.

[markelog]

I don't see not one commit when you changed the source, you were needed to wait, since your changes are only about updating browser versions (instead of creating package that would do that automatically :-) so you need to wait until ci is finishes.

@timmywil and @gibson042 actually never report CI failures, when they were happening. Usual workflow is you commit, then forget about it, if something is wrong, ci will notice you, that is why, travis, for example, send you an email only when you release or when something was wrong.

5-15 minutes is actually nothing, C++ devs would laugh in your face if say something like to them :-). That is where "nightly builds" expression come from btw.

No, of course, if you want to, you can keep cron job on running server, change the build logic and complicate lots of other stuff.

No one ever does this with travis, we would be the first.

[mgol]

I don't see not one commit when you changed the source, you were needed to wait, since your changes are only about updating browser versions (instead of creating package that would do that automatically :-) so you need to wait until ci is finishes.

It's not just about waiting; when I submitted a PR from the upstream branch so that all browsers are tested, there are two Travis runs scheduled - one for the PR and one for the upstream branch; both run on all browsers (perhaps it'd be wise to never run tests on all browsers on PRs since the branch testing covers that already). This created a lot of browsers connecting simultaneously and a lot of 403 messages from BrowserStack, breaking the build. I had to restart those jobs individually later, making sure nothing else runs. Far from ideal.

There's a change 6e8f617 will help, we'll see. Or we'll have to split the browser matrix even further to prevent 15 browsers from connecting at the same time.

@timmywil and @gibson042 actually never report CI failures, when they were happening. Usual workflow is you commit, then forget about it, if something is wrong, ci will notice you, that is why, travis, for example, send you an email only when you release or when something was wrong.

BrowserStack has implemented polling limits fairly recently so setups that didn't fail in the past may very well be failing now. I have experienced it. Before my yesterday's commits there haven't been any since April so the problems might not have been visible in the meantime.

5-15 minutes is actually nothing, C++ devs would laugh in your face if say something like to them :-). That is where "nightly builds" expression come from btw.

It's more about builds breaking than taking 15 minutes. But, again, maybe 6e8f617 will help.

Although the latter, too. The fact that some people have it worse doesn't mean we need to aim to reach the same state...

[markelog]

I submitted a PR from the upstream branch so that all browsers are tested, there are two Travis runs scheduled - one for the PR and one for the upstream branch; both run on all browsers

Don't do that, just use your own fork and your own browserstack key.

BrowserStack has implemented polling limits fairly recently so setups that didn't fail in the past may very well be failing now.

That should have little effect on our situation, since we run browsers sequentially.

The fact that some people have it worse doesn't mean we need to aim to reach the same state...

I'm not saying we should "aim", i'm saying you don't need to look at CI results, you commit and remember about CI only if you have a failure.

I would run CI for thousands hours on one commit, if it would mean we would have less bugs.

Proposition for complication of build logic and adding point of failure, to make CI be couple minutes faster is... weird. That thing with "weekly" stuff, front-end projects don't usually have that, in fact, i know only one project that does - core.

Weekly or Daily builds are "nightly builds", that term only exist, because on projects made with C++ (since i already made that analogy i should stick to it) CI run could take days. Do this because we have our 15 minutes, is, again, weird as hell.

[gibson042]

The current structure of Sizzle is such that fixing glitches in old browsers takes very little code. Couple that with its dubious utility for modern browsers (mostly enabling the ability to express slow-running selectors), and it encourages supporting a wide swath of old technology because that's where the problem space lies. But doing so makes testing much more burdensome, which I'd like to address.

I'm comfortable with doing a full run manually before each release, but Travis offers on-demand builds with custom environment variables that https://github.com/jquery/sizzle/blob/master/test/karma/environment.js could use (e.g., fast Travis-local testing of every PR, moderate BrowserStack testing of every branch, full BrowserStack testing only of API-initiated runs against our master). It's all about responsiveness.

[mgol]

I submitted a PR from the upstream branch so that all browsers are tested, there are two Travis runs scheduled - one for the PR and one for the upstream branch; both run on all browsers

Don't do that, just use your own fork and your own browserstack key.

How? If I submit PRs from my fork, BrowserStack isn't used. Do you suggest modifying the secret keys for the PR?

BrowserStack has implemented polling limits fairly recently so setups that didn't fail in the past may very well be failing now.

That should have little effect on our situation, since we run browsers sequentially.

We run tests in 17 browsers at the same time, it was definitely failing for me on the Sizzle repo.

Proposition for complication of build logic and adding point of failure, to make CI be couple minutes faster is... weird. That thing with "weekly" stuff, front-end projects don't usually have that, in fact i know only one project that does - core.

And it's very useful in Core, we finally have green builds that don't take ages. But yeah, Sizzle is different in that it has tremendously fewer tests than jQuery.

[mgol]

The current structure of Sizzle is such that fixing glitches in old browsers takes very little code. Couple that with its dubious utility for modern browsers (mostly enabling the ability to express slow-running selectors), and it encourages supporting a wide swath of old technology because that's where the problem space lies.

I agree with that point, mine was that we could limit this to old browsers that have a chance of people actually using them. Which is the case for IE6-7 (enterprise usage), Opera 12.16 (last Presto Opera which never got automatically updated to the Blink line) but I don't believe it's the case for e.g. Firefox 3.6 or Opera 11.6.

Also, for the case of jQuery & jQuery Compat it would be useful to not carry that old browser baggage - but I understand that's planned to resolve and just requires time.

That said, I don't care too much if regular runs are tested on a subset of browsers and full runs only periodically or before releases.

[gibson042]

Don't do that, just use your own fork and your own browserstack key.

How? If I submit PRs from my fork, BrowserStack isn't used. Do you suggest modifying the secret keys for the PR?

Environment variables: BROWSER_STACK_USERNAME=… BROWSER_STACK_ACCESS_KEY=… grunt --force build karma:phantom karma:desktop …

[markelog]

@gibson042

but Travis offers on-demand builds with custom environment variables that https://github.com/jquery/sizzle/blob/master/test/karma/environment.js could use (e.g., fast Travis-local testing of every PR

You would need a server for this, see above.

fast Travis-local testing of every PR

I would just made those keys public, lots projects already do that. Test that stuff automatically.

It's all about responsiveness.

I see it like that - you made changes to fix android bug? Run tests only in android grunt test --browsers=bs_android-4.0.

Then make a PR, which would run everything on everything, so you notice the issue with ios 5 right away, and don't wait for week after pr was landed and trashed the commit history.

@mzgol

We run tests in 17 browsers at the same time,

So lets divide them on smaller groups.

And it's very useful in Core, we finally have green builds that don't take ages.

We have green builds, that was primary reason, at least for me.

[mgol]

@gibson042

Environment variables: BROWSER_STACK_USERNAME=… BROWSER_STACK_ACCESS_KEY=… grunt --force build karma:phantom karma:desktop …

Ah, locally. I thought the remark was about making those tests run in the PR. OK then.

@markelog

I would just made those keys public, lots projects already do that. Test that stuff automatically.

I recall BrowserStack was against making their keys public; Sauce Labs was not. We'd need to get BrowserStack to agree or use Sauce Labs for PRs. But I agree in principle.

EDIT: Also, if we were to do that, I'd prefer to create a separate Travis-dedicated BrowserStack account and use its keys, not yours, @markelog. :-)

[markelog]

Well, yeah, but i remember other devs do that, so we should be in the clear

[nazar-pc]

What is current status here, taking into account that jQuery Compat is already dead before even being released?

[HolgerJeromin]

Removing support for some rotten browsers would reduce such reports:
jquery/jquery#3923, jquery/jquery#3551

sizzle/src/sizzle.js

Lines 816 to 818 in 3116795

	// Opera 10-11 does not throw on post-comma invalid pseudos
	el.querySelectorAll("*,:x");
	rbuggyQSA.push(",.*:");

I develop my App with "catch all exception" so i would like to get rid of this line, even if this is still valid:

sizzle/src/sizzle.js

Lines 833 to 836 in 3116795

	// This should fail with an exception
	// Gecko does not error, returns false instead
	matches.call( el, "[s!='']:x" );
	rbuggyMatches.push( "!=", pseudos );

[mgol]

This is probably not going to happen. Instead, we're going to remove the Sizzle dependency from jQuery and go from there.