[1.13] Feature/remove some jquery 3 migrate warnings #1773
Conversation
jsf-clabot
commented
Dec 2, 2016
•
|
|
Yes, this is something we would be interested in merging. Historically, we just used the deprecated methods as long as possible since that's the easiest solution for supporting so many versions of jQuery Core. However, we have switched to a different model where we upgrade to using the new APIs without any checks and then load in compatibility layers to support older versions of jQuery. You can see how we handle this in /ui/jquery-1-7.js. I'd suggest adding Thanks. |
|
Ok, I moved the polyfills into their own files now. I also removed the surrounding checks. |
eirslett
referenced
this pull request
Dec 4, 2016
Closed
Add ability to run tests with jQuery Migrate #1774
|
Sorry to nag you; are you still interested in this PR? (And eventually #1774) |
| + | ||
| +// Support: jQuery 1.11.x or older | ||
| +var versions = $.fn.jquery.split( "." ); | ||
| +if ( parseInt( versions[ 0 ], 10 ) === 1 && parseInt( versions[ 1 ], 10 ) <= 11 ) { |
scottgonzalez
Jan 25, 2017
Owner
I know you based this on jquery-1-7.js, but you'll notice there's a big comment about why we're using version sniffing instead of feature detection. The feature detection here is very simple, so this condition should change to if (!$.uniqueSort)
| +var versions = $.fn.jquery.split( "." ); | ||
| +if ( parseInt( versions[ 0 ], 10 ) === 1 && parseInt( versions[ 1 ], 10 ) <= 11 ) { | ||
| + | ||
| + // $.unique has been renamed to uniqueSort |
| +if ( parseInt( versions[ 0 ], 10 ) === 1 && parseInt( versions[ 1 ], 10 ) <= 9 ) { | ||
| + | ||
| + // $.expr [ ":" ] has been renamed to $.expr.pseudos | ||
| + $.expr.pseudos = $.expr[ ":" ]; |
scottgonzalez
Jan 25, 2017
Owner
Same thing here. You can just check for the existence of $.expr.pseudos.
|
I have updated the PR after code review/feedback, fixing the issues that were raised. |
|
Thanks. These changes look good. I just wanted to give you a heads up that it will be a little bit before this gets merged. Since it introduces new modules, it needs to be part of the 1.13.0 release. We're getting close to the 1.12.2 release, after which this can be merged. |
|
If the introduction of new modules is a breaking change, then maybe another change could be made? Instead of a 1.7 polyfill, a 1.9 polyfill, a 1.12 polyfill etc., there could be a 1.x polyfill for all jQuery versions 1.x. (Considering jQuery 2.0 was released in April 2013, it shouldn't be a big burden for people on jQuery 1.12.3 to include the whole 1.x polyfill) |
|
Introducing a 1.x module still requires 1.13.0, though I'm not against that idea. 1.7 is the heaviest, but it's really not a big price to pay for 1.x support in general, despite the fact that very few sites (comparatively) actually use 2.x or 3.x. Using statistics from builtwith.com, 1.7.x accounts for about 6.5% of jQuery usage, at ~4.4M sites. All 2.x releases only amount to a quarter of that. 3.x releases are so unpopular that they don't even report usage statistics for it yet. However, these stats don't say anything about rate of release and update. Barring any objections from @jquery/ui, I'd accept a PR that merged all of the files together (you can even just update this PR). |
|
A single module for jQuery "adapters" seems fine to me. These new ones are just one liners and don't need their own files. |
scottgonzalez
changed the title from
Feature/remove some jquery 3 migrate warnings to [1.13] Feature/remove some jquery 3 migrate warnings
Feb 21, 2017
|
How about we just use the existing @eirslett Would you like to make that change? We're getting ready to release 1.12.2 and I'd be happy to get this in there. |
|
Ok, I'll have a look at it. |
eirslett
added some commits
Dec 2, 2016
|
I moved all the polyfills into jquery-1-7 now, and rebased the commits on top of the master branch. |
| @@ -23,7 +23,7 @@ | ||
| factory( jQuery ); | ||
| } | ||
| } ( function( $ ) { | ||
| -return $.extend( $.expr[ ":" ], { | ||
| +return $.extend( jQuery.expr.pseudos, { |
scottgonzalez
May 2, 2017
Owner
This has to stay as $ instead of jQuery for noConflict() support. $ is a local reference via the parameter.
I'll make the change when I land this.
eirslett commentedDec 2, 2016
•
edited
There are some migration errors we get when using jQuery 3:
and
Both
jQuery.expr.pseudosandjQuery.uniqueSortwere introduced after 1.7.0 (which is the oldest supported jQuery version for jQuery UI), so this PR uses a polyfill solution.The unit tests appear to be passing.
Is this a change that would be interesting to merge into jQuery UI? In that case, are there any changes to the implementation, or the commit messages, that have to be done?
I'm not sure about where to put the polyfills; now I put them right above the code that had to be polyfilled. Another option would be to put the required polyfills in top of every file (module declaration) that needs them. Putting polyfills in a separate file is one option, but more involved, and it requires the file to be imported; I don't think that is a good idea.
Git commit messages should be per component, but this is really a cross-component change. Should I split the commits into separate ones per component?