Misc: Drop support for older browsers; update support comments #1837
Conversation
|
I've confirmed manually all tests pass on current Chrome, Firefox and on IE8. -846 bytes. I'm sure more can be squeezed out of it, there are most likely some optimizations now possible. |
|
Note to self: IE8 has EDIT: Done. We're at -1033 bytes now. |
mgol
force-pushed
the
drop-old
branch
5 times, most recently
from
3f9b803 to
63ad23c
Compare
|
This PR could use some eyes. Just saying. ;) @dmethvin @timmywil @markelog @gibson042 |
There was a problem hiding this comment.
Does it? That issue was reported for jQuery 2; besides, this changes
nothing in browsers other than IE<8.
Michał Gołębiowski
There was a problem hiding this comment.
Oh right, I was thinking of another issue.
|
Awesome! 👍 |
src/ajax.js
Outdated
There was a problem hiding this comment.
So the previous identification of IE<8 was incorrect?
There was a problem hiding this comment.
Yes, (I think) I've checked that, lemme look.
There was a problem hiding this comment.
Actually, I'm not sure, thanks for making me look into that. For context, see the discussion at #1597; some comments mention IE8. @markelog, you said at #1597 (comment) you couldn't reproduce it in IE8; do you think it might not be needed anymore?
There was a problem hiding this comment.
Yep, this is a tricky one though so not a 100%, but i think we should remove it.
There was a problem hiding this comment.
OK, I'll get rid of it; we can always restore it later.
src/ajax.js
Outdated
There was a problem hiding this comment.
Let's properly space the concat argument to bring this code in line with master and avoid future merge conflicts (cf. #1880).
|
PR updated; I've addressed @gibson042's comments. |
There was a problem hiding this comment.
It would be cool to mention that we need that to support IE8, we already have that in support module, but repetition here might help out
There was a problem hiding this comment.
@dmethvin proposed to go the other way round and I agree; I don't think duplicating such comments is needed; it's not DRY. It's clear here that this one is needed for browsers that misbehave on the input support test; the rest is documented over the test itself.
There was a problem hiding this comment.
Oh, DRY comments :-), when i was reviewing this, i had spent some time searching for what env this was required to, hence my proposition.
But it didn't take that long, so no worries then
There was a problem hiding this comment.
Heh. :) I guess we can discuss it if you prefer the other option; there would be a lot of comments to add so it's a matter for a separate PR anyway. Current way is consistent with the rest of the code so let's leave it as is and we can revisit later. :)
|
Nicely done! |
That includes IE<8, Opera 12.x, Firefox<29, Safari<6.0 and some hacks for old Blackberry. Fixes jquerygh-1836 Fixes jquerygh-1701 Refs jquerygh-1815 Refs jquerygh-1820
|
@markelog I've corrected the test; if you have no further remarks I'd like to land it (I'm currently running the test suite in supported browsers to make sure I didn't break anything). |
|
Landed! -1203 bytes. |
|
Hooray! |
|
The regular run is clean: http://swarm.jquery.org/job/3945. We'll see Androids tomorrow but I've tested locally on 2.3 & 4.0 and it was fine. |
|
👏 |
5 participants
That includes IE<8, Opera 12.x, Firefox<29, Safari<6.0 and some hacks
for old Blackberry.
Fixes gh-1836
Fixes gh-1701
Refs gh-1815
Refs gh-1820