ericclemmons / grunt-angular-templates

Grunt build task to concatenate & pre-load your AngularJS templates
MIT License
710 stars 107 forks source link

Option to use single quotes in output #111

Closed johannesjo closed 8 years ago

johannesjo commented 9 years ago

That would be nice, so I won't have to make an exception for jshint all the time.

ljwagerfield commented 9 years ago

+1

eugef commented 9 years ago

+1

Other good reason to use this is to reduce size of the generated js code.

Usually double quotes are used for html attributes and then in js code all of them are escaped. If single quotes could be used in js output then escape characters are not necessary any more.

SmoshySmosh commented 9 years ago

+1

iH8 commented 8 years ago

+1

underscorebrody commented 8 years ago

Typically I wouldn't want to support running jshint on code that's generated from another library (ie: you wouldn't run jshint on transpiled ES6 from babel) but i buy the size reduction argument and there seems to be enough interest in this feature. I'll look into it.

iH8 commented 8 years ago

I've just read that posting +1 comments can be annoying. If it's taken that way, i'm sorry. I just wanted to show that i'm also very interested in an option for this. For the same reason as @eugef mentioned: Reduction in size. I've done some testing with templates from my application and i get up to 20% less filesize when using single quotes. This is in my opinion a very significant save.

@underscorebrody i'de be delighted if you would take a look at this. I already took a peek but ran into trouble which made me suspect that the writer didn't use single quotes because it's simply impossible to implement. I'm just a mediocre JS user so it could be i'm overlooking something obvious. I'll try to explain, bare with me:

https://github.com/ericclemmons/grunt-angular-templates/blob/master/tasks/lib/compiler.js#L186

this.stringify = function(source) {
    return source.split(/^/gm).map(function(line) {
        return JSON.stringify(line);
    }).join(' +\n    ') || '""';
};

So stringify is responsible for escaping the double quotes and it appends double quotes to the beginning and end of the resulting string. So i tried the following:

this.stringify = function(source) {
    return source.split(/^/gm).map(function(line) {
        line = JSON.stringify(line);
        line = line.replace(/\\"/g, '"'); // Undo escaping of double quotes
        line = line.replace(/^"/, "'"); // Replace leading double quote with single quote
        line = line.replace(/"$/, "'"); // Replace trailing double quote with single quote
        return line;
    }).join(' +\n    ') || '""';
};

This works just as long as you don't have a template containing single quotes eg:

<div class="form-group" ng-class="{'has-error': login.email.$dirty}">

Then it breaks and i can't find a way to fix it, so i'm stuck unfortunately :( Been searching everywhere but my deduction so far is that it's impossible to implement. But as said, i could be wrong. Please prove that :)

underscorebrody commented 8 years ago

@iH8 it wasn't taken as annoying at all! I guess some people find +1 comments annoying but they don't bother me. They help to gauge interest in a feature.

I think I have a good solution in https://github.com/ericclemmons/grunt-angular-templates/pull/142. It drops the use of JSON.stringify which is probably preferable anyway because we're not actually operating on JSON data structures so we don't need to follow the rules that govern them. Lemme know what you think.

iH8 commented 8 years ago

Escape the single quotes, but ofcourse, how stupid of me :( Marvelous @underscorebrody! Works like a charm, tested it with single quotes in a template and it just works™. I vote for merge, thanks a million! ps. i personally think it should be the default option, at least, i see no reason why not. It's the better option in my opinion.

underscorebrody commented 8 years ago

I left the default as-is for now, I'd like to get it out in the wild for a bit first and make sure there aren't any edge cases that aren't working as intended. I'll probably flip it at some point though. Thanks!

iH8 commented 8 years ago

No problem, you're always welcome. Thank yourself :) I agree that waiting with changing the default is a better plan. You never now what might pop up. I've encountered no problems so far and used it on three applications. Got some ten more to go, if i run into something i'll let you know. The biggest application so far had 100KB worth of templates before this commit and now it's shaved 18KB of the total. Very nice result, i'm content. Thanks again!

underscorebrody commented 8 years ago

That's great!! FYI it has been released as 0.6.0, just in case you were switching your apps to a commit or branch or something

underscorebrody commented 8 years ago

Note for everyone interested in this feature, after https://github.com/ericclemmons/grunt-angular-templates/pull/148 lands you're going to need to pass experimental: true as an option as well. We found a number of edge case problems with this so it needs to be iterated on in a safer way.

evilaliv3 commented 8 years ago

@underscorebrody i retested the 1.0.2 and it works for me; the error i was getting is related to other reasons probably related to a more faster loading.

i would suggest for not going for the #148 and add mixed behaviors and users doing so many stuff. me and all the people here would rather prefer to support you in bugfixing and our use cases are sufficient to end in a fine solution (as probably it already is).

underscorebrody commented 8 years ago

Okay, I'm fine with that too, I just don't want to cause anyone too much of a headache.

@evilaliv3 based on what you're saying, is it safe to assume that https://github.com/ericclemmons/grunt-angular-templates/issues/149 is actually not an issue? I haven't run it through any testing as of yet.

evilaliv3 commented 8 years ago

not its definitely not an issue and already solved by #146. the other issue i spotted together with this change was due to an error in our codebase.

thanks

evilaliv3 commented 8 years ago

Just for statistical purposes i've looked on GlobaLeaks to see how many space we are saving an the compiled template.

We are passing from 336842 bytes to 329550 bytes , so we are saving %2 on a payload of 300k

Its valuable to not that results may really vary (my stats are different from the one of @iH8 that is getting 20%) in relation to the kind of quotes used in the project; in globaleaks the reesult is less valuable as we are using both single and multiple quotes where these are ineliminable like in:

<span class="text-red" uib-popover="{{'This field is mandatory' | translate}}" popover-placement="right" popover-trigger="mouseenter">*</span>
iH8 commented 8 years ago

@evilaliv3 it was the best result i've seen and i've looked into it. App has mostly templates with huge amounts of attributes, inline styling etc. with no expressions what-so-ever. That app was the reason i responded here in the first place. All the others i've redone yield about 3 to 5% so that comes much closer to the results you're getting. Still fine, all bytes matter to me.