miracle2k / webassets

Asset management for Python web development.
BSD 2-Clause "Simplified" License
924 stars 259 forks source link

Join Javascript files using ';' instead of a newline #100

Closed mvantellingen closed 12 years ago

mvantellingen commented 12 years ago

Hi,

The bootstrap 2.0 js files minified with rjsmin throw an error: "Uncaught SyntaxError: Unexpected token !"

Problem is the !function() {} syntax

miracle2k commented 12 years ago

Have you reported this to the rJSmin maintainer?

mvantellingen commented 12 years ago

Hey,

Sorry for the short description. I've also tried uglifyjs which also doesn't seem to work. Something weird is going on but i'll try to find out if it really is an issue in webassets or something else.

sean-lynch commented 12 years ago

Running into the same thing. It looks like it's an issue with Bootstrap 2.0's use of ! to define self-calling functions instead of ( ). YUI's compressor seems to handle it just fine so that's a short term fix.

I'm assuming you're bundling with other js files. If the JS file before bootstrap in the list ends in a semi-colon, that should also solve the problem.

sean-lynch commented 12 years ago

Scrap my second line, the semi-colon is not a general work around.

sean-lynch commented 12 years ago

Yep, confirmed as an issue with Bootstrap and jsmin. I've asked them to fix it: https://github.com/twitter/bootstrap/pull/266

miracle2k commented 12 years ago

I see, so the problem is not the ! per se, but the lack of a semicolon at the end of the files, which results in potentially invalid Javascript code, depending on how the next file begins. The reason then, I suspect, why YUI might work in some cases when rJSmin or UglifyJS do not, is that YUI is probably smart enough to recognize cases where the linebreak that webassets inserts is of actual importance as a separator, whereas these others remove the linebreak.

However, as is pointed out here:

https://github.com/twitter/bootstrap/commit/68605bdd51760a929cc661607f06f479c53b0bee#commitcomment-620727

there are cases where even a linebreak will not be enough.

I don't think bootstrap will fix this - the decision to remove the semicolons again was quite concious. They provide preminified versions instead. While I am not using bootstrap myself, I wouldn't find that satisfactory, since I like to use uncompressed third party libs in development.

Which leaves the question whether webassets shouldn't simply insert a semicolon between Javascript files. If I understand the Javascript Syntax rules correctly (a lone semicolon is a valid statement), then this shouldn't cause any problems, right?

sean-lynch commented 12 years ago

I'm with you in that I prefer the uncompressed third party libs.

Exploring the Bootstrap issue more, it looks like the issue isn't the semicolon between Javascript files, it's because bootstrap has several top level modules in the same file. Webassets would have to re-write the entire file in order to get it to pass. Seems like that might be out of scope?

miracle2k commented 12 years ago

I don't think so - there is nothing special about the way the bootstrap Makefile joines the file. They are simply using uglifyjs, which actually seems to be able to deal with this situation properly - rJsMin indeed is not.

Truth to be told, I don't expect this to be fixed in rJsMin. It might actually be not that easy even for a regular expression based parser, no idea.

I have just added a way to customize the separator. This is mainly for testing now, and may not stay, or not in this form:

bundle = Bundle(....)
bundle.joiner = ';'

This will join the source files using a semicolon, which I believe should help. The question is whether there are any side effects. I'll be using a semicolon for my JS bundles from now on for testing, and we'll see.

sean-lynch commented 12 years ago

Just a heads up, looks like Bootstrap is going to add semicolons back in the next version: https://github.com/twitter/bootstrap/pull/266#issuecomment-3954075

stewartadam commented 6 years ago

For those finding this today, bundle.joiner no longer exists and the functionality was merged into the concat filter method instead. You have to implement it like this:

class ConcatFilter(Filter):
    def concat(self, out, hunks, **kw):
        out.write(';'.join([h.data() for h, info in hunks]))

js = Bundle(
    'a.js',
    'b.js',
    filters=(ConcatFilter, 'jsmin'),
    output='app.min.js'
)

I ran into this as two Immediately Invokable Function Expression (see here) in different files without a semicolon to separate them cause a Uncaught TypeError: (intermediate value)(...) is not a function.