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

Optionally remove whitespace and HTML comments when compiled #49

Closed
kalebdf opened this issue Jul 23, 2012 · 8 comments
Closed

Optionally remove whitespace and HTML comments when compiled #49

kalebdf opened this issue Jul 23, 2012 · 8 comments

Comments

@kalebdf
Copy link

kalebdf commented Jul 23, 2012

I propose an option, removeWhitespace, to remove whitespace characters including new line characters (\n or \r or \r\n) and HTML comments <!-- -->.

These characters take up space within the compiled template strings.

@SlexAxton
Copy link
Owner

Interesting idea. Though it could prove to be extremely difficult to get right. gzip handles alot here. Any ideas on how it would work?

@kalebdf
Copy link
Author

kalebdf commented Jul 23, 2012

Before I started using this plugin, I used a grunt task to compile all Handlebars templates into a window.JST object.

I stripped out the whitespace and HTML comments using the following code.

/* Inside a grunt task */
      var templateFunction =
        require("handlebars").precompile(file.read(filepath));

      // Remove unwanted line ending characters \r\n or \n from compiled templates
      templateFunction = templateFunction.replace(/\s*(\\r\\n|\\n)+\s*/gm, '');

      // Remove unwanted HTML comments from compiled templates
      templateFunction = templateFunction.replace(/<!--[^>]*-->/gm, '');

@SlexAxton
Copy link
Owner

Right but this changes the rendering of the markup, doesn't it?

templateFunction = templateFunction.replace(/\s*(\\r\\n|\\n)+\s*/gm, '');

Wouldn't we want it to be:

templateFunction = templateFunction.replace(/\s*(\\r\\n|\\n)+\s*/gm, ' ');

Things like

Rating:
  <span class="rating">4</span>
  <span class="delimiter">/</span>
  <span class="max-rating">5</span>

Is an example I've personally run into recently. When you strip whitespace, there are no spaces between the output 4/5 - but if you kept them in, it'd be 4 / 5.

You'd also want to handle tabs.

I'd love to see like 10 templates with the whitespace stripped and 10 templates with it all there and see the gzipped difference. In my testing in my app, I didn't implement this because it added unpredictability of spacing, but it only saved us 40 bytes in a 3 KB response after gzip.

@kalebdf
Copy link
Author

kalebdf commented Jul 23, 2012

For your first point, if I need that space, I put it within the span.

The size difference between inclusion and exclusion of comments is extremely minimal when gzip'd versions are compared. Totally agree!

I have a requirement that certain comments must exist in every file. Comments such as:

<!-- Copyright that is huge... -->

I've seen "poorly" written jQuery plugins that fail .next() & prev() calls when comments are introduced, say in-between <li> elements of a <ul>. Of course, the plugin could do .next('li') etc.

@SlexAxton
Copy link
Owner

I'd love a pull request with an option for each of these - including no space and one space replaces.

Specifically, it'd be like 3 if-statements here: https://github.com/SlexAxton/require-handlebars-plugin/blob/master/hbs.js#L380

That's the output function string like you were modifying in grunt.

People have added an hbs section to the config object, so you could add a whitespace : 'onespace' || 'nospace' and a comments : 'strip' option inside of there and read them out of the config object that is available to you on that line.

Seem good?

@peol
Copy link
Contributor

peol commented Oct 17, 2012

I'm also worried about this, if added, please set it as false by default. There's a lot more issues with this. display: inline-block; loves to do crazy things. <pre> tags must be preserved etc. I'd rather see we exposed pre-compile hooks so users can do whatever they want with the template source before it gets thrown into the compiler.

@SlexAxton
Copy link
Owner

Closing. Optional patch welcome.

@johtso
Copy link

johtso commented Apr 14, 2013

Relevant Handlebars.js issue: handlebars-lang/handlebars.js#336

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

4 participants