Page MenuHomePhabricator

JSMinPlus (user script validator) should allow reserved words in dot notation (ES5)
Closed, ResolvedPublic

Description

I am trying to use reactjs in my language wiki now that it can be used without classes using their brand new hook API (and here is a demo if you are curious https://fa.wikipedia.org/?withJS=MediaWiki:Experimental-react-demo.js not using ES6 so being compatible with our modules system) however resource loader sees something incompatible apparently when I issue mw.loader.using('ext.gadget.experimental-reactjs') which essentially tries to minify https://fa.wikipedia.org/wiki/%D9%85%D8%AF%DB%8C%D8%A7%D9%88%DB%8C%DA%A9%DB%8C:Gadget-experimental-react.js and gives JavaScript parse error: Parse error: Unexpected token; token 3 expected in file 'MediaWiki:Gadget-experimental-react.js' on line 27 which doesn't make sense as the source doesn't use ES6 features unconditionally. Can you see what is going on? Thanks!

Event Timeline

Ebraminio created this task.Feb 5 2019, 4:02 PM
Restricted Application added a project: Performance-Team. · View Herald TranscriptFeb 5 2019, 4:02 PM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript
Ebraminio renamed this task from MediaWiki's minifer compatible is not compatible with ReactJS source to MediaWiki's minifer is compatible is not compatible with ReactJS source.Feb 5 2019, 4:03 PM
Ebraminio added a subscriber: Ladsgroup.
Ebraminio renamed this task from MediaWiki's minifer is compatible is not compatible with ReactJS source to MediaWiki's minifer is not compatible with ReactJS source.Feb 5 2019, 4:05 PM

This would be T75714: Allow ES6 syntax features in gadgets and other site-wide scripts, unfortunately. Patches are very welcome, but it's a seriously hard problem; our ES3/5 PHP minifier is astonishingly quick, and experiments making it compatible with ES6 syntax have each made it unreasonably slow.

Krinkle claimed this task.Feb 5 2019, 5:14 PM
Krinkle added a subscriber: Krinkle.

The 0x notation is what it is failing on. That's a relatively new syntax feature, but indeed unlike the binary and octal number literals, the hex literals existed in ES5 as well.

We probably overlooked this when auditing for ES5 features. Will fix.

Thank you Krinkle. I've converted hex numbers to decimals but I still see some issue. Ideally I want to apply such fixes locally for a while and being able to not care about those fixes on updating.

I mean apparently line is "var hasSymbol = typeof Symbol === 'function' && Symbol.for;" which is weird. Sorry that I don't have a local installation of MediaWiki right now to check this better, I am trying to get jsminplus.php and run it independently to see whether I can improve it or fix the script issues.

Great, I was able to fix all the issues here https://fa.wikipedia.org/w/index.php?title=%D9%85%D8%AF%DB%8C%D8%A7%D9%88%DB%8C%DA%A9%DB%8C:Gadget-experimental-react.js&diff=25445722&oldid=25445607 the issue is jsmin raise error when using reserved keywords as property, like .delete .default .for and others. Please make that work also if possible. Thanks

Ebraminio added a comment.EditedFeb 5 2019, 6:26 PM

Just to note use of reversed words is ES5 not ES6 as https://stackoverflow.com/a/17911822 In ECMAScript, starting from ES5, reserved words may be used as object property names "in the buff". This means that they don't need to be "clothed" in quotes when defining object literals, and they can be dereferenced (for accessing, assigning, and deleting) on objects without having to use square bracket indexing notation.

Thanks for all your helps. Next I should develop some gadget with reactjs now I've imported with lots of hassle :) (just to note, I keep try to avoid ES6 as I want to keep it RL compatible)

matmarex removed a subscriber: matmarex.Feb 5 2019, 7:49 PM
Krinkle renamed this task from MediaWiki's minifer is not compatible with ReactJS source to JavaScriptMinifier should allow reserved words in dot notation (ES5).Feb 5 2019, 8:35 PM
Krinkle moved this task from Inbox to Confirmed Problem on the MediaWiki-ResourceLoader board.

@Ebraminio Thanks, I'm familiar with that being part of ES5 and we did actually add support for that a while ago.

Examples from the unit tests:

var a = { true: 12 };
a.true = 12;

However, it would seem we missed a few cases. Will look into it! Also, if you're comfortable contributing to the PHP code base, patches welcome!

Krinkle renamed this task from JavaScriptMinifier should allow reserved words in dot notation (ES5) to JavaScriptMinifier should allow all reserved words in dot notation (ES5).Feb 5 2019, 8:38 PM
Krinkle triaged this task as Medium priority.Feb 5 2019, 8:41 PM

Triaging as normal given this isn't a regression.

Krinkle renamed this task from JavaScriptMinifier should allow all reserved words in dot notation (ES5) to JSMinPlus (user script validator) should allow reserved words in dot notation (ES5).Feb 5 2019, 8:42 PM
This comment was removed by Ebraminio.
Krinkle removed Krinkle as the assignee of this task.Feb 7 2019, 3:30 AM
Krinkle lowered the priority of this task from Medium to Low.
Krinkle moved this task from Inbox to Backlog: Small & Maintenance on the Performance-Team board.

Lowering priority for the time being given other commitments and priorities. If and when resourcing is available, will probably go into T75714 instead. However, I'll keep this task open for anyone to work when they have time.

Ebraminio claimed this task.EditedFeb 7 2019, 8:34 AM

Ok. I experienced a bit with it and it looks fun so let me claim it :)

Interestingly if I give this as you said

var a = { true: 12 }; a.true = 12;

Or run this on a temp folder,

wget https://github.com/wikimedia/mediawiki/raw/master/includes/libs/jsminplus.php
php -r 'require("jsminplus.php"); echo JSMinPlus::minify("var a = { true: 12 }; a.true = 12;");'

It fails actually even the fact we see that on the unit test. Do you know what has went wrong here @Krinkle?

This comment was removed by Ebraminio.
This comment was removed by Ebraminio.

Change 491883 had a related patch set uploaded (by Ebrahim; owner: Ebrahim):
[mediawiki/core@master] Make JSMinPlus allow reserved words as property name (ES5)

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

@Ebraminio We have indeed, unfortunately, two parsers/minifiers currently.

We use JavaScriptMinifier for actual minification of all code served to users.
We use JSMinPlus only to validate (not minify) code submitted by users in the form of gadgets and user scripts. If code appears invalid, it is replaced with a simple "mw.log.error(errorMessage)" statement, so that it can still be safely concatenated and bundled with other code.

The reason we have both, is because JavaScriptMinifier is fast and can scale to what we need for all JavaScript code we serve (only a small portion is user-generated). JSMinPlus, on the other hand, is slow, but provides much better error messages. So we only use it for user-script validation.

Ideally, JSMinPlus would be removed in favour of JavaScriptMinifier being able to serve both needs in a way that won't compromise its speed.

Change 491883 merged by jenkins-bot:
[mediawiki/core@master] resourceloader: Make JSMinPlus allow reserved words as property name (ES5)

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

Now it makes sense. Thanks!

Krinkle closed this task as Resolved.Apr 7 2019, 12:31 AM

My new tiny tool based on react (didn't go rewrite my old ones yet), https://fa.wikipedia.org/wiki/%D9%85%D8%AF%DB%8C%D8%A7%D9%88%DB%8C%DA%A9%DB%8C:Massblock.js having jsx around could be huge boost, still, is nice to have react around.