| Schnark | |
| Jul 26 2016, 8:18 AM |
| F4711075: Screen Shot 2016-11-09 at 22.05.57.png | |
| Nov 9 2016, 10:27 PM |
| F4711077: Screen Shot 2016-11-09 at 22.06.14.png | |
| Nov 9 2016, 10:27 PM |
The json module provides a polyfill for the global JSON object. But according to Can I Use all Grade A browsers provide the feature natively anyway. Additionally, jQuery 3.0 requires native JSON parsing, too.
So the polyfill should no longer be necessary. Additionally it might be a good idea to explicitly feature-test for window.JSON in startup.js, to make sure no Grade X browser without native support slips through.
| Status | Subtype | Assigned | Task | ||
|---|---|---|---|---|---|
| Resolved | Krinkle | T127328 Optimise critical rendering path | |||
| Resolved | Krinkle | T124742 Upgrade to jQuery 3 | |||
| Resolved | Peter | T141344 Remove JSON polyfill |
@Krinkle I believe the last major browser for which we needed this was IE8, which indeed is now Grade C; is there any other reason we were holding back on this?
@Schnark The module already has a skip-function that essentially implements such feature test but in a backward-compatible way.
This doesn't have to block jQuery 3.0 (T124742), but I'm marking it as such now because loading the polyfill that early on would make this fairly inflexible for future maintenance. We're better off without it indeed.
@Jdforrester-WMF This was attempted 1-2 years ago and subsequently reverted because 1) It broke backwards-compatible with modern code that was doing the right thing by declaring the dependency on json, and 2) Because we don't yet require it in our feature test (and can't imho be reliably inferred from the other feature tests).
The JSON interface became standard in ES5. This task could be dependent on (or merged into) T128115: Drop support for ES3 javascript browsers in MediaWiki.
I'm fine with adding a feature test now, but we can also wait for T128115 instead. I'd rather wait for T128115 because we'd have to do some research to verify the impact of the feature-test change. We're already running that research for ES5 now.
Once done, we can turn the "json" module into an empty placeholder for back-compat; to be removed 1 or 2 releases later.
Change 301308 had a related patch set uploaded (by Krinkle):
rlfeature: Measure result of JSON feature test
Change 303119 had a related patch set uploaded (by Krinkle):
rlfeature: Track use of json polyfill as fail, not pass.
Added a few panels for this on the dashboard:
https://grafana.wikimedia.org/dashboard/db/resourceloader-feature-test
Initial data coming in as of a few hours ago.
Change 303119 merged by jenkins-bot:
rlfeature: Track use of json polyfill as fail, not pass.
Change 303602 had a related patch set uploaded (by Krinkle):
rlfeature: Track use of json polyfill as fail, not pass.
Change 303602 merged by jenkins-bot:
rlfeature: Track use of json polyfill as fail, not pass.
Mentioned in SAL [2016-08-08T18:48:43Z] <krinkle@tin> Synchronized php-1.28.0-wmf.13/extensions/WikimediaEvents/modules/ext.wikimediaEvents.rlfeature.js: T141344 Track JSON support (duration: 00m 47s)
Change 320694 had a related patch set uploaded (by Krinkle):
rlfeature: Remove json-support tracking
We've been capturing this for about 3 months now, I think we've got enough to make a decision.
Below screenshots from https://grafana.wikimedia.org/dashboard/db/resourceloader-feature-test
Last 3 months:
Last week:
Support is nearly 100%. Here is a breakdown of all "fail" entries from the last 10 days (61 hits):
| # | Browser | Vendor support |
|---|---|---|
| 13 | Mobile Safari 4.0 on iOS 3.x | 2009-2012 (Apple) |
| 13 | Safari 4.x on Mac OS X 10.5.x | 2007-2011 (Apple) |
| 5 | Chrome 1.0 on Windows XP (Windows NT 5.1) | 2008-2009 (Google); 2001-2009, 2009-2014 (Microsoft) |
| 30 | LG device "Phantom/V2" browser | ?? |
@Krinkle is the best practice to first change depending extensions (searched through Github search) and then completely remove the JSON polyfill or is there a better way?
We should deprecate the module first (and mention in release notes). All existing uses of it should continue to work. Since we will soon require JSON in our startup feature test, we can indeed remove the polyfill files, but we should not remove the module yet. After your commit, loading the module should basically do nothing. (Except maybe a deprecation warning.)
Note that previously the json module already did nothing in most cases because the module uses ResourceLoader's skipFunction feature to mark the module as resolved without loading any files – if window.JSON and others already exist. Perhaps we can use the same technique to register the module without any files and have skinFunction always return true.
Once JSON is required in the startup feature test in MediaWiki core master (either in the same commit or separately beforehand), then we can indeed start removing json from various extensions that use it. We should do so in the same week, at least for our deployed extensions, to avoid too many deprecation warnings in production.
Could you please explain what happens to an extension where the dependency is removed, but is used with older version of MediaWiki without the patch referred Depends-On?
@Nikerabbit Probably best to revert the patch in that case and either have a ULS deprecation warning in prod on every page for a year, or move the module definition to Hooks.php and append the dependency conditionally using PHP logic.
This is my guess:
Now, if #2 is uncommon enough, I'd probably let it go as-is and put a note in our (MLEB) release notes etc. It's a bit late here, so I did not check if new browsers have been added to the blacklists recently (past year or so) or if there is some other reason to think that non-Wikimedia wikis have higher number of these than Wikimedia wikis.
@Nikerabbit We've not had a browser blacklist for some time now. It's based on a feature test instead. (Aside from a few rare exceptions.)
Affected browsers are listed in T141344#2784065. In summary, less than 0.1% of js-supported Grade A page views did not support JSON. Mainly Safari 4.x on older Mac OS and iOS versions. These browsers have been moved from Grade A to Grade C when we removed the JSON polyfill by adding !!window.JSON to our feature test in startup.js.
Right. Well, in my opinion this sounds as an acceptable breakage during the transition time and no need to revert.