Fix #22 (extra lines)

[AliSoftware]

Fix #22

[kylef]

Thanks for the pull request @AliSoftware, for a few reasons I don't like this implementation:

• When using a node parser which contains a block you will always have to add individual logic for trimming the CR/LF for consistent behaviour.

• This will unintentionally trim CR/LF's on a line where there is both text and a block, given the following example:

  The following people are administrators:{% for name in people %}
    - {{ name }}
   {% endfor %}

  Where this will now be rendered as:

   The following people are administrators:  - Kyle
     - Katie

  It should instead be rendered as:

   The following people are administrators:
     - Kyle
     - Katie

• This doesn't correctly handle the whitespace before the block token. Given the following example:

   <ul>
    {% for name in people %}
      <li>{{ name }}</li>
    {% endfor %}
   </ul>

  In this above example, the two spaces before the for block will be shown on the first li's line, along with the whitespace before the endfor will end up on the line of ul. Which results in the following:

   <ul>
         <li>Kyle</li>
       <li>Katie</li>
     </ul>

  Instead should be rendered as:

   <ul>
       <li>Kyle</li>
       <li>Katie</li>
   </ul>

To solve this, instead the following rule should be applied:

A block on it's own line should trim leading whitespace and trailing newlines and whitespace. A block with other text or components should not trim anything.

Example 1

1\n
    {%  foo %}\n
2\n

Would result in:

Token.Text("1\n")
Token.Block("foo")
Token.Text("2\n")

Example 2

1\n
    this is awesome:{%  foo %}\n
        - 2\n

Would result in:

Token.Text("1\n")
Token.Text("this is awesome:")
Token.Block("foo")
Token.Text("\n        - 2\n")

Note: The new line at the end of the this is awesome is retained (in the following text token).

Instead of doing this work in the individual parser level, I think this should be moved into the lexer and the lexer should ensure that any text token after a block token doesn't start with CR/LF. While currently this would be difficult to implement due how it's currently implemented. I do have some local changes I'm working on which removes the use of regex for it's own character position to be able to construct source maps to allow the errors to clearly state which part of the template caused them (#28).

Once this change is done, it would be fairly easily to remove all whitespace and trailing new lines for any blocks.

I hope to finish the source maps today and I can make an alternative PR to implement this on-top of the source-map parser inside the lexer itself.

[AliSoftware]

When using a node parser which contains a block you will always have to add individual logic for trimming the CR/LF for consistent behavior.

That was in fact kinda intended, because I think not every Node would want to trim the CRLFs. But I didn't like it either.

A block on it's own line should trim leading whitespace and trailing newlines and whitespace. A block with other text or components should not trim anything.

Totally agree, implementing this rule would be much better.

In the meantime, I'll try to rework #33 to apply guard on the code without the two commits from #32 now that we know we'll not merge them.

[kylef]

That was in fact kinda intended, because I think not every Node would want to trim the CRLFs. But I didn't like it either.

Would you see any problems with my proposed solution where it would unintentionally trim characters that some node's wouldn't like?

[AliSoftware]

Well imagine I create an LowercaseNode that is expected to be used like {% lowercase variable %}.

{% for obj in list %}
== {{ obj.name }} ==
{% lowercase obj.foo %}
{% lowercase obj.bar %}
{% endfor %}

I expect it to be rendered like this:

== Object1 ==
object1foo
object1bar
== Object2 ==
object2foo
object2bar

Isn't there a risk that with your proposed implementation, the CRLFs after every `{% lowercase x %} will be trimmed then?

[AliSoftware]

Note: agreed that lowercase stuff would better be implemented as filters once Stencil support them but in the meantime…

And a more concrete example is the use of {% now %} which is an already existing Node in Stencil and shouldn't trim CRLF imho.

Example

  Date:
    {% now yyyy-MM-dd %}
  Time:
    {% now HH:mm %}

Should generate:

  Date:
    2015-10-18
  Time:
    20:45

But your solution will generate:

  Date:
2015-10-18Time:
20:45

Probably only blocks that expect an end block would expect to trim CRLFs I think. But you can't know that during lexing, can you?

[kylef]

Sorry for not getting to this sooner. Let me have a think on this one and get back to you.

[yonaskolb]

Any updates on this? Is a valid solution stripping the whitespace and newlines only when it's a control flow block (e.g. if and for)

[kylef]

Any updates on this? Is a valid solution stripping the whitespace and newlines only when it's a control flow block (e.g. if and for)

@yonaskolb This is something I'm going to get in before Stencil 1.0.

[kylef]

Thanks for the effort here @AliSoftware. I'd like to go with the alternative design in #92 because this allows the template author to control how it is handled. @AliSoftware Do you have any major concerns with #92?

[AliSoftware]

Add said in #85 I'm in agreement with a solution (also seen in other templating languages) of allowing the user to enable lead and tail trimming per tag invocation using {%- and -%} if they want to opt in. Feels more flexible than the other solutions trying to be generic, and in phase with other templating langs 👍

[djbe]

Replaced by #287