Consider indentation and remove newlines generated for "empty" lines #141

Lasering · 2017-06-30T15:39:17Z

Given this template:

@(list: List[String])

<html>
  <body>
    @if(list.isEmpty) {
      <h1>Nothing to display</h1>
    } else {
      <h1>@list.size items!</h1>
    }
  </body>
</html>

The generated html will be:



<html>
  <body>
    
      <h1>2 items!</h1>
    
  </body>
</html>

Notice that the h1 tag has an extra indentation level and it is surrounded by empty lines. This is happening because of the @if. Twirl could be smarter (and way more awesome) if it would remove a level of indentation from the branches of the if.

It could also remove the generated empty lines from the if. Please note that if the if was declared like this:
@if(list.isEmpty) { <h1>Nothing to display</h1> } else { <h1>@list.size items!</h1> } no newlines are created.

The text was updated successfully, but these errors were encountered:

marcospereira · 2017-07-03T18:39:00Z

We could possibly trim the empty lines (a pull request would be good if you have the time).

But I don't think we should touch indentation:

It is not really super useful
Should we consider the indentation tabs or spaces? How many spaces?
What if the user had not added a level of indentation?

In other words, too many things to consider and not much value added, IMO.

helllamer · 2017-07-03T19:11:10Z

Just strip out all useless spaces during play stage including identation is enought.

For example, i'm using grunt with replace task against twirl templates for minification during stage (but buggy play-yeoman is a real pain). This is parts from fat Gruntfile.js:

...
grunt.initConfig({
  //...
  replace: {
    txtmin: {
	src: ['<%= yeoman.dist %>/**/*.scala.txt'],
	overwrite: true,
	replacements: [
	  {from: /@\*.*?\*@/mg, to: ''},    // strip twirl comments.
	  {from: /^[ \t]+/mg, to: ''}	    // strip spaced BOL offsets
	]
      },
      
      csstplmin: {
	src: ['<%= yeoman.dist %>/**/*Css.scala.txt'],
	overwrite: true,
	replacements: [
	  {from: /^\s+/mg, to: ''},	    // BOL offsets, empty lines
	  {from: /}\s+}/mg, to: '}}'}
	]
      },

      htmlmin: {
	src: ['<%= yeoman.dist %>/**/*.scala.html'],
	overwrite: true,
	replacements: [
	  {from: /^\s+/mg, to: ''},	    // strip BOL offsets
	  {from: /\s\s+/mg, to: ' '},	    /* strip 2+ whitespaces */
	  {from: />[\n\r]+/mg, to: '>'},    /* strip newlines after html tags */
	  {from: /[\n\r]+}/mg, to: '}'},
	  {from: '\s+/?>', to: ''}
	]
      },
      jstplmin: {
	src: ['<%= yeoman.dist %>/**/*.scala.js'],
	overwrite: true,
	replacements: [
	  {from: /^\s+/mg, to: ''},	    // strip BOL offsets
	  {from: /\s\s+/mg, to: ' '},	    /* strip 2+ whitespaces */
	  {from: />[\n\r]+/mg, to: '>'}     /* strip newlines after html tags */
	]
      }

  }

});

grunt.registerTask('build-dist', [
    // ...
    'replace:tplmin',
    'replace:csstplmin',
    'replace:htmlmin',
    'replace:jstplmin'
];

Why? HTML templates generates a tons of useless spaces from identation, and mobile-device users suffering from more traffic, more parsing. Using play-html-compressor is ok for simple cases, but bad for streamed responses (html-compressor concating everything), and totally useless for chunked responses (unsupported).

Lasering · 2017-07-03T19:14:14Z

@marcospereira The indentation is easy to work around. Just keep track of the spaces and tabs of the if and remove the same amount of indentation on the branches. That will cover 99% of the cases. If this heuristic fails revert to the original implementation which does not touch the indentation.

@helllamer You have a different concern, what you want to achieve is minification. There exist already many tools capable of minifying HTML, CSS, JS, etc.
But I do agree with you, HTML template engines generate tons of useless spaces. But implementing what I am suggesting we would be removing most of the useless indentation spaces.

helllamer · 2017-07-03T19:20:46Z

you want to achieve is minification

Yes and no. Full minification of twirl templates is not possible, because they are stops to compile. My solution with "zero identation" and it is related to $subj: If someone will start to magic around twirl output identation, it will be good to implement identation using "" (empty strings, tweakable via sbt).

There exist already many tools capable of minifying HTML, CSS, JS, etc.

Yes, but for play-framework really where only one working for-years partial solution, and several hand-written crunches over grunt/gulp/bash/etc.

Lasering · 2017-07-03T19:33:07Z

Does https://github.com/sbt/sbt-uglify solve your problem?

helllamer · 2017-07-03T19:37:36Z

@Lasering uglify is only for assets pipeline, not for identated twirl templates.

I have no problem, only thing i've saying: if someone will implement custom ident in twirl-generated files, it will be good to also allow for "zero identation".

schmitch · 2017-07-03T19:51:24Z

well I think if somebody from the community will step up and do this, there is no reason to don't include it. But i don't think that this should have any priority.

Edit: and btw. there is no way of streaming/chunk twirl templates. or to say it differently. you would still have the whole template in memory since render()/apply() will return the full template string.

AlbaroPereyra · 2017-09-07T22:09:22Z

Indentation aside, in my humble opinion, I believe the lines created by twirl are annoying. Who really writes twirl templates without line breaks {code here}{some more code here}.

I created this patch for the Play Framework. It might be helpful when addressing this issue.
def prettify(content: Html): Html = { Html(content.body.trim().replaceAll("\\n\\s*\\n", "\n")) }

JackyChan · 2017-09-14T11:03:57Z

This issue also bothers me. My template just like

@helper.form(action = routes.HomeController.loginPost()) {
    @helper.inputText(loginForm("username"))
    @helper.inputPassword(loginForm("password"))
    <input type="submit" />
}

but generated html like this

<form action="/login" method="POST" >










<dl class=" " id="username_field">

    <dt><label for="username">username</label></dt>

    <dd>
    <input type="text" id="username" name="username" value="" />
</dd>


        <dd class="info">Required</dd>

</dl>










<dl class=" " id="password_field">

    <dt><label for="password">password</label></dt>

    <dd>
    <input type="password" id="password" name="password" />
</dd>


        <dd class="info">Required</dd>

</dl>


    <input type="submit" />

</form>

I expect at least like this

<form action="/login" method="POST" >
<dl class=" " id="username_field">
    <dt><label for="username">username</label></dt>
    <dd>
    <input type="text" id="username" name="username" value="" />
</dd>
        <dd class="info">Required</dd>
</dl>
<dl class=" " id="password_field">
    <dt><label for="password">password</label></dt>
    <dd>
    <input type="password" id="password" name="password" />
</dd>
        <dd class="info">Required</dd>
</dl>
    <input type="submit" />
</form>

It should be good if support some config to the template output, such as cleanup empty line. In general, it shouldn't generate empty line for the @import or other statements which just for logic purpose. Or add some symbol to indicate that a line is just a logic statement, should not generate empty line.

mkurz · 2018-02-20T21:08:51Z

I opened a pull request which addresses some of the newline complains. See #169

mkurz · 2018-02-22T08:27:45Z

#169 is merged, that should make the newline behaviour a bit better.
However for twirl expressions like if, for, etc. I think a solution could be to remove that lines in the rendered result if a twirl expression is the only thing in a line (besides spaces and tabs).
Just an idea, I don't have time to implement that myself anyway.

Lasering · 2019-07-16T13:39:38Z

I have the time to do a PR. I just need a little bit of guidance: where should I look at? The parser?

marcospereira · 2019-07-30T15:18:10Z

I have the time to do a PR. I just need a little bit of guidance: where should I look at? The parser?

Hey @Lasering, thanks for taking the time to work on this.

You can start here:

twirl/compiler/src/main/scala/play/twirl/compiler/TwirlCompiler.scala

Lines 177 to 207 in fe20c77

    
           def compile( 
        
               source: File, 
        
               sourceDirectory: File, 
        
               generatedDirectory: File, 
        
               formatterType: String, 
        
               additionalImports: collection.Seq[String] = Nil, 
        
               constructorAnnotations: collection.Seq[String] = Nil, 
        
               codec: Codec = TwirlIO.defaultCodec, 
        
               inclusiveDot: Boolean = false 
        
           ) = { 
        
             val resultType = formatterType + ".Appendable" 
        
             val (templateName, generatedSource) = 
        
               generatedFile(source, codec, sourceDirectory, generatedDirectory, inclusiveDot) 
        
             if (generatedSource.needRecompilation(additionalImports)) { 
        
               val generated = parseAndGenerateCode( 
        
                 templateName, 
        
                 TwirlIO.readFile(source), 
        
                 codec, 
        
                 source.getAbsolutePath, 
        
                 resultType, 
        
                 formatterType, 
        
                 additionalImports, 
        
                 constructorAnnotations, 
        
                 inclusiveDot 
        
               ) 
        
               TwirlIO.writeStringToFile(generatedSource.file, generated.toString, codec) 
        
               Some(generatedSource.file) 
        
             } else { 
        
               None 
        
             } 
        
           }

And then navigate the code from there.

Lasering · 2019-08-09T23:14:49Z

This is more for curiosity:

Why does the TwirlCompiler code uses collection.Seq everywhere and not simply Seq?

The code in generateCode appends strings one by one, without even using \n nor interpolations. For example, this:

val generated = {
      Vector.empty :+ """
package """ :+ packageName :+ """

""" :+ imports  :+ """
""" :+ (...)
}

Would be much easier to read in this form

val generated = Vector(
  s"\npackage $packageName\n\n",
  s"$imports\n",
  (...)
)

Also I cannot understand why the generated templates include the position as a comment everywhere (eg: def /*3.2*/main/*3.6*/(content: Html)). Is it to somehow help debug the templates? Would removing them break anything?

marcospereira · 2019-08-21T17:42:06Z

Why does the TwirlCompiler code uses collection.Seq everywhere and not simply Seq?

You are free to change that, but I would do that as a separated change to keep contributions on small and easier to review. :-)

Also I cannot understand why the generated templates include the position as a comment everywhere

Yeah, they are important. The positions are a "source map" that provides a way of mapping code generated by Twirl back to its original position in a .scala.html file. This means that if there is a compilation error or an exception we can point users to where it happened in html.scala.

Long story short, removing them will break things.

Lasering · 2019-08-22T13:36:24Z

Aren't the positions redundant information with https://github.com/playframework/twirl/blob/master/compiler/src/main/scala/play/twirl/compiler/TwirlCompiler.scala#L733? Especially with MATRIX and LINES?

If that information is so important shouldn't it be reified is some data structure that would be generated along side with the generated class?

mkurz mentioned this issue Feb 20, 2018

newLine at end of @this(...) and @(...) are not part of content #169

Merged

patriknw mentioned this issue Aug 24, 2018

Make generated code look nicer? akka/akka-grpc#368

Open

dwijnand added the status:backlog label Feb 14, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Consider indentation and remove newlines generated for "empty" lines #141

Consider indentation and remove newlines generated for "empty" lines #141

Lasering commented Jun 30, 2017 •

edited

Loading

marcospereira commented Jul 3, 2017

helllamer commented Jul 3, 2017

Lasering commented Jul 3, 2017

helllamer commented Jul 3, 2017 •

edited

Loading

Lasering commented Jul 3, 2017

helllamer commented Jul 3, 2017

schmitch commented Jul 3, 2017 •

edited

Loading

AlbaroPereyra commented Sep 7, 2017

JackyChan commented Sep 14, 2017

mkurz commented Feb 20, 2018

mkurz commented Feb 22, 2018

Lasering commented Jul 16, 2019 •

edited

Loading

marcospereira commented Jul 30, 2019

Lasering commented Aug 9, 2019 •

edited

Loading

marcospereira commented Aug 21, 2019

Lasering commented Aug 22, 2019

Consider indentation and remove newlines generated for "empty" lines #141

Consider indentation and remove newlines generated for "empty" lines #141

Comments

Lasering commented Jun 30, 2017 • edited Loading

marcospereira commented Jul 3, 2017

helllamer commented Jul 3, 2017

Lasering commented Jul 3, 2017

helllamer commented Jul 3, 2017 • edited Loading

Lasering commented Jul 3, 2017

helllamer commented Jul 3, 2017

schmitch commented Jul 3, 2017 • edited Loading

AlbaroPereyra commented Sep 7, 2017

JackyChan commented Sep 14, 2017

mkurz commented Feb 20, 2018

mkurz commented Feb 22, 2018

Lasering commented Jul 16, 2019 • edited Loading

marcospereira commented Jul 30, 2019

Lasering commented Aug 9, 2019 • edited Loading

marcospereira commented Aug 21, 2019

Lasering commented Aug 22, 2019

Lasering commented Jun 30, 2017 •

edited

Loading

helllamer commented Jul 3, 2017 •

edited

Loading

schmitch commented Jul 3, 2017 •

edited

Loading

Lasering commented Jul 16, 2019 •

edited

Loading

Lasering commented Aug 9, 2019 •

edited

Loading