Change 758086 merged by jenkins-bot:

[mediawiki/extensions/Gadgets@master] Add support for ES6 gadgets, but with validation disabled

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

I started updating the documentation at https://www.mediawiki.org/w/index.php?title=Extension:Gadgets&diff=5557485&oldid=5495165. I left a couple table cells blank. Feel free to fill them in.

I was looking into this task as part of T334438: Create a basic "Hello world" example of how to use Codex in a user script, and I think that we may need some solution that can work for userscripts as well – unlike Gadgets, it seems that there is no way to disable validation for userscripts.

What is more, some ES6 syntax (like const) seems to be allowed, while other things like arrow functions will still cause validation to fail.

For example, the following code will work on a user-level common.js page:

mw.loader.using( [ 'vue', '@wikimedia/codex' ], function ( require ) {
	const Vue = require( 'vue' );
	const codex = require( '@wikimedia/codex' );
	
	console.log( Vue );
	console.log( codex );
} );

This code will not work:

mw.loader.using( [ 'vue', '@wikimedia/codex' ], ( require ) => {
	const Vue = require( 'vue' );
	const codex = require( '@wikimedia/codex' );
	
	console.log( Vue );
	console.log( codex );
} );

I wonder if we should seriously consider providing a way for MW to shell out to a more modern Node.js-based validator that supports current language features. This is another area where doing the work outlined in T328699: Consider including a JS runtime as part of MediaWiki could be beneficial.

Both should work fine in User:Example/hello-vue.js which you can import from common.js with e.g. importScript("User:Example/hello-vue.js").

Most of the time you should move scripts to separate files anyway to make them more shareable.

As noted at T178356#8782595, all mainstream browsers passing current check should support ES8 (and most of ES9 except some regex-related features).

In T75714#8777659, @egardner wrote:

What is more, some ES6 syntax (like const) seems to be allowed, while other things like arrow functions will still cause validation to fail.

const was a reserved keyword back when so that's why it works, however, let (and arrow functions as you mentioned) won't work.

@Krinkle recently told me that he found https://github.com/mck89/peast/ , an ES2023 parser for PHP that can be configured to parse any ES2015 (=ES6) or any later version. This seems like a promising candidate to replace the old ES5 validator with.

Change 927797 had a related patch set uploaded (by SD0001; author: SD0001):

[mediawiki/core@master] resourceloader: Post-save validation for onwiki JS

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

Have uploaded a patch for moving the validation to a post-save update (was meant to be a POC patch, but ended up being pretty functional). This should enable us to use slower validators like Peast (T75714#7607131). It also introduces a JavaScriptValidator interface so that the implementation can be swapped for another one (such as one based on Rust or JS for WMF production).

@Legoktm pointed out that page properties are an artifact of parsing and should only be modified within the parse, so instead of validating post-save we'd have to validate synchronously during the save – which I suppose is just about fine. The other option would be to do it post-save but store the result in some cache or a new db table.

In T75714#8961708, @SD0001 wrote:

@Legoktm pointed out that page properties are an artifact of parsing and should only be modified within the parse, so instead of validating post-save we'd have to validate synchronously during the save – which I suppose is just about fine. The other option would be to do it post-save but store the result in some cache or a new db table.

I left some code review comments at https://gerrit.wikimedia.org/r/c/mediawiki/core/+/927797/ in which I arrived at the same conclusion. Thanks for working on this! Let me know if you have time in the next few weeks to iterate on this with us, or if you'd like someone to help finishing the patch on your behalf.

Change 959373 had a related patch set uploaded (by Krinkle; author: Krinkle):

[mediawiki/vendor@master] Initial audit of mck89/peast package

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

Change 962732 had a related patch set uploaded (by Krinkle; author: Krinkle):

[mediawiki/core@master] ResourceLoader: Improve and generalize validateScriptFile test coverage

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

Change 962739 had a related patch set uploaded (by Krinkle; author: Krinkle):

[mediawiki/core@master] ResourceLoader: Switch validateScriptFile() from JSMinPlus to Peast

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

In T75714#7607131, @SD0001 wrote:

https://github.com/mck89/peast […]

       JSParser::parse (jquery-3.2.1.js.gz)    Peast\Peast::ES6::parse (jquery-3.2.1.js.gz)
count:          4                                           4
 rate:      0.2/s                                       0.1/s
 mean:  4835.29ms                                  10063.21ms
  max:  5367.42ms                                  10615.39ms
Peak memory usage:   34.00 MiB                      58.01 MiB

which is disappointing – 2x slower than JSParser (JSMinPlus, the current validator). […]

I'm unable to reproduce this result. In my benchmarks Peast is about 8% (or 0.08x) slower, not 200% (or 2.0x) slower.

My code and Peast version are part of https://gerrit.wikimedia.org/r/962739.

$ php maintenance/benchmarks/benchmarkJSMinPlus.php 

Running PHP version 8.2.10 (arm64) on Darwin 22.5.0 …

JSParser::parse (jquery-3.2.1.js.gz)
   count: 10
    rate:      1.6/s
    mean:   633.58ms
     max:   637.16ms
   Peak memory usage: 34.00 MiB

Running PHP version 8.2.10 (arm64) on Darwin 22.5.0 …

Peast::parse (jquery-3.2.1.js.gz)
   count: 10
    rate:      1.4/s
   total:  6929.07ms
     max:   707.90ms
   Peak memory usage: 58.03 MiB

Change 962740 had a related patch set uploaded (by Krinkle; author: Krinkle):

[mediawiki/core@master] ResourceLoader: Remove libs/jsminplus.php (JSMinPlus, JSParser)

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

Change 962732 merged by jenkins-bot:

[mediawiki/core@master] ResourceLoader: Improve and generalize validateScriptFile test coverage

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

T347922 has been resolved. Is there anything else because of which this task should be stalled, or is it ready to be worked on?

Change 988101 had a related patch set uploaded (by Krinkle; author: Krinkle):

[mediawiki/extensions/Gadgets@master] tests: Add test case to assert ES6 is valid by default

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

In T75714#9218334, @Krinkle wrote:

In T75714#7607131, @SD0001 wrote:

...

I'm unable to reproduce this result. In my benchmarks Peast is about 8% (or 0.08x) slower, not 200% (or 2.0x) slower.

My code and Peast version are part of https://gerrit.wikimedia.org/r/962739.

I guess it's system dependent. T75714#7607131 was done on my older laptop with php inside docker. On my current setup (no docker), I am seeing that Peast is more than twice as fast!

Running PHP version 8.2.11 (arm64) on Darwin 22.5.0 Darwin Kernel Version 22.5.0: ...

JSParser::parse (jquery-3.2.1.js.gz)
   count: 10
    rate:      0.8/s
   total: 12647.02ms
    mean:  1264.70ms
     max:  1294.99ms
  stddev:    22.39ms
Current memory usage: 36.00 MiB
   Peak memory usage: 36.00 MiB

Running PHP version 8.2.11 (arm64) on Darwin 22.5.0 Darwin Kernel Version 22.5.0: ...

Peast::parse (jquery-3.2.1.js.gz)
   count: 10
    rate:      1.7/s
   total:  5746.78ms
    mean:   574.68ms
     max:   596.23ms
  stddev:     8.58ms
Current memory usage: 48.00 MiB
   Peak memory usage: 56.02 MiB

Change 959373 merged by jenkins-bot:

[mediawiki/vendor@master] Initial audit of mck89/peast package

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

Change 962739 merged by jenkins-bot:

[mediawiki/core@master] ResourceLoader: Switch validateScriptFile() from JSMinPlus to Peast

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

Change 988101 merged by jenkins-bot:

[mediawiki/extensions/Gadgets@master] tests: Add test case to assert ES6 is valid by default

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

OK, this will ship to Wikimedia production starting tomorrow (with wmf.13); worth a User-notice entry I think.

User-notice

(Edited to incorporate @AntiCompositeNumber's suggestion below.)

requiresES6 disabled validation so in practice it could also be used to add post-ES7 features. Are such gadgets going to break, or does the option still disable validating?
(At a glance the answer is that it does, just double-checking.)

@Tgr Correct. The option is not for ES8 code though, as explicitly coded into the temporary sounding "requiresES6". The Gadgets documentation makes explicit that opting in created a responsibility for developers to ensure validity on their until until we were ready to close the gap between core and Gadgets. Allowing later ES code to be served long-term creates cascading problems, there would be no documentation by default to explain this requirement or to prevent it from loading, plus, it makes the gadget module unsafe to depend on. This is not something I intend to support long-term, or rather, to support long-term would require a new option and some thinking through the ramifications and possible equity impact, especially when it is unintentional. One could easily assume that the option only exists to satisfy some infrastructure need, as opposed to creating a situation where future edits to the gadget can silently break the gadget for some people who currently use the gadget.

The option was created to cover the gap in browser requirements between core and Gadgets. Now that we use the Peast library, this gap no longer exists. Note that the Peast library already has support for ES8 through to ES2022. Future changes to MediaWiki core browser support will include, simultaneously, a change in ResourceLoader's script validator to specify the appropiate ES spec edition, thus naturally granting Gadgets always the same support level as core.

I would suggest changing the user-notice to reflect that:

... Previously, Gadgets could use the requiresES6 option. This option is no longer needed and will be removed in the future.

In T75714#9443736, @AntiCompositeNumber wrote:

I would suggest changing the user-notice to reflect that: […]

Done!

There should probably also be a task to remove requiresES6 at arbitrary later date from a valid definition and/or a task for the more-immediate term to make it a no-op of some sort.

In T75714#9443557, @Krinkle wrote:

The option is not for ES8 code though, as explicitly coded into the temporary sounding "requiresES6".

I did not mean to argue that using requiresES6 for ES8+ should be supported, only that if that parameter is dropped or people are directed to remove it, Tech News should make it clear that that isn't a no-op change. I don't think it's that unlikely that someone did use for ES8+ code, either because they did not realize the code is ES8+, or because they didn't know requiresES6 means "ES6 and ES7 but not ES8". If a gadget breaking is the gadget developer's fault, it is still helpful to point them to the cause of the breakage.

I did not mean to argue that using requiresES6 for ES8+ should be supported, only that if that parameter is dropped or people are directed to remove it, Tech News should make it clear that that isn't a no-op change. I don't think it's that unlikely that someone did use for ES8+ code, either because they did not realize the code is ES8+, or because they didn't know requiresES6 means "ES6 and ES7 but not ES8". If a gadget breaking is the gadget developer's fault, it is still helpful to point them to the cause of the breakage.

I guess I'm in the group of devs that thought "requiresES6" means ES6+.

The option was created to cover the gap in browser requirements between core and Gadgets. Now that we use the Peast library, this gap no longer exists. Note that the Peast library already has support for ES8 through to ES2022. Future changes to MediaWiki core browser support will include, simultaneously, a change in ResourceLoader's script validator to specify the appropiate ES spec edition, thus naturally granting Gadgets always the same support level as core.

I'm confused. So which version of ES is supported e.g. without using requiresES6?

I'm definitely using async/await in gadgets (ES8 / ES2017 feature). I'm now glad I didn't use static methods in an upcoming gadget 🙃 (ES2022 feature)... Should I still be using requiresES6 when using async/await? I know gadgets with requiresES6 does currently work with async/await. E.g.: https://pl.wikipedia.org/wiki/MediaWiki:Gadget-pendingChangesHelper.js

Note that async allows you to do much, much more readable code. E.g. by using await in HTML dialogs:
https://github.com/Eccenux/wiki-DYKCzyWiesz/blob/59d3b9ed4d93b6c42a59a9e7ce85b327d4bd9d64/src/DoneHandling.js#L80

I have a custom function there, but this would work as well:

await mw.loader.using( 'oojs-ui-core' );
if (await OO.ui.confirm('Are you ready for ES2017?')) {
  await OO.ui.alert(`I am ready, yes I am!`);
} else {
  await OO.ui.alert(`Nope, sorry`);
}
console.log('done')

In T75714#9458179, @Nux wrote:

I'm confused. So which version of ES is supported e.g. without using requiresES6?

ES2016 (ES7).

Assuming the analysis in T178356#8782595 is correct, all Grade A browsers support ES2017. The Wikimedia-Minify library also supports ES2017 partially, which is why you're able to use async–await in gadgets today. Whenever full support for ES2017 is added to the minifer (tracked in T277675), we can update the Peast validation to let ES2017 through as well. That would also be a good time to remove the requiresES6 option.

Change 962740 merged by jenkins-bot:

[mediawiki/core@master] ResourceLoader: Remove libs/jsminplus.php (JSMinPlus, JSParser)

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

Change #927797 abandoned by Krinkle:

[mediawiki/core@master] [WIP] resourceloader: On-save validation for onwiki JS

Reason:

Closing for now as the associated task is resolved. We might be able to re-use this in the future.

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