OpenUserJS / OpenUserJS.org

The home of FOSS user scripts.
https://openuserjs.org/
GNU General Public License v3.0
850 stars 300 forks source link

Test and support new option for *express-minify* with keeping comments #432

Open Martii opened 9 years ago

Martii commented 9 years ago

See also:


SummerWish f.k.a Breeswish was gracious enough to grant the request for enhancement and a personal thank you goes out. avdg fixed the newline issue in the UserScript metadata block with minification.... once again thank you too.


OUJS refs:

Martii commented 9 years ago

Before I get too gung ho on this... should there be an opt out in user preferences when logged in? or should we always minify on production (for user.js installs and what about the ../src routes?)?

Zren commented 9 years ago

Never minify the user.js. None of the other sites minify by default.

End users should not have to deal with minification.

Martii commented 9 years ago

Well we should be the first to offer a feature that beats out the other run of the mill sites. ;) And that's not entirely true... gf, in a similar fashion, has discussed default languages via whatever persistent storage... which this can be considered inclusive.

And yes end users should be dealing with portable device constraints. JavaScript white space can take around 20% to 30% of mass storage which is really just wasted space (this was proven with CI btw on USO).

The only reason I offer a question here is because if someone wants to back up their unminified code (say they aren't on GH for some reason) they can... but the ../src route could not minify.

See also:

Zren commented 9 years ago

first to offer a feature

Minification is not a feature for end users. It's a feature to lower bandwidth for the server owner. One of our rules is to not minify your scripts, why in tarnation would we then minify it for the user so he can't read/edit the source in his GM addon?

We could use gzip compression to transfer it to the user if bandwidth is the issue.

Martii commented 9 years ago

One of our rules is to not minify your scripts

No it's not... see https://openuserjs.org/about/Terms-of-Service#minification

We could use gzip compression to transfer it to the user if bandwidth is the issue.

We already do... see https://github.com/OpenUserJs/OpenUserJS.org/blob/f3233decd7368aeb1fa425ffbd4a24abc8395fb0/app.js#L7 and https://github.com/OpenUserJs/OpenUserJS.org/blob/f3233decd7368aeb1fa425ffbd4a24abc8395fb0/app.js#L68

why in tarnation would we then minify it for the user so he can't read/edit the source in his GM addon?

...

should there be an opt out in user preferences when logged in?

Can offer an opt in too... I am just handing the conch around with this question.

Btw haven't seen "tarnation" in a looooooong time. ;)

Zren commented 9 years ago

should there be an opt out in user preferences when logged in?

No because you assume the person will always have a logged in session, which they won't. You assume the GM addon will even send the cookies to signal there's a session, which I'm not sure about.

Er wait... who has the opt in option? An end user (see issues above) or a script auther (see issues in previous comment).

Martii commented 9 years ago

who has the opt in option?

I don't always follow to conclusion every greasyfork (gf) issue but it was under discussion last time I read for default languages (i18n or whatever silly name someone wants to use).

No because you assume the person will always have a logged in session, which they won't. You assume the GM addon will even send the cookies to signal there's a session, which I'm not sure about.

And because I maintain the GM Port on SF I know exactly what GM will do and not do currently... updating on a portable device will most likely not be logged in to conserve their bandwidth $$$ not ours... so logged out would either need something persistent in the user.js engine/browser or just default to on... which leaves logged in for opt out. I've had many months (technically years before the divestiture of USO) to think about this even before gf thought about it for translations ... but I wanted to hear some feedback before I go "gung ho" on this.

Martii commented 9 years ago

I do want you to cross-examine a similarity between your quotes here and notice something:

why in tarnation would we then minify it for the user so he can't read/edit the source in his GM addon?

...

Except chrome (and probably firefox) can unminify/prettyprint it for you.

Zren commented 9 years ago

updating on a portable device will most likely not be logged in to conserve their bandwidth $$$ not ours...

You could check UserAgents but that would be a bitch to keep track of. It would also make testing a pain in the ass. Also, they are not downloading unless there's an update. The GM addon should prompt before downloading over their data plan. We're also talking about files < 1 Mb here... If that's costing you 1% of your data plan, then get on wireless beforehand ffs.

I do want you to cross-examine a similarity between your quotes here and notice something:

One comment is about something the end user see (userscripts). The other is about shit the developer sees (who has tools to unminify). Also, most GM addons editors are a POS (textareas). Users should not have to unminify to read/edit a userscript.

Martii commented 9 years ago

An end user (see issues above) or a script auther (see issues in previous comment).

And yes that is another valid question... which I missed the first read around above... I thought of that too... serving minified by author choice could be an option too.

Martii commented 9 years ago

Users should not have to unminify to read/edit a userscript.

That's what the Source Code tab is for on OUJS and GH commits/trees/heads/branches/ etc. I didn't say minify it there... although some authors will probably still want to "game" the current 1MiB limit.

tobbexiv commented 9 years ago

That's what the Source Code tab is for on OUJS and GH commits/trees/heads/branches/ etc. I didn't say minify it there... although some authors will probably still want to "game" the current 1MiB limit.

If a user provides a script without different translations (and also does not support easy i18n) users often translate the strings of the script in the downloaded one. Also if there are bugs in the script I often check my own installed code and modify it prior to check the code on the page. So this should not be forced! What about having a second install button which says "Install for development" or only "Development" on it where you can get the unmodified version instead of providing a opt in for all pages? So the normal install is minified and developers have a second install option which is non minified?

Also I can not remeber such a discussion on GF.

Martii commented 9 years ago

What about having a second install button which says "Install for development" or only "Development" on it where you can get the unmodified version instead of providing a opt in for all pages?

A selective install button is also an option with a unique route and/or QSP... I was thinking about toying with a drop down menu type similar to installWith (if you have ever used that when USO was around) but of course with bootstraps CSS instead. e.g. http://getbootstrap.com/components/#dropdowns

A few things need to be retested here to ensure that it's still compatible to do all of this which of course can be in the dev environment.

Thank you for your feedback tobias-engelmann and great to see a newer returning voice. :)


Thinking out loud here for route possibilities (and @tobias-engelmann do any of these seem appealing?):

/install/author/*.min.user.js
/src/libs/author/*.min.js
/src/scripts/author/*.min.user.js
/minstall/author/*.user.js
/msrc/libs/author/*.js
/msrc/scripts/author/*.user.js
/installmin/author/*.user.js
/srcmin/libs/author/*.js
/srcmin/scripts/author/*.user.js
jerone commented 9 years ago

I'm against minifying the userscripts. As noted before I'm against modifying my userscripts in anyway, and minification is one modification.

The reason is that my scripts are tested when I write them and minifying alters the script after they are published. Another reason as an advantaged user, is that modifying a script is a whole lot harder when minified.

I do see advantages for users that are on their mobile devices (where bandwidth is an issue) and want to install the smallest possible. I prefer a solution that let's the user choose. The solution I would suggest is to put a (smaller) button besides the install button stating that you download the same script for mobile.

Zren commented 9 years ago

Is it even worth minifying when we have compression? Compressing ASCII already shrinks responses by a lot I would think. I would think that it would shrink whitespace pretty easy as well as repeated characters compresses well. Decided to do some tests.

Test source: https://openuserjs.org/scripts/nobodyrandom/MouseHunt_AutoBot_REVAMP Reuploaded to: https://openuserjs.org/scripts/Zren/TestMinification And minified here: https://openuserjs.org/scripts/Zren/TestMinification.min

A 100kb script unminified gets compressed to ~15-20Kb. Minified (without comments, so the real thing would be bigger) it's 50kb. Compressed it's 9kb.

This would save approximately 5Kb per script update... That's not even 1 webpage's worth of bandwidth savings.

It's not like we're transferring movies here people...

jerone commented 9 years ago

@Zren commented on 21 nov. 2014 10:57 CET:

It's not like we're transferring movies here people...

Great test. It's worth noting that your test script has around 2300 lines of code (including comments), which is a pretty decent sized userscript. To my opinion the advantages (reduced bandwidth) of minifying are too small to overrule the users ability to offline edit userscripts and other.

Martii commented 9 years ago

I prefer a solution that let's the user choose.

Agreed... opt in... which is why installWith was always opt in and I did allude to it at https://github.com/OpenUserJs/OpenUserJS.org/issues/116#issuecomment-44769613.

I'll fidget around with the UI first but I need some more current stats based of OUJS values instead of a single unit test that has just been presented... I may have to make Count Issues work with userscripts-mirror.org to pull up a more accurate test base. Some dependencies also depend on across the pond with this as well... as it sits now my tracking upstream is potentially blocking.

Martii commented 9 years ago

And I'm going to say this differently and don't pull it out context in the future... I don't care about our server bandwidth for this issue... it's primarily for storage on portable devices and if everyone rereads https://github.com/OpenUserJs/OpenUserJS.org/issues/116#issuecomment-44769355 you might get some more insight... since I'm the one that has been administrating OUJS production primarily more than anyone else (hint, hint) it would be nice to have this ability.

Martii commented 9 years ago

Here's some stats on one single lonely, active, Unit Test script that appears to be "wildly" popular ;)

Script Full Basic available minified
oujs - Meta View 2.2.10¹ 17324 bytes 12660 bytes

So let's see...

... and that's just one script that is very tiny.

Cluster/block size will always be an issue on some platforms and I had this argument with an ex supervisor back in the early 90's... and after 2 years of that debate I upgraded to FAT32 on that machine and all the sudden we didn't have to buy tiny HDDs all the time (which at the time were very pricey and costing the corporation beau coup bucks) and have one or more full SCSI array farms. e.g. he lost the debate and I also was offered his job. I understand that you all are trying to grasp this concept and I hope that you can do some further reading and learn from real world, practical, verified experience.

Martii commented 9 years ago

I would suggest is to put a (smaller) button besides the install button stating that you download the same script for mobile.

There's also more than one level of minification and there's also obfuscation possibilities... so a drop down is better suited for scaling. The default should be raw, author initiated, as I have stated since at least early 2010 with installWith. I asked the question to get your opinions since quite a few of you actually ignored it in #116 ... e.g. you all were sleeping and/or possibly slacking for approximately six months or longer... but at least some of you are here now.

Martii commented 9 years ago

Test source: https://openuserjs.org/scripts/nobodyrandom/MouseHunt_AutoBot_REVAMP Reuploaded to: https://openuserjs.org/scripts/Zren/TestMinification And minified here: https://openuserjs.org/scripts/Zren/TestMinification.min

A 100kb script unminified gets compressed to ~15-20Kb. Minified (without comments, so the real thing would be bigger) it's 50kb. Compressed it's 9kb.

Here's what those two scripts of yours compute here:

111362 B - 54967 B = 56395 B approximately == 108.75KiB - 53.68KiB = 55.07KiB

What is the difference here again and percentage? (rhetorical and granted you stripped all comments out except the metadata block which is why I said there is different levels of minification and that's one of them)


$ ls -al
...
-rw-rw-r--  1 user user  54967 Nov 21 14:45 TestMinification.min.user.js
-rw-rw-r--  1 user user 111362 Nov 21 14:44 TestMinification.user.js

$ sudo tune2fs -l /dev/sda5
tune2fs 1.42.5 (29-Jul-2012)
...
Block size:               4096
...
Zren commented 9 years ago

I remember my families first hard drive. It was 89Mb big. I managed to fill it up with super awesome MS paint drawings. Fun times. That was back when Win 3.1 was around. So over a decade ago. Storage isn't measured in Kilobytes or event megabytes anymore. It's measured in hundreds of gigabytes to terabytes. Even phone have at least a gigabyte or two of storage. I can't imagine a user using up more than 10Mb to store all their userscripts on their phone. Minification saves (in our two test cases) 30-50% storage, an upwards of 5Mb saved.

That said. Storage of userscripts clientside should be handled and solved by the browser addon. Should storage be an issue, they will probably compress the scripts, which is a lossless operation. If they use the same algorithm we use when transferring the script, it compresses to ~20% of the original size.

Marti: Regardless of whether it could be implemented it would be encouraging open source rather than encouraging authors to minify/obfuscate. It would also keep the moderators and up from getting burnt out too quickly.

Sizzle: This is good reason to implement minifying on the site. If we do it for them, maybe they'll let us get a look at the original source. I'd also give us good cause to ban minified scripts on the site since there would no longer be a valid reason to upload them.

If moderation is an issue with minified scripts, why not make a button that pretty prints the source code on the source page. It'd be a client side script to lessen the burden on computational resources.

Also should point out that a user might want to minify a script to get it under our script size limit. Though the only script that's pushing over 1Mb is probably YT Center atm.

To summarize my points.

Pros for minification

Cons for minification

Martii commented 9 years ago

If moderation is an issue with minified scripts, why not make a button that pretty prints the source code on the source page

Already have that user.js side and why do you think I've been fidgeting with Ace more and more... e.g. node and the packages we utilize (and researched the ones we don't) aren't quite there yet. You might consider following my RSS feed to be up to date on this.


Also should point out that a user might want to minify a script to get it under our script size limit. Though the only script that's pushing over 1Mb is probably YT Center atm.

same as..

although some authors will probably still want to "game" the current 1MiB limit.


Anyhow without reiterating a dead horse here... the consensus and establishing owner approval is currently at:

The question is closed and it's moving forward with 6 months plus time already given for discussion... again I can't say when because there is a bug right now but I may be creating the route for it in experimental status so it can be tracked and tested on pro in case some other weird bug shows up... We might even need to wait for node 0.11.x too for official support since ES6 isn't supported by most parsers and it won't minify as of last night... thought that was another interesting pitfall of V8.


P.S. 8 inch diskettes rule ;)


If anyone is willing to participate in the experimental phase the preferred route so far for installs has been suggested as:

/min/author/scriptname.user.js

... this offers the least impact to everyone... don't know about the other two existing routes for non-counted installs yet.

or possibly...

/install/min/author/scriptname.user.js
/src/min/libs/author/scriptname.js
/src/min/scripts/author/scriptname.user.js

... but I think I can see already an issue there on the latter... tinkering time first though.

Martii commented 9 years ago

btw this falls a little under #135 and #249 which is also a little of #262 ... and since GH can't do multiple milestones... about time to create #135 milestone.

Martii commented 8 years ago

Now that this has been in experimental status for a few weeks _(and there is interest)_ I wanted to reiterate in some different words that minification is a type of "site forking" in which OUJS via UglifyJS2 forks the script in minified form on installation. Being a little direct here for prior remarks... forking is allowed on any script... this is no different than GH... just a little more automated. Authors may at their option "unsupport" this if issues are found with minfied code but that becomes an upstream issue with UglifyJS2 as we at OUJS are a pass through for this. e.g. a messenger. If express-minify encounters an error with minification it has an error trap to return the native/raw code.

Since SummerWish is a little busy at the moment I've temporarily forked his project on OUJS and bumped the version for testing on a branch. All of my ES6 scripts minify now in dev and local pro... however those are also ancient from the defunct USO. jerone has one ES6 stated specific script and when I bump the dep to our fork he and his users can say if it's working before an update from GH. (this last part will lead into some discussion as well but one step at a time)

There are a few more options that we can set, including but not limited to, not mangling identifier names e.g. var someIdentifier = 10; becomes shortened with some other letter (back to BASIC days I guess) such as var a = 10;... this is one of the defaults we're currently using already with ES5. Currently we use all the defaults of UglifyJS2 via express-minify.

As a final note... we've been minifying all of our deps .js since express-minify was added... so it does work well so far.

tiansh commented 8 years ago

I'm not sure why, but... Try this one: https://openuserjs.org/scripts/ts/Test_Script_for_Keep_Comments It removed the comment 8

Martii commented 8 years ago

@tiansh

It removed the comment 8

Try it on a newline and see what it does... there was a newline issue that may affect this... I don't know if all types were affected... // vs /*


btw CC 0 isn't allowed on licensing... that's considered to be a type of public domain by OSI.

tiansh commented 8 years ago

@Martii ahh, i got it. it is an old issue on uglifyjs...

Martii commented 8 years ago

@tiansh Cc: @mishoo , @avdg , @fabiosantoscode , @SummerWish

So I've retested a few tests... and yes Function Expressions have some issues currently... which is unfortunate. I've disabled all of the compressor options in another test and it still wipes out the comments in certain situations.

As we initially stated make a comment on your script homepage for now... eventually I'll attempt to put in a key for the OpenUserJS metadata block that will be something like @unstableMinify or the like if an author ends up using those... until then "don't bend it where it hurts" is a good idea. I want to give UglifyJS2 every chance to contribute and test their ideas... so disabling this feature is not an option at this time... just like disabling OUJS support issues isn't an option even if someone is using another issue tracker like GH with @support.

The ES6 template strings @sizzlemctwizzle mentioned might be a better route for coding in general. I have not tested those yet with this feature as the harmony branch hasn't been activated on OUJS yet and is only local in my dev right now. UPDATE: Tested with this... does work however not full browser compliment support.

Please remember this feature enhancement is still experimental and when I get to the point where I can dig into UglifyJS2 a bit I might be able to add a new perspective but I'll have to learn their system more. I'm still just barely working/learning with the current API that has been exposed.

With time things can change... and I know some people are afraid of change.

Ref:

Martii commented 8 years ago

@tiansh Cc: @jesus2099 _(some of your UserScript metadata block wraps apply to this as well ex. here)_

I have roughed in the feature in for your edge use case. I would suggest using something like this OpenUserJS metadata block template for what you need... this is now active on production. Please note this is not inclusive to the UserScript metadata block.

As I mentioned on OUJS production putting in a reason improves your users experience as to why something may or may not be useful to them... plus we can track down if the dependency fails with some good explanations... so I recommend something like the below message:

// ==OpenUserJS==
// @unstableMinify Multi-line string comments using Function Expressions
// ==/OpenUserJS==

General format is // @unstableMinify some brief reason string and only one, last, key is currently used as per GM specifications, in the OpenUserJS metadata block. e.g. most people won't need a novel.


Currently used in the wild:

Martii commented 6 years ago

Just a note here RFC 2606§3 - ES6 Template String Unit Test fails with minification now. e.g. the multi-line string is removed. This used to work with UglifyJS2.

breezewish commented 6 years ago

Note: express-minify was upgraded to v1.0 several weeks ago, and there are many breaking API changes (nearly all interfaces are refined). You may want to take a look at the new README to make adaption.

Martii commented 6 years ago

I don't think it's express-minify (see notes at #1160 for when I converted it to your release)... we're using uglify-js for system and uglify-es for script sources. I don't really trust the -es branch at the moment enough to put it on system minification... this is a prime example although haven't tested it with -js branch yet.

It's possible they added a switch/property but seems to me that they shouldn't be transforming multi-line strings to nothing by default. I'll roll back the ES line a bit and see where it was broken when I get a chance.

Thanks for the visit and encouragement. :)