-
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
ufuzz failure #4598
Comments
@kzc another day, another spec quirk... $ echo 'if (f) function f() {}' | node
$ echo 'if (f) async function f() {}' | node
[stdin]:1
if (f) async function f() {}
^^^^^
SyntaxError: Async functions can only be declared at the top level or inside a block.
at createScript (vm.js:80:10) |
This is a case where the spec makes sense. |
In what way? All I see is yet another instance of disregard for consistency and compatibility. |
I thought a scoped declaration in a blockless
But traditional function declarations are grandfathered due to ES3:
I suppose there's a case for consistency, but I think they were trying to correct an historical oversight. |
But they did correct this mistake apparently...
|
You can see that the decl is no longer function scoped in recent V8 engines:
|
Thanks for confirming my suspicions 😅 Those subsequent deviation from JavaScript is why I mentioned "disregard for compatibility" earlier. To summarise − they poked a hole, fell into it, then decided the best course of action is the keep digging. Good times. |
That may be true, but the latest JS engines converged on a new set of scoping rules that uglify-js is not entirely compatible with:
An |
I'm very much against fragmenting In this particular case, I'm currently leaning towards the fact that people almost never wrap |
Not sure what "proper JavaScript behavior" means. It's an evolving standard. No browser built in the last few years supports ES5 scoping rules. I don't see a way to support old scoping rules and modern scoping rules without an ecma option of some sort - they are fundamentally incompatible. |
If you mean runtime behaviour, yes − but don't forget we aren't evaluating the input code most of the time. Just see how we currently handle |
There are browsers built in the last few years, and then there are ones that people actually use. They are not necessaily correlated, especially when the former keeps evolving away useful features & functional behaviours... 😏 |
How do you propose to handle minification of cases like #4598 (comment) so they work in both old ES5 browsers and in modern ES6+ browsers when they produce different output? Leave the AST as is? An ecma option would be a lot easier. |
I think we are considering this from different angles − you see it as each code paths being simpler, whereas I see the permutations of code paths is now multiplied by at least a factor of two. |
If the ecma target is unknown then you cannot drop unused code in certain cases. Hypothetical example:
|
Ultimately if we want to support the broken behaviour in ECMAScript, we'll do it just like how we handle However, there is also the function variable not being assigned until executed over − except if you are within the same block scope in which case it behaves just as expected before. The latest ECMAScript is very broken − they've made |
The reason why this has not been worked on is a pragmatic one − a lot of the existing code will need to be modified (introducing a new flag would be way worse in terms of code churn), and it's not something that we've encountered in the wild. If you want to have a go at it, be my guest − but it really is messy. I'd know since I tried that for |
These edge cases are indeed rare. It was not intended as a criticism. I was just arguing for whatever behavior the majority of browsers in use support. Whether you choose to implement it is entirely your call. I'm out of the minification game. Just an armchair spectator now. |
The text was updated successfully, but these errors were encountered: