Page MenuHomePhabricator

Drop redundant checks from the startup capability logic
Closed, ResolvedPublic

Description

So I've recently been reading through the ResourceLoader startup code looking for ways to improve it, and one of the things I spotted is the list of features we check for to determine if a browser supports JavaScript

startup.js
function isCompatible( ua ) {
	return !!(
		// https://caniuse.com/#feat=es5
		// https://caniuse.com/#feat=use-strict
		// https://caniuse.com/#feat=json / https://phabricator.wikimedia.org/T141344#2784065
		( function () {
			'use strict';
			return !this && Function.prototype.bind && window.JSON;
		}() ) &&
		// https://caniuse.com/#feat=queryselector
		'querySelector' in document &&
		// https://caniuse.com/#feat=namevalue-storage
		// https://developer.blackberry.com/html5/apis/v1_0/localstorage.html
		// https://blog.whatwg.org/this-week-in-html-5-episode-30
		'localStorage' in window &&
		// https://caniuse.com/#feat=addeventlistener
		'addEventListener' in window &&
		// Hardcoded exceptions for browsers that pass the requirement but we don't
		// want to support in the modern run-time.
		//
		// Please extend the regex instead of adding new ones!
		// And add a test case to startup.test.js
		!ua.match( /MSIE 10|NetFront|Opera Mini|S40OviBrowser|MeeGo|Android.+Glass|^Mozilla\/5\.0 .+ Gecko\/$|googleweblight|PLAYSTATION|PlayStation/ )
	);
}

Do we still need to check for some of these features being around? Looking at use-strict (https://caniuse.com/use-strict) vs queryselector (https://caniuse.com/queryselector), as far as I can tell, all browsers that support strict mode also support queryselector, and so we don't really need to check for queryselector support.

The same is true for addeventlistener (https://caniuse.com/addeventlistener) - all browsers that support strict mode also have addeventlistener.

For the localStorage check (https://caniuse.com/#feat=namevalue-storage), the only browser I see that supports strict mode but not local storage (i.e. the only browser that gets filtered out by also checking for localStorage) is Opera Mini, which would get excluded anyway because of the regex (does this mean we should drop the localStorage check, or remove it from the browser regex?)

For window.JSON (https://caniuse.com/json), all browser that support strict mode have JSON too.

Are these checks still needed? If not, I can send a patch to remove, but I wanted to file a task to discuss this given how critical this is to ensuring we don't try to run JavaScript on unsupported browsers.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald Transcript
Krinkle triaged this task as Medium priority.
Krinkle moved this task from Inbox to Accepted Enhancement on the MediaWiki-ResourceLoader board.

For the ES5 check, we can remove the window.JSON check. We should have swapped this for the es5-strict check when it was added. Good catch.

For DOM compliance, we can remove addEventListener indeed. Although not because of es5-strict since that's an unrelated API, but because of querySelector being checked for which is newer and part of the same API. I do wonder why the BBC originally did this. Unlike the ES5 checks, these two not evolve separately for us, but came straight from the original "Cut the mustard" version that we originally started with. In any event, the event listener check is redundant.

For the localStorage check (https://caniuse.com/#feat=namevalue-storage), the only browser I see that supports strict mode but not local storage (i.e. the only browser that gets filtered out by also checking for localStorage) is Opera Mini, […]

Not exactly. Opera Mini is a "proxy" browser where the app is merely a rendering stream from a headless browser that runs on a remote server (kind of like screensharing, but much more efficient). I suspect it is listed as negative there is because it doesnt' work as expected, but I don't know if the API is actually absent in its runtime engine. Either way, Local Storage is the only HTML5 API liste here and that seems worth spending a handful of bytes on to ensure presence of, especially since there is a history of certain browser extensions messing with this API and if they do so in a way that removes the interface entirely then we probably shouldn't engage. So it's not just about the base level as measured in Caniuse etc, but also about what happens in the real-world through browser addons/extensions.

For DOM compliance, we can remove addEventListener indeed. Although not because of es5-strict since that's an unrelated API, but because of querySelector being checked for which is newer and part of the same API. I do wonder why the BBC originally did this. Unlike the ES5 checks, these two not evolve separately for us, but came straight from the original "Cut the mustard" version that we originally started with. In any event, the event listener check is redundant.

What about removing the querySelector check? Caniuse suggests its not needed, but I'm not sure about the relevant apis / real world cases.

I understand you proposed it in the task description. My comment clarifies that we shouldn't remove it.

Change 722706 had a related patch set uploaded (by DannyS712; author: DannyS712):

[mediawiki/core@master] resourceloader: drop some unneeded startup compatibility checks

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

Change 722706 merged by jenkins-bot:

[mediawiki/core@master] resourceloader: drop some unneeded startup compatibility checks

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