-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[ES1-7] return
outside of function
#935
Comments
Are you using the harmony branch? If not, which version of uglifyJS are you running? |
This is applicable for ES1 through ES7 as @mishoo wanted in the subject... doesn't currently matter if it's the harmony branch... e.g. happens in current release as well. The CLI is referenced with the commit above but I think it was missed in the API version of this project. :\ |
Also, I just threw the code in the browser console. Chrome doesn't accept it either. So I'm not sure what to do since it seems to be a non-standard syntax. Need to dig in the specs. |
The commit in the CLI is f36a1ea but seems to be absent in the API. We are getting several dozen of these "errors" and GM handles them as "warnings" depending on if a
I'll try TamperMonkey with Chromium in a moment but I seem to recall it wraps too. |
I see the spec does require return statement to occur in functions. So the best thing you can do is to wrap the code. |
At least I hope it's not about a lot of code :-) |
We can't do that if it's a library script or something that is "export"ed into the sandbox. (also depends on .user.js engine too)
As I stated it's quite vague in the specs and got looser as ECMA-Script evolved... the implied function caller is always present in all browsers... the ES committee needs to strict'ify this a bit more in ES7... but until then it's valid syntax except for ES1. |
It's quite good defined in the grammar once a geek with above average understanding can read it. So there is no room for argument about that once the understanding is on the same level. Note: es6 syntax grammer below
|
Passes as valid code in Chromium Version 47.0.2526.106... which is where Chrome gets their code from. ... so ....
is invalid. (just stating a fact here so please don't get upset because I'm super literal) |
Running chrome 47.0.2526.111 m (64-bit), pressing f12, pasting code, not working here. |
Tampermonkey may wrap the code as a function, so that might make contextual difference (so that's chrome respecting the grammar on the parsing side because TamperMonkey probably injects code that way) |
This portion could have been omitted though... I respect this project and you for what you've done but please, oh please, don't do passive-aggressive statements... anyhow enough of that off-topic reply to your response. I spent the weekend drafting and rereading the specs and what @mishoo did was to allow the option in the CLI but it needs to be present in the API otherwise OUJS may have to drop any and all support of UgliflyJS2... which I am hoping isn't the case. I haven't even drafted up the next issue yet which is something you'll probably respond with similarly. |
Note that I didn't take a decision in the UglifyJS situation yet, I'm just making sure we're on the same page. |
But at the same time it wouldn't be interesting to output code with return statement in global-ish scope that ends up browsers saying that the code is invalid (though it's not likely we would encounter this phase by compressing the code anyway). |
So, this is possible with |
Thanks.... Perhaps for a seasoned collaborator though... but as I pointed out to one of our users I'm just barely entering into this package directly... so a PR would be bound to be flawed or hit the wrong chord with someone since I don't fully have "the larger picture" of the logic of the code itself and the flow of things here just yet. There are some advanced items I haven't even tested yet but I did go looking for them. When a new collaborator comes into the fold there is bound to be some learning curves. That being said... I'll see what I can do for an option when time permits outside of OUJS. I don't have the luxury of a single component project to maintain... I have 3 major duties alone just for OUJS production and development of OUJS so that makes more. The harmony branch is where this is most needed for the experimental test of UglifyJS2 and it will probably be there that I concentrate ... since there is release vs harmony. Cc: @fabiosantoscode |
I doubt that, since it's all pretty straightforwardly implemented in the cli right now. There's 2 relevant locations:
... But maybe I'm reading this all completely wrong, and those files point you to exactly what you need? |
The CLI isn't organized in the same representational logic as the API though... which is why I mentioned both API Objects above under suggestions e.g. I don't know if this would be a generator option, compressor option, or at the "top-level" where |
Some scribblings (analysis notes that may change) out loud for this. Taken from the README.md and also code comments e.g. a consolidation...
vs
So my guess is that This tells me to "match" as close as possible to the current project maintainers:
|
@rvanvelzen Cc: @mishoo , @fabiosantoscode and anyone else who cares to chime in This is going to get way confusing here on how to do this... OUJS has a fork of upstream here... and as a rule of thumb, so far, if I fork our upstream personally (which is preferred)... I'd want to incorporate that into OUJS's fork so we could use it right away... in which case upstream here would never see the PR I think... then I'd have to merge it into the harmony branch and the harmony-named branch. For upstream here a git remote would need to be added I think to the OUJS fork to grab the PR there... but never done it this complicated before. Any ideas on making this simpler with the above requirements? |
Returning outside of a function is really not per the spec. It's only possible in node (since it wraps its modules in a function) but imho it's a bug. But! I have been known to use this to early-return from browser-only node modules, for example, and it turns out to be pretty useful. Tbh uglifyjs shouldn't concern itself with imposing the restriction of a return being inside a function, and although I haven't checked, the change is probably just the removal of an assertion. Whether to do this, however, is certainly not my decision, and neither is defining what uglifyjs should do in the face of invalid constructs. However a workaround exists, which is to wrap the module in a function declaration before minification and remove it afterwards. |
Yah that's what I've seen it used with... I usually IIFE my Userscripts going way back and never see this... but asking everyone on the planet to do this from our standpoint won't travel very far... especially with library scripts that are pre I've monkey hacked it into our node_modules folder harmony branch (a massive 2 new lines total for change besides the README.md) and using the data analysis from above it works!: // ==UserScript==
// @name RFC 2606§3 - Hello, World!
// @namespace http://localhost.localdomain
// @description Test minification with return statement
// @copyright 2007+, Marti Martz (https://openuserjs.org/users/Marti)
// @license GPL version 3 or any later version; http://www.gnu.org/copyleft/gpl.html
// @license (CC); http://creativecommons.org/licenses/by-nc-sa/3.0/
// @version 0.0.0
// @author Marti Martz <somewhere@example.com> (https://openuserjs.org/users/Marti)
// @include http://www.example.com/*
// @include http://www.example.net/*
// @include http://www.example.org/*
// ==/UserScript==
// ==OpenUserJS==
// @author Marti
// ==/OpenUserJS==
var condition=!0;if(condition)return;console.log("I am going to be doing something else now"); ... so until GM/TamperMonkey/and the plethora of other extensions/add-ons that do Userscripts don't wrap as the
There are too many possibilities on how it would be wrapped e.g. unpredictable... sometimes the shebang, which is added, may or may not be added to the function and the transformation isn't static from what I've seen... already thought of that too that one weekend.
Agree ... this should be a pass through imho... mapping the existing |
So there's the code reference... time to see what chords this twinges. :) |
Cleaned up the commits a bit more in a new PR... same code though with some README.md enhancements... a few hours of RnR help for clarity. As I forewarned fabiosantoscode I'm a bit noisy, this time due to learning curve and a few tired mistakes... so apologies again and hopefully I have this complex forking system down a bit better. :) |
@rvanvelzen @fabiosantoscode |
@Martii the harmony branch on this repository is not under my control, it's usually the maintainers who dilligently merge master into it once in a while <3 |
@fabiosantoscode |
Can this issue be closed? |
So we've got a few failures in minification already... this report is the first one, tested and confirmed.
A great way to circumvent by throwing a monkey wrench into UglifyJS2 (with the API) via Greasemonkey (GM) is to use the following script:
... which with our error trap will return this:
With GM there may be a wrapped function call depending on version and in most browsers there is always an implied "caller". With some versions of recent Mozilla based browsers the caller is usually a restricted privileged application in which scoped
caller
will be forced tonull
orundefined
for the scope of the script depending on injection.Work around:
return
statement and/or optionally placed in an IIFE (both the preferred Mozilla style and Chrome style included) however however it may be inconvenient for some and not fully backward compatible... and definitely a way to destroy the feature.Suggestion_(s)_:
External refs:
caller
)Historical refs:
TIA
Cc: @fabiosantoscode and @avdg (mishoo will always get these as owner so omitting)
The text was updated successfully, but these errors were encountered: