[1.13] Feature/remove some jquery 3 migrate warnings #1773
Conversation
|
|
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. |
|
Sorry to nag you; are you still interested in this PR? (And eventually #1774) |
ui/jquery-1-11.js
Outdated
|
|
||
| // Support: jQuery 1.11.x or older | ||
| var versions = $.fn.jquery.split( "." ); | ||
| if ( parseInt( versions[ 0 ], 10 ) === 1 && parseInt( versions[ 1 ], 10 ) <= 11 ) { |
There was a problem hiding this comment.
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)
ui/jquery-1-11.js
Outdated
| var versions = $.fn.jquery.split( "." ); | ||
| if ( parseInt( versions[ 0 ], 10 ) === 1 && parseInt( versions[ 1 ], 10 ) <= 11 ) { | ||
|
|
||
| // $.unique has been renamed to uniqueSort |
There was a problem hiding this comment.
uniqueSort -> $.uniqueSort
ui/jquery-1-9.js
Outdated
| if ( parseInt( versions[ 0 ], 10 ) === 1 && parseInt( versions[ 1 ], 10 ) <= 9 ) { | ||
|
|
||
| // $.expr [ ":" ] has been renamed to $.expr.pseudos | ||
| $.expr.pseudos = $.expr[ ":" ]; |
There was a problem hiding this comment.
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. |
|
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. |
This commit polyfills jQuery.expr.pseudos for old versions of jQuery.
This commit will polyfill uniqueSort for old versions of jQuery, < 1.12.0.
These are similar to the already existing 1.7.x support file. The compatibility/migration checks for $.expr.pseudos and $.uniqueSort were moved into these files, as they are included from /ui/core.js.
|
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, { | |||
There was a problem hiding this comment.
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.
|
Thank you for your work on this, Eirik and Scott. Looking forward to the release of 1.13. Will it fix all jquery 3 deprecation warnings? |
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?