Remove JSON polyfill
Closed, ResolvedPublic

Description

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.

Schnark created this task.Jul 26 2016, 8:18 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJul 26 2016, 8:18 AM

@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

https://gerrit.wikimedia.org/r/301308

Krinkle triaged this task as Low priority.Jul 27 2016, 1:35 AM
Krinkle moved this task from Inbox to Accepted: Enhancement on the MediaWiki-ResourceLoader board.

Change 301308 merged by jenkins-bot:
rlfeature: Measure result of JSON feature test

https://gerrit.wikimedia.org/r/301308

Change 303119 had a related patch set uploaded (by Krinkle):
rlfeature: Track use of json polyfill as fail, not pass.

https://gerrit.wikimedia.org/r/303119

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.

https://gerrit.wikimedia.org/r/303119

Change 303602 had a related patch set uploaded (by Krinkle):
rlfeature: Track use of json polyfill as fail, not pass.

https://gerrit.wikimedia.org/r/303602

Change 303602 merged by jenkins-bot:
rlfeature: Track use of json polyfill as fail, not pass.

https://gerrit.wikimedia.org/r/303602

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

https://gerrit.wikimedia.org/r/320694

Krinkle added a comment.EditedNov 9 2016, 10:27 PM

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):

#BrowserVendor support
13Mobile Safari 4.0 on iOS 3.x2009-2012 (Apple)
13Safari 4.x on Mac OS X 10.5.x2007-2011 (Apple)
5Chrome 1.0 on Windows XP (Windows NT 5.1)2008-2009 (Google); 2001-2009, 2009-2014 (Microsoft)
30LG device "Phantom/V2" browser??

Sounds reasonable.

Change 320694 merged by jenkins-bot:
rlfeature: Remove json-support tracking

https://gerrit.wikimedia.org/r/320694

Gilles moved this task from Inbox to Backlog on the Performance-Team board.Nov 10 2016, 8:26 PM
Krinkle assigned this task to Peter.
Krinkle moved this task from Backlog to Next-up on the Performance-Team board.

Change 322617 had a related patch set uploaded (by Phedenskog):
Remove JSON polyfill

https://gerrit.wikimedia.org/r/322617

@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?

@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.

Krinkle closed this task as Resolved.Nov 23 2016, 2:20 AM

Change 322617 merged by jenkins-bot:
Remove JSON polyfill, deprecate 'json' module

https://gerrit.wikimedia.org/r/322617

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:

  1. Browser that supports JSON: nothing happens
  2. Browser that does not support JSON but still gets JavaScript: there will be a JavaScript error when JSON is used, possibly breaking multiple things.

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.