-
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
Mark vars with /** @const */ pragma as consts so they can be eliminated. #928
Conversation
I wonder if this should be documentated. |
Aware of the failing test, will fix - agreed that there should be a README update as well. |
Fixes older browser support for consts and allows more flexibility in dead code removal.
Looks like a good feature. Being compatible with Closure is a plus. Out of curiosity, are you aware of the uglifyjs
|
@kzc Yes - the intention here is to create constants that may be expensive to compute (e.g. based on See this comment for an example. |
So you're depending on envify or some other compile-time transpiler to turn process.env.NODE_ENV into a constant in order for the "const" var elimination to work? |
I like the idea. Though, the implementation doesn't work - you're looking at the first statement of it's current scope, which isn't correct at all. I'm not really sure how to work around that right now, as the available tokens don't have the comments in them as far as I can see. |
@rvanvelzen In what circumstances will it break? It's working fine in the test, and after toying with some additional test cases, it seems to work identically to |
If you add a |
and it will produce the wrong result in other cases: without uglify:
with uglify with your patch:
Your patch is incorrectly applying "const" behavior to all vars in the scope. |
Understood. Still new to this project. I found some documentation, I'll go through it and fix the issue. Thanks. |
I think I found the right solution. Please review - I also made the tests much more complex to catch scope issues. |
Should tighten up the regex for the pragma:
|
Thanks for the note @kzc, adding a word boundary to the end of it catches:
but not:
etc. |
This looks really good, nice job! I'm not completely sold on the variable, but I don't really see a viable different solution. |
Mark vars with /** @const */ pragma as consts so they can be eliminated.
What does Closure accept as a valid pragma?
All of the above? Their docs show all their pragmas starting after |
@rvanvelzen In case there's an unanticipated issue shouldn't this be behind a new compress flag defaulting to disabled? |
Hmm. I don't believe this annotation to be a common thing in the wild. If it is, we can add the option later. However, even if we add an option, I'd like to default it to true. These options are highly undiscoverable by nature, and I'd like everyone to benefit by default whenever possible. |
Since there will likely be other pragmas in the future, having a compress pragma option defaulting to true would be reasonable. At least then you have the option of explicitly disabling them. |
Aside from |
That's why I made the envify comment above. If the |
I misread the question. I don't see a reason why the same |
Fixes older browser support for consts and allows more flexibility
in dead code removal.
This means that this:
will compile to:
successfully removing the dead code without requiring the
const
keyword, which is not supported in many browsers.This was born from nodejs/node#3104 (comment), essentially mimicking Closure Compiler's behavior on this. This annotation will allow us to keep the bundle ES5-compliant, pre-compute the environment (to fix Node's slow
process.env
performance), and successfully eliminate dead code in browser builds both built by the package maintainers and those consumed by webpack/browserify users who are using this version of UglifyJS2.Please let me know if I'm not on the right track with this PR - I am not very familiar with this project.
Also fixes #133.