Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Core: Drop support for IE <11, iOS <11, Firefox <65, Android Browser & PhantomJS #4347

Merged
merged 1 commit into from Apr 29, 2019

Conversation

mgol
Copy link
Member

@mgol mgol commented Apr 10, 2019

Summary

Also, update support comments format to match format described in:
jquery/contribute.jquery.org#95 (comment)
with the change from:
jquery/contribute.jquery.org#95 (comment)
(open-ended ranges end with +).

Further size reductions will be achieved when we drop Firefox 60, iOS 10
and pre-Chromium Edge versions.

Fixes gh-3950
Fixes gh-4299

Checklist

@mgol mgol added this to the 4.0.0 milestone Apr 10, 2019
@mgol mgol force-pushed the support-updates branch 2 times, most recently from ba2c346 to 108d0a0 Compare April 10, 2019 13:22
@mgol
Copy link
Member Author

mgol commented Apr 10, 2019

-540 -577 -867 -856 bytes.

I recommend reviewing with whitespace changes ignored (e.g. via adding ?w=1 to the URL) to see real changes in xhr.js that got de-indented.

A few open questions:

  1. I removed support.cors & support.ajax - are we OK with always assuming they're true? I'm not sure if IE 11 allows to disable native XHR as earlier IE did or if we still want to cater to those scenarios (we don't support the ActiveX versions anyway).
  2. In a few places (e.g. src/data/Data.js) we don't delete stuff from DOM nodes, instead setting them to undefined. It's supposedly to help with DOM performance in Blink/WebKit; the ticket referenced there is not available to the public, though. Do we know if this is still true and if it maybe applies to Gecko as well or not? I'd tweak the comment then.
  3. wrapMap.js references IE 9 directly in two places. Most of the file is covered by the comment the wrapping is needed when used in XHTML; is that not true for option as well? We should either move option handling to be under the same XHTML-related comment, remove it or update the support comment if it's still needed for IE 11. Update: this code has been removed from the PR.
  4. curCSS.js references that elem.style is retrieved before the computed style as it fixes some issue in Firefox where it gets wrong values on detached elements. I couldn't find any relevant test or any more details about the supposed issue, could you help, @timmywil, as you committed the change? This has been removed as the workaround that needed it is gone (it was neeeded for iOS 10 only).

Copy link
Member

@dmethvin dmethvin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👏 👏

},
xhrSupported = jQuery.ajaxSettings.xhr();

support.cors = !!xhrSupported && ( "withCredentials" in xhrSupported );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The scenarios where I've ever seen this used are jQuery.support.cors = true, that is, just setting the value. I wonder if there are a lot of cases where people check to see if it's true in their own code? If so it might be safest to set it to true here. Here's an example of some code that would break if we didn't: victorquinn/Backbone.CrossDomain#1

Copy link
Member Author

@mgol mgol Apr 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we could add a line with:

jQuery.support.ajax = jQuery.support.cors = true;

to deprecated.js and add a warning to Migrate for code accessing it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's mainly useful of you want to gracefully degrade and/or fail in browsers without cors which will be none with the new support list. I think it would be better to keep it set to true for a good while in deprecated.js before removing it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could also have Migrate fill in those properties as it's easy to do from outside. Technically speaking, external access to jQuery.support has never been supported.

@mgol
Copy link
Member Author

mgol commented Apr 10, 2019

BTW, dropping support for iOS 10 & Firefox 60 (which we'll most likely do before the 4.0 release as we'll drop them around September/October) will cause a further reduction by -272 bytes, getting us to -812 bytes compared to master and only +14 bytes compared to latest 2.2.x. Here are relevant changes on top of this PR: mgol@724f955.

@mgol
Copy link
Member Author

mgol commented Apr 10, 2019

I ran tests locally on this PR on most of the supported browsers:
Screen Shot 2019-04-10 at 16 48 18

@mgol mgol force-pushed the support-updates branch 2 times, most recently from 3f6f9ea to 40567ca Compare April 15, 2019 16:52
@mgol
Copy link
Member Author

mgol commented Apr 15, 2019

Per the discussion at the today's meeting, I changed the format of IE support comments from:

// Support: IE <=9 - 11 only

to:

// Support: IE <=9 - 11+

@mgol
Copy link
Member Author

mgol commented Apr 15, 2019

I added one more commit removing IE 9-related code from wrapMap.js - I run some manual tests in XHTML mode and it didn't seem to cause any changes. The original wrapping of <option> in <select> was added in 14b0902 (12.5 years ago!), it was changed to be wrapped in <select multiple> in 264ffbc. Removing that wrapping does make more tests fail in IE 9 but IE 11 still passes all the tests.

This removal saved additional 33 bytes. 🍾

@mgol mgol self-assigned this Apr 15, 2019
@mgol
Copy link
Member Author

mgol commented Apr 16, 2019

@jquery/core I've been thinking that maybe we should just drop Firefox 60 & iOS 10 in this PR as well. We'll do that anyway in October and it's not very likely we'll release 4.0 before that date. Especially dropping iOS 10 would give us a lot - it has a partially broken module support, early versions had partially broken Shadow DOM support, etc. Dropping it would help not only with library size but also it'd be easier to do all the upcoming major changes like the event system refactor, getting rid of Sizzle etc. if we didn't have to care about iOS 10 and breaking any of current workarounds targeted at it.

If, by any chance, we manage to land all those major changes, release an alpha/beta & then stable before we'd normally drop Firefox 60/iOS 10 (not very likely, IMO), we can always update the browser support page for jQuery 4.0 to state explicitly we don't support those browsers. We could later remove that note once they'd be out of our supported browser range anyway.

I already have relevant changes ready at mgol@724f955, I could just merge it to this PR.

What do you think?

@dmethvin
Copy link
Member

I agree on the timeline, it seems likely that 4.0.0 could wait for those older browsers to drop below the threshold where we'd need to support them. It definitely cleans up some things!

@mgol mgol force-pushed the support-updates branch 2 times, most recently from 074a326 to ee2fc25 Compare April 23, 2019 21:20
@mgol
Copy link
Member Author

mgol commented Apr 24, 2019

I added a commit dropping Firefox 60 & iOS 10. We can drop it if others disagree with me, @dmethvin & @jbedard but I think it makes sense to do it now.

We're down to -867 bytes!

Copy link
Member

@dmethvin dmethvin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New changes LGTM

documentElement.mozMatchesSelector ||
documentElement.oMatchesSelector ||
documentElement.msMatchesSelector,
matches = documentElement.matches || documentElement.msMatchesSelector,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could use a // Support comment since the ms version should go away when we get a Chromium Edge.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point about a support comment but it won’t go away as IE needs it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment added. I only mentioned IE as Edge doesn't need it since 4 versions already.

@mgol mgol removed the Needs review label Apr 29, 2019
…& PhantomJS

Also, update support comments format to match format described in:
jquery/contribute.jquery.org#95 (comment)
with the change from:
jquery/contribute.jquery.org#95 (comment)
(open-ended ranges end with `+`).

Fixes jquerygh-3950
Fixes jquerygh-4299
@mgol mgol changed the title Core: Drop support for IE <11, iOS <10, Android Browser & PhantomJS Core: Drop support for IE <11, iOS <11, Firefox <65, Android Browser & PhantomJS Apr 29, 2019
@mgol mgol merged commit cf84696 into jquery:master Apr 29, 2019
@mgol mgol deleted the support-updates branch April 29, 2019 20:56
@@ -172,9 +150,6 @@ function domManip( collection, args, callback, ignored ) {

// Keep references to cloned scripts for later restoration
if ( hasScripts ) {

// Support: Android <=4.0 only, PhantomJS 1 only
// push.apply(_, arraylike) throws on ancient WebKit
jQuery.merge( scripts, getAll( node, "script" ) );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mgol I was just going back over this PR and wondering about the loss of context here and before other jQuery.merge calls... should we switch this to push.apply( scripts, getAll( node, "script" ) ) or have a comment explaining why it avoids that pattern?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right... We no longer care about Android 4.0 or PhantomJS 1 but I remember using the push.apply approach may lead to issues like #4320. Perhaps a comment to that effect would be useful.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I created #5298 to track this & other related uses of push.apply vs. jQuery.merge to consider.

@@ -475,9 +456,6 @@ jQuery.each( {
for ( ; i <= last; i++ ) {
elems = i === last ? this : this.clone( true );
jQuery( insert[ i ] )[ original ]( elems );

// Support: Android <=4.0 only, PhantomJS 1 only
// .get() because push.apply(_, arraylike) throws on ancient WebKit
push.apply( ret, elems.get() );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here too; why isn't this push.apply( ret, elems )?

mgol added a commit to mgol/jquery that referenced this pull request Apr 30, 2019
A leftover `rboxStyle` was left in the wrapper parameters but not in the
dependency array, causing `getStyles` to be undefined in AMD mode.

Since `rboxStyle` is no longer used, it's now removed.

Ref jquerygh-4347
@mgol mgol mentioned this pull request Apr 30, 2019
2 tasks
mgol added a commit that referenced this pull request Apr 30, 2019
A leftover `rboxStyle` was left in the wrapper parameters but not in the
dependency array, causing `getStyles` to be undefined in AMD mode.

Since `rboxStyle` is no longer used, it's now removed.

Ref gh-4347
Closes gh-4380
mgol added a commit to mgol/jquery that referenced this pull request Aug 21, 2019
Previously, jQuery.ajaxSettings.xhr, contents were wrapped in a try-catch
as we defined jQuery.support.ajax & jQuery.support.cors executed during the
jQuery load and we didn't want to crash if IE had native XHR disabled (which
is possible). While jQuery hasn't supported the ActiveX-based XHR since 2.0,
jQuery with XHR disabled could still be used for its other features in such
a crippled browser.

Since jquerygh-4347, jQuery.support.ajax & jQuery.support.cors no longer exist, so
we don't need the try-catch anymore.

Fixes jquerygh-1967
Ref jquerygh-4347
mgol added a commit to mgol/jquery that referenced this pull request Aug 22, 2019
Previously, jQuery.ajaxSettings.xhr, contents were wrapped in a try-catch
as we defined jQuery.support.ajax & jQuery.support.cors executed during the
jQuery load and we didn't want to crash if IE had native XHR disabled (which
is possible). While jQuery hasn't supported the ActiveX-based XHR since 2.0,
jQuery with XHR disabled could still be used for its other features in such
a crippled browser.

Since jquerygh-4347, jQuery.support.ajax & jQuery.support.cors no longer exist, so
we don't need the try-catch anymore.

Fixes jquerygh-1967
Ref jquerygh-4347
mgol added a commit that referenced this pull request Aug 26, 2019
Previously, jQuery.ajaxSettings.xhr, contents were wrapped in a try-catch
as we defined jQuery.support.ajax & jQuery.support.cors executed during the
jQuery load and we didn't want to crash if IE had native XHR disabled (which
is possible). While jQuery hasn't supported the ActiveX-based XHR since 2.0,
jQuery with XHR disabled could still be used for its other features in such
a crippled browser.

Since gh-4347, jQuery.support.ajax & jQuery.support.cors no longer exist, so
we don't need the try-catch anymore.

Fixes gh-1967
Closes gh-4467
Ref gh-4347
@lock lock bot locked as resolved and limited conversation to collaborators Oct 27, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
4 participants