I don't think we need to go through an opt-in period for the minifier. The benchmarks suffice in being representative, I think. If we're unsure, we can run it from CLI on production hardware and/or against a few hundred core/wmf extension modules to gather more data. Exposing it to end-users is not going to give us useful information we don't already have afaik.

As it stands, I understand it to be too slow, and we'll need some engineering time to profile it and tweak it a bit, after which we can go roll out it progessively with the train, to be evaluated during that week through our metrics.

Alongside this, I think we should also formalize the notion of grade B (in between grade A and grade C) in ResourceLoader. Modules that are flagged as using ES6 should not be executed in browsers that don't support ES6 (most prominently including IE11). ResourceLoader currently does not have a concept of per-module executability, only a global one: the startup code feature-detects ES5 support, and either decides that no JavaScript is going to be executed or that everything can be. In browsers that support ES5 but not ES6 (like IE11), we'd presumably want to execute the ES5-compatible modules, but not the ES6 ones.

In T272104#6750238, @Catrope wrote:

Alongside this, I think we should also formalize the notion of grade B (in between grade A and grade C) in ResourceLoader. Modules that are flagged as using ES6 should not be executed in browsers that don't support ES6 (most prominently including IE11). ResourceLoader currently does not have a concept of per-module executability, only a global one: the startup code feature-detects ES5 support, and either decides that no JavaScript is going to be executed or that everything can be. In browsers that support ES5 but not ES6 (like IE11), we'd presumably want to execute the ES5-compatible modules, but not the ES6 ones.

I like this idea. So maybe it's less about which minifier to use and more about giving developers the ability to decide whether or not to support legacy browsers on a per-module basis. If a team is developing a new feature which already provides a no-JS fallback, they may decide that the JS-enhanced version doesn't need to jump through the extra hoops to support IE11.

There will likely be a lengthy transitional period where a few features are written this way but most are still backwards-compatible with ES5. Providing a way for module authors to specify whether or not their code should be run in a legacy context could help streamline things.

ES6 compatibility isn't easy to test for, because there are so many aspects of ES6. I looked through kangax's compatibility tables and explored using native Promise support as a proxy for "good enough" ES6 support. That seems to work relatively well. If we ignore tail call optimization (which is not supported even by recent versions of Chrome), the only browsers that support Promise but don't support some ES6 features are:

• Android 4.4.3: supports Promise but almost nothing else
• Safari 12 - 13.1, iOS <14: doesn't support using certain Unicode characters in identifiers
• iOS <11: doesn't support scope shadowing resolution and shadowing parameters in for-in loop bindings
• Safari 12 ,12.1: doesn't do template string caching correctly (which leads to equality checks sometimes breaking)
• Edge 17, 18: Many RegExp-related features are missing, including Symbol.match, and some other ES6-y things (like proxies) don't work when RegExp objects are involved

With that, I think we should use the following feature detection for ES6:

• Check that native Promise exists
• Check that /./g.flags === 'g' this excludes Android <= 4.4.3 and Edge <= 18
• Check that eval( 'var 𐋀;') (where 𐋀 is U+102C0 CARIAN LETTER G) does not throw an exception; this excludes Safari <= 13.1 and iOS < 14.

(we wouldn't have to check for the scope shadowing and template string caching bugs, because those only affect browsers that also fail the Unicode characters in variable names test)

In T272104#6767527, @Catrope wrote:

• Check that eval( 'var 𐋀;') (where 𐋀 is U+102C0 CARIAN LETTER G) does not throw an exception; this excludes Safari <= 13.1 and iOS < 14.

(we wouldn't have to check for the scope shadowing and template string caching bugs, because those only affect browsers that also fail the Unicode characters in variable names test)

Does that actually throw a catchable exception, rather than error as unparseable? If so, can we do the same with e.g. an arrow function use to test more directly the thing we're trying to use most?

In T272104#6767669, @Jdforrester-WMF wrote:

Does that actually throw a catchable exception, rather than error as unparseable?

Yes, as long as it's wrapped in eval(). Attempting to eval() code that contains a syntax error throws a catchable exception.

If so, can we do the same with e.g. an arrow function use to test more directly the thing we're trying to use most?

We could, as long as it's also wrapped in eval. But I don't think testing for arrow functions directly is necessary, since browsers that support native Promises but don't support arrow functions, let/const, etc don't exist (aside from Android 4.4.3). This means we the minimal check would be native Promise support plus some ES6 feature that isn't supported in Android 4.4.3, and then we could use eslint rules to stop ourselves from using the new RegExp features or variable names containing esoteric Unicode characters. But those rules don't currently exist (eslint-plugin-es doesn't have the latter, at least), so I think it's easier to just add those two things to the check. We get one of them for free, because either of them can fill the "some ES6 feature that isn't supported in Android 4.4.3" slot.

The drawback here is that, if we prevented ourselves from using these (somewhat obscure) features with eslint rather than feature-detecting for them, we would not have to exclude older versions of Safari and iOS from ES6-only features, and those versions are 5.4% of global internet traffic according to caniuse.

Change 657953 had a related patch set uploaded (by Catrope; owner: Catrope):
[mediawiki/core@master] resourceloader: Allow modules to mark themselves as ES6-only

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

My patch above implements what I outlined in T272104#6750238. A module can add "es6": true to its definition to mark itself as ES6-only, and loading such a module will throw an error on non-ES6 browsers. This error is handled similarly to other module loading errors, so it fails the promise when using mw.loader.using()but is suppressed when using mw.loader.load().

This only gets us ES6-only modules (essentially creating a "grade B" implicitly). It doesn't get us actual ES6 support yet, for that we still need the new minifier.

In T272104#6770997, @gerritbot wrote:

Change 657953 had a related patch set uploaded (by Catrope; owner: Catrope):
[mediawiki/core@master] resourceloader: Allow modules to mark themselves as ES6-only

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

This seems like a huge step forward. Will users be able to set their common.js to ES6-only and skip minification?

In T272104#6779098, @Nirmos wrote:

In T272104#6770997, @gerritbot wrote:

Change 657953 had a related patch set uploaded (by Catrope; owner: Catrope):
[mediawiki/core@master] resourceloader: Allow modules to mark themselves as ES6-only

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

This seems like a huge step forward. Will users be able to set their common.js to ES6-only and skip minification?

No, not yet. Yes, it wil be after two other changes have happened.

The above commit is preparation work for making it possible to indicate that a module will target "ES6 browsers" only, thus not being loaded at all in older browsers. This change does not update the minifier. And it does not update the user script validator.

• Task T272882 is for supporting ES6 syntax in the minifier. This will be enabled for all of MediaWiki, as there is no need to have two minifiers. Whether a module will actually use ES6 syntax is a decision to be made inside the module, and not the minifier. Most of the MediaWiki and extension features will continue to use ESLint via Jenkins during development in Git/Gerrit, and will play by the older ES5 syntax for wider browser support. But after the abvove change and task T272882, there can be modules that contain ES6 code, be minified correctly, and safely load only in newer ES6+ browsers.

• Task T75714 is for updating the on-wiki script validator to support ES6 syntax. This is needed as otherwise your user script will be replaced with mw.log.error('SyntaxError') before it even reaches the minifier. Once the script validator supports ES6 syntax, I think it would be best to deploy it and enable it by default for "user" scripts, so that you can always use ES6 syntax in personal skin.js and common.js pages. For site-wide scripts that load for all users we probably want to continue with ES5 validation. After task T75714 is resolved, we can turn on the "ES6-only" marker for the "user" scripts modules in MediaWiki.

This is not necessarily something I personally recommend, but some projects load scripts from the user namespace in the MediaWiki namespace. An okay-ish example would be this search on enwiki and then searching on the page (ctrl-F) for User:. This finds for example https://en.wikipedia.org/wiki/MediaWiki:Gadget-wikEdDiff.js that imports https://en.wikipedia.org/wiki/User:Cacycle/wikEdDiff.js

Is this a concern before allowing users to turn their user scripts to ES6-only? I think I reached the conclusion "no" here because:

1. The change only concerns modules, not all user scripts like User:Nirmos/myScript.js
2. When using mw.loader.load, the imported code is no longer a module, even if it was before

Is this correct thinking? Just wanted to bring this up in case it could cause problems.

Change 657953 merged by jenkins-bot:
[mediawiki/core@master] resourceloader: Allow modules to mark themselves as ES6-only

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

Resolved?

Change 828764 had a related patch set uploaded (by Jakob; author: Jakob):

[mediawiki/core@master] Add es6 field to QUnitTestModule in extension.json schema

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

Change 828764 merged by jenkins-bot:

[mediawiki/core@master] Add es6 field to QUnitTestModule in extension.json schema

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