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

Option to use single quotes in output #111

Closed
johannesjo opened this issue Dec 5, 2014 · 17 comments
Closed

Option to use single quotes in output #111

johannesjo opened this issue Dec 5, 2014 · 17 comments

Comments

@johannesjo
Copy link

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

TriangleJuice added a commit to Antwerp-Operating-System/sass_css_js_tink that referenced this issue Feb 12, 2015
@ljwagerfield
Copy link

+1

@eugef
Copy link

eugef commented Jul 16, 2015

+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
Copy link

+1

1 similar comment
@iH8
Copy link

iH8 commented Jan 14, 2016

+1

@underscorebrody
Copy link
Collaborator

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
Copy link

iH8 commented Jan 15, 2016

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
Copy link
Collaborator

@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 #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
Copy link

iH8 commented Jan 15, 2016

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
Copy link
Collaborator

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
Copy link

iH8 commented Jan 15, 2016

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
Copy link
Collaborator

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
Copy link
Collaborator

Note for everyone interested in this feature, after #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
Copy link
Contributor

@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
Copy link
Collaborator

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 #149 is actually not an issue? I haven't run it through any testing as of yet.

@evilaliv3
Copy link
Contributor

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
Copy link
Contributor

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>

evilaliv3 added a commit to globaleaks/globaleaks-whistleblowing-software that referenced this issue Jan 19, 2016
@iH8
Copy link

iH8 commented Jan 20, 2016

@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.

evilaliv3 added a commit to globaleaks/globaleaks-whistleblowing-software that referenced this issue Jan 26, 2016
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

7 participants