Whitespace issues

[fw42]

We need to do something about the amount of generated whitespace in rendered Liquid templates. We have several issues and pull requests about that right now (#215, #214, #194, #171, #162) and no suitable fix.

I want to open this for discussion because it's not only annoying for third-party users but also considerably slowing down a lot of Shopify storefronts because of megabytes of whitespace resulting from big nested loops.

A few ideas have shown up:

• Add Erb like tags (this slows down parsing a lot, as can be seen in Add ERB-like trim mode for tags and outputs #214).
• Automatically collapse all white-space (this will break stuff where whitespace is intended, email templates for instance).
• Create a filter for collapsing white space which can manually be wrapped around loops (not sure if this will make things much faster, but at least the output will be smaller - on the other hand, it has to be done by hand for each template).
• Automatically collapse whitespace in loops if (and only if) the rendered output contains only whitespace (this will catch some of our worst-cases, but won't fix the problem entirely).
• ...

Comments?

@boourns @hornairs @csfrancis @dylanahsmith @camilo @arthurnn
cc @jcrawford @nickpearson @avalanche123 @ragalie

[nickpearson]

The ERB-like trims only slow down parsing if the trimming is done as part of the parsing regexes (as I originally did in #214). The newer implementation in #215 trims before parsing and is significantly faster (with almost no performance hit). Also, see my comment at the end there for a way to reduce the performance impact even further by only trimming when there is something to trim.

Unfortunately for Shopify, this means that template developers would have to update their templates to use {%-, -%}, etc., and most are probably not likely to do so. If it's always HTML, then perhaps Shopify could either (a) auto-replace {% with {%- and %} with -%} before passing the template to liquid (assuming the ERB trim mode gets merged), or (b) use an HTML compactor after the content has been passed through Liquid.

[fw42]

Maybe make your behaviour for "{%- ... -%}" the default one and introduce some way to disable it (globally for that template) when necessary (for emails, etc.)?

[fw42]

To illustrate this with an example:

This template

{% for 1 in (1..1000) %}
  {% if false %} whatever {% endif %}
{% endfor %}

generates almost 4K of whitespace, where it should (imho, at least in the majority of cases) generate exactly 0 bytes of output (and in fact does, when applying #215 and using those tags).

[fw42]

@hornairs suggested that during parsing, we look for children of code blocks ({% ... %}, like for loops) and if all of them are "code tags" too (i.e., don't generate output) and none of them are non-Liquid stuff or "output tags" ({{ ... }}), we mark them to automatically collapse all whitespace inside that block during rendering. The additional parsing overhead should be negligible I think.

This won't solve the problem entirely, but will be mostly backwards compatible and get rid of a lot of our worst cases. Optionally, we might merge #215 (or some variant of that) as an opt-in solution.

@tobi, do you have a strong opinion on this?

Other comments anyone?

[nickpearson]

Do you think it would be better to make this a global setting or to accept an options hash in Template#parse here and here? Or both, where the global setting acts as the default but can be overridden by passing :trim => false to parse?

[tobi]

I think we should do whatever PHP does.

• tobi
  CEO Shopify

On Wed, Jun 26, 2013 at 11:31 AM, Florian Weingarten <
notifications@github.com> wrote:

@hornairs https://github.com/hornairs suggested that during parsing, we
look for children of code blocks ({% ... %}, like for loops) and if all
of them are "code tags" too (i.e., don't generate output) and none of them
are non-Liquid stuff or "output tags" ({{ ... }}), we mark them to
automatically collapse all whitespace inside that block during rendering.
The additional parsing overhead should be negligible I think.

This won't solve the problem entirely, but will be mostly backwards
compatible and get rid of a lot of our worst cases. Optionally, we might
merge #215 #215 (or some
variant of that) as an opt-in solution.

@tobi https://github.com/tobi, do you have a strong opinion on this?

Other comments anyone?

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/216#issuecomment-20055754
.

[fw42]

@tobi I think switching to PHP's behaviour might break backwards compatibility for us too much. People might rely on our current behaviour (think <pre> tags and emails).

[fw42]

Two new approaches to parts of the problem: #217 and #218. The first one a bit less likely to break stuff, the second one a bit more general/aggressive. Comments welcome.

[airhorns]

PHP's behaviour is a good baseline and would go a ways to solving the problem, but not completely:

start
<?php for ($i = 1; $i <= 10; $i++) { ?>
   <?php true; ?>
   <?php true; ?>
   <?php true; ?>
<?php } ?>
end

gives

oration ~  ➜  php test.php
start
                                                            end

In liquid:

start
{% for i in (1..10) %}
  {% assign foo = i %}
{% endfor %}
end

gives

start





















end

[byroot]

Actually in this particular example PHP do not perform better than Liquid.

If you look closely at the output PHP in your example its:

"start\n                                                                                          end\n\n"

and Liquid:

"start\n\n  \n\n  \n\n  \n\n  \n\n  \n\n  \n\n  \n\n  \n\n  \n\n  \n\nend\n\n"

So 101 bytes versus 52.

But the comparison is not very fair, lets use an example more similar to Liquid one:

start
<?php for ($i = 1; $i <= 10; $i++) { ?>
   <?php true; ?>
<?php } ?>
end

You end up with:

"start\n                              end\n\n"

So 41 bytes.

But if you put a single space right after <?php true; ?> you'll get:

"start\n    \n    \n    \n    \n    \n    \n    \n    \n    \n    \nend\n\n"

Just like liquid.

The PHP behavior being "strip one line-ending directly following a php tag".
It should be trivial to implement and It will definitely save some bytes, but nothing in comparison of the indentation, and it will still break emails formatting.

You may save more by automatically converting 2/4 spaces in tab.

My 2 cents.