Skip to content
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

Avoid using local names #298

Closed
sandstrom opened this issue Aug 18, 2011 · 16 comments
Closed

Avoid using local names #298

sandstrom opened this issue Aug 18, 2011 · 16 comments

Comments

@sandstrom
Copy link

Currently jade can't be minified with UglifyJS without using the --no-mangle flag. That is because some local functions are assumed to keep their names at runtime, e.g. node.constructor.name and node.constructor.toString().

I ran into this problem including jade in the rails 3.1 asset pipeline. It will simply minify all javascripts, exposing the problem mentioned above. There are workarounds, but it would be nice if jade minified just as fine most javascript libraries does.

@tj
Copy link
Contributor

tj commented Aug 18, 2011

yeah im fine with that as long as it doesn't affect anything crucial, though I dont think uglify should fuck with some of the things that it does

@rauchg
Copy link
Contributor

rauchg commented Aug 18, 2011

-1 for writing code "in such a way that it minifies". +1 for fixing Uglify

@mishoo
Copy link

mishoo commented Aug 20, 2011

though I dont think uglify should fuck with some of the things that it does

Compressing local names to single-letters is a basic goal of every JS minifier nowadays. IMO, relying on function (or variable) names at run-time is a very poor thing to do, and UglifyJS can't detect it. I didn't try, but I'm sure Closure and the YUI compressor would fail on JADE code as well.

Does it fuck with other things that it shouldn't?

One workaround would be to use eval — this way you'd force name mangling off, regardless of global UglifyJS arguments. Just add eval("0") to your code, or something. (of course, if I were you I'd remove the name dependency instead; I'm sure it must be possible.).

-1 for writing code "in such a way that it minifies". +1 for fixing Uglify

Here by "fixing" you mean "breaking". Not an option, really.

@tj
Copy link
Contributor

tj commented Aug 20, 2011

eval? nooOoO no no. function names are part of the language, I'd expect things to break if you go stripping them, besides, gzip is your friend! I don't see function names as that big of a deal personally, a much smaller deal than breaking libraries to save a few bytes

@tj
Copy link
Contributor

tj commented Aug 20, 2011

that being said the functions do not need to be inlined like they are, but it's even arguable that you shouldn't bother even serving the template engine itself, and just the compiled functions (when possible)

@mishoo
Copy link

mishoo commented Aug 20, 2011

eval? nooOoO no no

Ugly, indeed. :) I was just sayin'.

function names are part of the language

Not quite so. In fact you're lucky you can inspect them at run-time in JS; most other languages won't allow you to do this. I know it can be a handy feature, but the proper way to think about it is "it's impossible to know a function's name at run-time".

In any case, I could easily add to UglifyJS an option not to mess with function names. You'd probably be the only user :-) but what the heck, it's easy. Please open a ticket if you feel it would be useful.

@tj
Copy link
Contributor

tj commented Aug 20, 2011

who cares what most other languages let you do lol, you could say that about anything, most languages dont let you perform run-time reflection, but some do, so you utilize it.

@mishoo
Copy link

mishoo commented Aug 20, 2011

who cares what most other languages let you do lol, you could say that about anything, most languages dont let you perform run-time reflection, but some do, so you utilize it.

Let's just say it's a nasty feature, on the same level as with, eval, arguments.callee and arguments.caller. It might be useful sometimes, but that's not your case.

I took a look at your code and you have no excuse to rely on function names at run-time, other than “it's developed and tested, we're not changing it now” ;-) In fact, it makes things worse; instead of defining what's related to, say, a Block node, in a single file, you have to define it in compiler.js (visitBlock) and nodes/block.js. And you export a Block object for no reason, as it's obvious that no other app than JADE will create one. I'm pretty sure your code is longer (and maybe slower) than it needs to be.

But I might be wrong, of course, I didn't put more than a few minutes to look at your code. Just sayin'... please take it constructively, not offensively.

@tj
Copy link
Contributor

tj commented Aug 20, 2011

I'm not saying I wont change it, or that it was necessary, I'm just saying it's really silly to rewrite your code for compression, especially when ideally you save all those bytes and dont even include Jade on the client-side.

Compile-time is pretty irrelevant, like I said ideally you serve up these functions and call it a day, the initial compiled/cached function doesn't really matter, and it's exported so that the rest of Jade can access it, that's how you write Node modules :p I dont write code for the sake of brevity, that's certainly not a goal

@tj
Copy link
Contributor

tj commented Aug 20, 2011

and actually I should mention that you can (and we do) make use of the AST nodes, we do some automated translation support for the text nodes etc, and because of npm's dependency resolution you cannot use instanceof (typeof/instanceof are massive fails anyway), so you have to refer to a method or prop, in the case of Jade/Stylus currently it just happens to be the constructor's name lower-cased

@mishoo
Copy link

mishoo commented Aug 21, 2011

I checked in a sample of how I'd do it (well, designed to rewrite as few lines as possible; the patch might look big but it's just moving code around). I only did for Tag and Comment nodes; I stopped there as I don't know how to run the test suite and I was working a bit blindly.

I'm not saying I wont change it, or that it was necessary, I'm just saying it's really silly to rewrite your code for compression

agree; this is more about best practice than about compression (incidentally, best practice makes compression better ;).

@tj
Copy link
Contributor

tj commented Aug 21, 2011

trailing all you definitions with a comma is far from a best practice lol

@manast
Copy link

manast commented Apr 13, 2012

+1 to fix this. If jade is supposed to be used in client side I think it should be fixed. Furthermore, what is the point of maintaining jade.min.js when it does not work?

@tj
Copy link
Contributor

tj commented Apr 13, 2012

i havent used jade.min.js, minification is always part of our build process but if it's still broken I'll just remove it, not worth the headache of fighting uglify, or we can use something else

@mishoo
Copy link

mishoo commented Apr 13, 2012

Try something else, it's a pita to remove a feature just because UglifyJS is such a bad ass. :-p

@tj
Copy link
Contributor

tj commented Jun 17, 2012

This issue has been inactive for over 2 months so I'm closing it. If you think it's still an issue re-open. - tjbot

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants