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

Add ERB-like trim mode for tags and outputs #215

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
@nickpearson
Contributor

nickpearson commented Jun 21, 2013

This change adds support for ERB-like trimming to Liquid. Tags and outputs are supported, though they behave differently.

TAG BEHAVIOR

For any tag starting with {%-, as with any ERB segment starting with <%-, all tabs and spaces between the beginning of the line and the beginning of the tag will be removed.

For any tag ending with -%}, as with any ERB segment ending with -%>, the newline character immediately following the tag will be removed.

For example:

<p>
  {%- if true -%}
  hi
  {%- endif -%}
</p>

becomes:

<p>
  hi
</p>

Unlike ERB, this implementation for Liquid is more forgiving in that (a) it also removes any tabs and spaces that precede the newline character, and (b) it does not require a newline to be present.

OUTPUT BEHAVIOR

Trim mode for output segments behaves a little differently.

For any output starting with {{-, all tabs and spaces between the beginning of the line and the beginning of the output and the newline preceding it will be removed. (Tabs and spaces preceding the newline will not be removed.)

For any output ending with -}}, all tabs and spaces and the newline character immediately following the output and all tabs and spaces after the newline will be removed.

For example:

<p>
  {{- 'hi' -}}
</p>

becomes:

<p>hi</p>

NOTES

In the descriptions of functionality, "newline" means \n and/or \r (specifically, /\r?\n?/). The reason both are optional is so that {%- and {{- are handled even if they are at the very beginning of the template, and -%} and -}} are handled even if they are at the very end of a template. In fact, none of the characters that are part of the trim are required, ensuring the trim-hyphen is never left in a template when it is ready to be parsed.

The beginning and ending trim modes can be used independently. For example, {%- if x %} or {{ x -}}.

The reason tabs and spaces preceding the newline in a {{- trim are not removed is so that whitespace between multiple output segments can be controlled. The reason tabs and spaces following a newline in a -}} trim are removed is so that indentation can be removed. For example, consider the following, where underscores are shown in the place of spaces:

<p>
__{{- 'abc' -}},_
__{{- 'def' -}}
</p>

becomes:

<p>abc,_def</p>

The space after the comma is kept, but the indentation spaces before 'abc' and 'def' are removed.

BENCHMARKS

Before:

Rehearsal ------------------------------------------------
parse:        12.120000   0.030000  12.150000 ( 12.153195)
parse & run:  28.540000   0.080000  28.620000 ( 28.636938)
-------------------------------------- total: 40.770000sec

                   user     system      total        real
parse:        12.290000   0.020000  12.310000 ( 12.315549)
parse & run:  28.510000   0.060000  28.570000 ( 28.589187)

After:

Rehearsal ------------------------------------------------
parse:        12.330000   0.030000  12.360000 ( 12.367535)
parse & run:  29.020000   0.070000  29.090000 ( 29.104512)
-------------------------------------- total: 41.450000sec

                   user     system      total        real
parse:        12.650000   0.030000  12.680000 ( 12.698423)
parse & run:  29.010000   0.080000  29.090000 ( 29.133262)

raw TAG HANDLING

As opposed to the original implementation of this trimming behavior, trimming now happens before the Liquid template is parsed. Because of this, any {%-, -%}, {{-, or -}} sequences inside of a raw tag (between {% raw %} and {% endraw %}) will have their whitespace trimmed.

The only place I can imagine this being an issue is in a Liquid-enabled page that is explaining how to use the trimming feature of Liquid. In this case, putting a period or some other character at the beginning and/or end of the lines with these sequences would be one way of preventing the trimming. I think the benefit of this behavior vastly outweighs this.

@nickpearson

This comment has been minimized.

Show comment
Hide comment
@nickpearson

nickpearson Jun 21, 2013

Contributor

I'm not sure how you feel about conditionally processing the template, but the gsubs could be moved into an if that checks whether the template contains {%-, -%}, {{-, or -}}. If not, then the gsubs won't be called.

So, rather than what I changed here, I could leave the tokens = source.split(TemplateParser) line alone and apply trimming on the source only when there's something to trim:

module Liquid
  # ...
  class Template
    # ...
    def tokenize(source)
      # ...
      if source =~ /\{[\{\%]-|-[\%\}]\}/
        source = source.gsub(/[ \t]*\{\%-/, '{%').
                        gsub(/-\%\}[ \t]*\r?\n?/, '%}').
                        gsub(/\r?\n?[ \t]*\{\{-/, '{{').
                        gsub(/-\}\}[ \t]*\r?\n?[ \t]*/, '}}')
      end
      tokens = source.split(TemplateParser)
      # ...
    end
  end
end

Besides a slight performance boost, this has the added benefit of ensuring no regressions with existing templates, though there shouldn't be any with valid templates; in the current version of Liquid, having {{- causes blank output and having {%- causes a Liquid::SyntaxError.

(Note: I tried an isolated benchmark to see whether checking source against a regex was faster than checking for individual strings (four calls to String#index to look for {%-, -%}, {{-, or -}}), and the regex is about 40% faster.)

If you like this better, let me know and I'll update this PR.

Contributor

nickpearson commented Jun 21, 2013

I'm not sure how you feel about conditionally processing the template, but the gsubs could be moved into an if that checks whether the template contains {%-, -%}, {{-, or -}}. If not, then the gsubs won't be called.

So, rather than what I changed here, I could leave the tokens = source.split(TemplateParser) line alone and apply trimming on the source only when there's something to trim:

module Liquid
  # ...
  class Template
    # ...
    def tokenize(source)
      # ...
      if source =~ /\{[\{\%]-|-[\%\}]\}/
        source = source.gsub(/[ \t]*\{\%-/, '{%').
                        gsub(/-\%\}[ \t]*\r?\n?/, '%}').
                        gsub(/\r?\n?[ \t]*\{\{-/, '{{').
                        gsub(/-\}\}[ \t]*\r?\n?[ \t]*/, '}}')
      end
      tokens = source.split(TemplateParser)
      # ...
    end
  end
end

Besides a slight performance boost, this has the added benefit of ensuring no regressions with existing templates, though there shouldn't be any with valid templates; in the current version of Liquid, having {{- causes blank output and having {%- causes a Liquid::SyntaxError.

(Note: I tried an isolated benchmark to see whether checking source against a regex was faster than checking for individual strings (four calls to String#index to look for {%-, -%}, {{-, or -}}), and the regex is about 40% faster.)

If you like this better, let me know and I'll update this PR.

@fw42 fw42 referenced this pull request Jun 26, 2013

Closed

Whitespace issues #216

@fw42

This comment has been minimized.

Show comment
Hide comment
@fw42

fw42 Jun 26, 2013

Member

Something seems weird with the parsing here if you mix tags like {% ... -%} or {%- ... %}. What's the intention here? Trim whitespace if at least one of the tags has a minus? Only if both? Syntax error?

Member

fw42 commented Jun 26, 2013

Something seems weird with the parsing here if you mix tags like {% ... -%} or {%- ... %}. What's the intention here? Trim whitespace if at least one of the tags has a minus? Only if both? Syntax error?

@nickpearson

This comment has been minimized.

Show comment
Hide comment
@nickpearson

nickpearson Jun 26, 2013

Contributor

This implementation supports using {%- or -%} by themselves ({%- ... %} or {% ... -%}) or together ({%- ... -%}). (Same goes for {{- and -}}.) I'm guessing in most cases, either both sides would have a - or neither would, but that'd be up to the template developer.

The reason they can be used independently is to give more control over the whitespace, and the trimming regexes are much simpler (and I'm guessing faster) to do it this way. This also mirrors what's available in ERB.

{%- means: remove any whitespace between the beginning of the line and the start of this tag
-%} means: remove the newline after this tag (and any whitespace that comes before the newline)

Contributor

nickpearson commented Jun 26, 2013

This implementation supports using {%- or -%} by themselves ({%- ... %} or {% ... -%}) or together ({%- ... -%}). (Same goes for {{- and -}}.) I'm guessing in most cases, either both sides would have a - or neither would, but that'd be up to the template developer.

The reason they can be used independently is to give more control over the whitespace, and the trimming regexes are much simpler (and I'm guessing faster) to do it this way. This also mirrors what's available in ERB.

{%- means: remove any whitespace between the beginning of the line and the start of this tag
-%} means: remove the newline after this tag (and any whitespace that comes before the newline)

@fw42

This comment has been minimized.

Show comment
Hide comment
@fw42

fw42 Jun 27, 2013

Member

Okay, this looks promising. I will take a closer look at this soon!

Member

fw42 commented Jun 27, 2013

Okay, this looks promising. I will take a closer look at this soon!

@nickpearson

This comment has been minimized.

Show comment
Hide comment
@nickpearson

nickpearson Jun 27, 2013

Contributor

Since Liquid is likely used in HTML most of the time, we could make {%- ... -%} the default behavior and add a {%+ ... +%} syntax, where + means "keep the whitespace around this tag" (the opposite of -).

I'm thinking that by default, all {% ... %} would act like {%- ... -%}, and there would be a global flag that could be set to disable this new auto-trimming behavior. There would also be a Template#parse option to override the global. Here's how the syntax would work in each global mode:

Global trimming enabled (the new Liquid default):
{% ... %} trims surrounding whitespace (acts like {%- ... -%})
{%- ... -%} trims surrounding whitespace
{%+ ... +%} keeps surrounding whitespace

Global trimming disabled (the current Liquid behavior):
{% ... %} keeps surrounding whitespace (acts like {%+ ... +%})
{%- ... -%} trims surrounding whitespace
{%+ ... +%} keeps surrounding whitespace

Note that the explicit {%- ... -%} and {%+ ... +%} behave the same regardless of the trim mode. This gives template developers control regardless of their environment. (Useful for things like <pre> tags.) The global setting only affects {% ... %}.

The implementation would be very simple. It would build on this PR and would add the following:

  1. Add a global boolean flag for trim mode
  2. Add an options hash parameter to Template#parse and to Template#tokenize, and recognize trim as an option
  3. In Template#tokenize (where the whitespace trimming happens):
    1. If global flag is true and trim option isn't false, turn all {% into {%- (and all %} to -%})
    2. Regardless of the global flag and trim option, turn all {%+ into {% (and all +%} into %})
    3. Perform whitespace trimming as already implemented in this PR

If the global flag is false or the trim option is false, then all {% and %} would be left alone (and since they wouldn't be converted to {%- and -%}, their surrounding whitespace would be left intact).

The new {%+ ... +%} syntax is just a placeholder to prevent trimming. The only thing that happens here is that the + characters are removed. (The +s are just there to so we have a way of preventing {% from being turned into {%- and %} into -%}. Regardless of the global flag and the trim option, the +s get removed in #tokenize after the other conversions happen but before the actual tokenization.)

With the new global flag set to true by default, using Liquid programmatically would be exactly the same as it is today, but trimming would happen automatically. Here are the two (hypothetical) ways trimming could be disabled:

# disable trimming globally
Liquid::Template.trim_mode = false

# force-enable or force-disable trim mode (override the global setting)
Liquid::Template.parse(source, :trim => false)

Performance-wise, another benefit of this solution overall is that the whitespace trimming happens before the nodes are rendered, so there's no need to check or modify each node individually. This could be used in conjunction with #217 or #218.

Contributor

nickpearson commented Jun 27, 2013

Since Liquid is likely used in HTML most of the time, we could make {%- ... -%} the default behavior and add a {%+ ... +%} syntax, where + means "keep the whitespace around this tag" (the opposite of -).

I'm thinking that by default, all {% ... %} would act like {%- ... -%}, and there would be a global flag that could be set to disable this new auto-trimming behavior. There would also be a Template#parse option to override the global. Here's how the syntax would work in each global mode:

Global trimming enabled (the new Liquid default):
{% ... %} trims surrounding whitespace (acts like {%- ... -%})
{%- ... -%} trims surrounding whitespace
{%+ ... +%} keeps surrounding whitespace

Global trimming disabled (the current Liquid behavior):
{% ... %} keeps surrounding whitespace (acts like {%+ ... +%})
{%- ... -%} trims surrounding whitespace
{%+ ... +%} keeps surrounding whitespace

Note that the explicit {%- ... -%} and {%+ ... +%} behave the same regardless of the trim mode. This gives template developers control regardless of their environment. (Useful for things like <pre> tags.) The global setting only affects {% ... %}.

The implementation would be very simple. It would build on this PR and would add the following:

  1. Add a global boolean flag for trim mode
  2. Add an options hash parameter to Template#parse and to Template#tokenize, and recognize trim as an option
  3. In Template#tokenize (where the whitespace trimming happens):
    1. If global flag is true and trim option isn't false, turn all {% into {%- (and all %} to -%})
    2. Regardless of the global flag and trim option, turn all {%+ into {% (and all +%} into %})
    3. Perform whitespace trimming as already implemented in this PR

If the global flag is false or the trim option is false, then all {% and %} would be left alone (and since they wouldn't be converted to {%- and -%}, their surrounding whitespace would be left intact).

The new {%+ ... +%} syntax is just a placeholder to prevent trimming. The only thing that happens here is that the + characters are removed. (The +s are just there to so we have a way of preventing {% from being turned into {%- and %} into -%}. Regardless of the global flag and the trim option, the +s get removed in #tokenize after the other conversions happen but before the actual tokenization.)

With the new global flag set to true by default, using Liquid programmatically would be exactly the same as it is today, but trimming would happen automatically. Here are the two (hypothetical) ways trimming could be disabled:

# disable trimming globally
Liquid::Template.trim_mode = false

# force-enable or force-disable trim mode (override the global setting)
Liquid::Template.parse(source, :trim => false)

Performance-wise, another benefit of this solution overall is that the whitespace trimming happens before the nodes are rendered, so there's no need to check or modify each node individually. This could be used in conjunction with #217 or #218.

@fw42

This comment has been minimized.

Show comment
Hide comment
@fw42

fw42 Jun 27, 2013

Member

Hmm. I'm not sure if I like the idea of introducing even more tags (which are not relevant to Shopify, since we will stick to the current defaults to not break backwards compatibility).

Member

fw42 commented Jun 27, 2013

Hmm. I'm not sure if I like the idea of introducing even more tags (which are not relevant to Shopify, since we will stick to the current defaults to not break backwards compatibility).

@nickpearson

This comment has been minimized.

Show comment
Hide comment
@nickpearson

nickpearson Jun 27, 2013

Contributor

The only time someone would need to use the {%+ ... +%} syntax is to prevent whitespace trimming in a Liquid environment where trimming is on by default. The + syntax wouldn't even need to be documented for environments that don't use the global trim mode (like Shopify), since in these environments {%+ would behave exactly like {%.

To add support for this would be as simple as this in Template#tokenize:

def tokenize(source)
  # ...
  source = source.gsub('{%+', '{%').gsub('+%}', '%}') if source =~ /\{\%\+|\+\%\}/
Contributor

nickpearson commented Jun 27, 2013

The only time someone would need to use the {%+ ... +%} syntax is to prevent whitespace trimming in a Liquid environment where trimming is on by default. The + syntax wouldn't even need to be documented for environments that don't use the global trim mode (like Shopify), since in these environments {%+ would behave exactly like {%.

To add support for this would be as simple as this in Template#tokenize:

def tokenize(source)
  # ...
  source = source.gsub('{%+', '{%').gsub('+%}', '%}') if source =~ /\{\%\+|\+\%\}/
@fw42

This comment has been minimized.

Show comment
Hide comment
@fw42

fw42 Jun 30, 2013

Member

I like the simplicity of your implementation. @boourns, @dylanahsmith, @csfrancis what do you guys think? I want to merge this. (Not sure about making it the default behavior for non-email templates in Shopify, but I'm lending towards it.)

Member

fw42 commented Jun 30, 2013

I like the simplicity of your implementation. @boourns, @dylanahsmith, @csfrancis what do you guys think? I want to merge this. (Not sure about making it the default behavior for non-email templates in Shopify, but I'm lending towards it.)

@csfrancis

This comment has been minimized.

Show comment
Hide comment
@csfrancis

csfrancis Jul 1, 2013

Member

I also like the simplicity of it. I don't like the idea of changing the default behaviour unless we are very confident that it won't break existing templates.

Member

csfrancis commented Jul 1, 2013

I also like the simplicity of it. I don't like the idea of changing the default behaviour unless we are very confident that it won't break existing templates.

@nickpearson

This comment has been minimized.

Show comment
Hide comment
@nickpearson

nickpearson Jul 1, 2013

Contributor

I agree -- if Shopify won't be using this to fix the whitespace problem caused by for loops (and I don't think it can fix the problem, at least not on its own), then I think trimming should not happen by default.

As such, I think this PR is good to go as is, and if you're interested, I can implement the "trim everything" mode described in my comment above (but leave it off by default) in another PR.

Contributor

nickpearson commented Jul 1, 2013

I agree -- if Shopify won't be using this to fix the whitespace problem caused by for loops (and I don't think it can fix the problem, at least not on its own), then I think trimming should not happen by default.

As such, I think this PR is good to go as is, and if you're interested, I can implement the "trim everything" mode described in my comment above (but leave it off by default) in another PR.

].gsub(/^ /, '')
expected = %[
abc:bmw:def
].gsub(/^ /, '')

This comment has been minimized.

@fw42

fw42 Jul 2, 2013

Member

What's with the gsub here? Looks weird.

@fw42

fw42 Jul 2, 2013

Member

What's with the gsub here? Looks weird.

This comment has been minimized.

@nickpearson

nickpearson Jul 2, 2013

Contributor

It unindents the Liquid code. I thought this was more readable than using \n literals in the strings, and unindenting makes the Liquid code more realistic.

@nickpearson

nickpearson Jul 2, 2013

Contributor

It unindents the Liquid code. I thought this was more readable than using \n literals in the strings, and unindenting makes the Liquid code more realistic.

tokens = source.gsub(/[ \t]*\{\%-/, '{%').
gsub(/-\%\}[ \t]*\r?\n?/, '%}').
gsub(/\r?\n?[ \t]*\{\{-/, '{{').
gsub(/-\}\}[ \t]*\r?\n?[ \t]*/, '}}').

This comment has been minimized.

@fw42

fw42 Jul 2, 2013

Member

Can we put those regular expressions into a constant with a meaningful name? WHITESPACE_TRIMMING_TAGS = [ ..., ..., ... ] or something and then loop over them here.

@fw42

fw42 Jul 2, 2013

Member

Can we put those regular expressions into a constant with a meaningful name? WHITESPACE_TRIMMING_TAGS = [ ..., ..., ... ] or something and then loop over them here.

This comment has been minimized.

@nickpearson

nickpearson Jul 2, 2013

Contributor

Sure -- I'll update the PR with this change.

@nickpearson

nickpearson Jul 2, 2013

Contributor

Sure -- I'll update the PR with this change.

This comment has been minimized.

@fw42

fw42 Jul 2, 2013

Member

Cool, also, can you add the regexp check you mentioned?

@fw42

fw42 Jul 2, 2013

Member

Cool, also, can you add the regexp check you mentioned?

This comment has been minimized.

@tobi

tobi Feb 7, 2016

Member

That change is still needed

@tobi

tobi Feb 7, 2016

Member

That change is still needed

# Make sure the trim isn't too agressive
def test_no_trim_tags
assert_template_result("<p>yes</p>", "<p>{%- if test -%}yes{%- endif -%}</p>", 'test' => true)
assert_template_result("<p></p>", "<p>{%- if test -%}no{%- endif -%}</p>", 'test' => false)

This comment has been minimized.

@fw42

fw42 Jul 2, 2013

Member

You can just do {%- if true -%} here, no need to have a variable.

@fw42

fw42 Jul 2, 2013

Member

You can just do {%- if true -%} here, no need to have a variable.

<p>
{%- if test %}
no
{%- endif %}

This comment has been minimized.

@fw42

fw42 Jul 2, 2013

Member

Can we have a test which checks that pre- and post-trimming can be combined? Like {%- if foo %} ... {% endif -%}?

@fw42

fw42 Jul 2, 2013

Member

Can we have a test which checks that pre- and post-trimming can be combined? Like {%- if foo %} ... {% endif -%}?

</p>
</div>", "
<div>
<p>

This comment has been minimized.

@fw42

fw42 Jul 2, 2013

Member

Can we have a test, similar to this one, which checks that, if the "<p>" would have trailing whitespace, like "<p> ", it would not be removed? Or maybe it should? I'm not sure :-)

@fw42

fw42 Jul 2, 2013

Member

Can we have a test, similar to this one, which checks that, if the "<p>" would have trailing whitespace, like "<p> ", it would not be removed? Or maybe it should? I'm not sure :-)

{{- name -}}
</p>
</div>",
'name' => 'John')

This comment has been minimized.

@fw42

fw42 Jul 2, 2013

Member

You can do {{- 'John' -}}, same for test above, etc.

@fw42

fw42 Jul 2, 2013

Member

You can do {{- 'John' -}}, same for test above, etc.

@ghost ghost assigned fw42 Jul 2, 2013

@burke

This comment has been minimized.

Show comment
Hide comment
@burke

burke Jul 2, 2013

Member

I like the implementation of this PR, but I'm very hesitant to add new features to liquid. We've tried hard to resist the temptation to add a bunch of new features over the years. Keeping the language small makes it easier to use and learn. Additionally, this being effectively a performance feature, and opt-in at that, it won't be widely-used.

However, the extra whitespace is definitely an issue that should be addressed.

IMO, anyone relying on superfluous whitespace generation is doing it wrong, so I'm not especially concerned about changing the default behaviour, especially because we can't rely on most liquid users to be aware of the performance-optimization features available to them (on our platform, at least!).

@fw42, if we hypothetically reject the concern around changing the default behaviour, what does your open PR not do that this does?

Member

burke commented Jul 2, 2013

I like the implementation of this PR, but I'm very hesitant to add new features to liquid. We've tried hard to resist the temptation to add a bunch of new features over the years. Keeping the language small makes it easier to use and learn. Additionally, this being effectively a performance feature, and opt-in at that, it won't be widely-used.

However, the extra whitespace is definitely an issue that should be addressed.

IMO, anyone relying on superfluous whitespace generation is doing it wrong, so I'm not especially concerned about changing the default behaviour, especially because we can't rely on most liquid users to be aware of the performance-optimization features available to them (on our platform, at least!).

@fw42, if we hypothetically reject the concern around changing the default behaviour, what does your open PR not do that this does?

@fw42

This comment has been minimized.

Show comment
Hide comment
@fw42

fw42 Jul 2, 2013

Member

@burke: My PR (#218) strips whitespace inside of Block tags if none (!) of the Tags inside that block rendered any non-whitespace output. So something like

{% for i in (1..100) %}
  {% comment %} foo {% endcomment %}
{% endfor %}

will render to an empty string.

If you have something like

{% for i in (1..100) %}
  {% if false %} foo {% endif %}
  bla
{% endfor %}

the whitespace before the opening if tag will still be rendered (since not all of the for block's output is blank). If using the {%- tag from this PR, it wouldn't.

Member

fw42 commented Jul 2, 2013

@burke: My PR (#218) strips whitespace inside of Block tags if none (!) of the Tags inside that block rendered any non-whitespace output. So something like

{% for i in (1..100) %}
  {% comment %} foo {% endcomment %}
{% endfor %}

will render to an empty string.

If you have something like

{% for i in (1..100) %}
  {% if false %} foo {% endif %}
  bla
{% endfor %}

the whitespace before the opening if tag will still be rendered (since not all of the for block's output is blank). If using the {%- tag from this PR, it wouldn't.

@fw42

This comment has been minimized.

Show comment
Hide comment
@fw42

fw42 Jul 2, 2013

Member

Also, we can't make this the global default behaviour because it will break text/plain formatting (emails). Could enable it for HTML though and just have people deal with it (if it breaks your template, it probably sucked in the first place).

Member

fw42 commented Jul 2, 2013

Also, we can't make this the global default behaviour because it will break text/plain formatting (emails). Could enable it for HTML though and just have people deal with it (if it breaks your template, it probably sucked in the first place).

@fw42

This comment has been minimized.

Show comment
Hide comment
@fw42

fw42 Jul 3, 2013

Member

As I said, I like this solution, but we talked this over with a few people, including @tobi, and decided that for now, we are not going to introduce new tags but look for other solutions to our Shopify-specific problems instead and try to keep Liquid core as small and simple as possible. Our need for this feature is not strong enough. We can't make this the default behavior at Shopify (because it might break existing templates) and introducing an opt-in solution wont solve our problems.

Since your implementation is basically a pre-processing step, it should be pretty easy to do outside of Liquid if needed.

We really appreciate all the effort you have put into this and I am sorry that it took us so long to decide on whether to merge this or not.

Member

fw42 commented Jul 3, 2013

As I said, I like this solution, but we talked this over with a few people, including @tobi, and decided that for now, we are not going to introduce new tags but look for other solutions to our Shopify-specific problems instead and try to keep Liquid core as small and simple as possible. Our need for this feature is not strong enough. We can't make this the default behavior at Shopify (because it might break existing templates) and introducing an opt-in solution wont solve our problems.

Since your implementation is basically a pre-processing step, it should be pretty easy to do outside of Liquid if needed.

We really appreciate all the effort you have put into this and I am sorry that it took us so long to decide on whether to merge this or not.

@erwanlr

This comment has been minimized.

Show comment
Hide comment
@erwanlr

erwanlr Jul 31, 2015

Thanks for this PR @nickpearson, really needed those white spaces to be removed as I output the rendered in a terminal.

Although the original code does not work if moved in the Tokenizer class (new class created after this PR it seems), it does the job when placed in Liquid::Template#tokenize (like originally).

As far as I hate monkey patching, that's the only solution so far :s

erwanlr commented Jul 31, 2015

Thanks for this PR @nickpearson, really needed those white spaces to be removed as I output the rendered in a terminal.

Although the original code does not work if moved in the Tokenizer class (new class created after this PR it seems), it does the job when placed in Liquid::Template#tokenize (like originally).

As far as I hate monkey patching, that's the only solution so far :s

@tobi

This comment has been minimized.

Show comment
Hide comment
@tobi

tobi Feb 7, 2016

Member

I'm now in favor of bringing this into liquid. My question is about the implementation. This simple change is taking liquid parsing from a single pass split up to 5 total passes over the source with a lot of template substitution and garbage creation.

I feel this needs to be implemented as a feature of the tag parsing system with perhaps one quick pass over the token stream before outputting. Any CS students following who are looking for a challenge?

Member

tobi commented Feb 7, 2016

I'm now in favor of bringing this into liquid. My question is about the implementation. This simple change is taking liquid parsing from a single pass split up to 5 total passes over the source with a lot of template substitution and garbage creation.

I feel this needs to be implemented as a feature of the tag parsing system with perhaps one quick pass over the token stream before outputting. Any CS students following who are looking for a challenge?

@pushrax

This comment has been minimized.

Show comment
Hide comment
@pushrax

pushrax Feb 7, 2016

Member

We could port the Liquid-C tokenizer to Ruby and unify their algorithm.

Member

pushrax commented Feb 7, 2016

We could port the Liquid-C tokenizer to Ruby and unify their algorithm.

@nickpearson

This comment has been minimized.

Show comment
Hide comment
@nickpearson

nickpearson Feb 8, 2016

Contributor

I haven't checked how much this helps with garbage creation, but the four regexes can be combined into one, causing only one pass to be necessary. (I don't object that this could and perhaps should be done in the tag parsing system; this is just for discussion.) Example:

html = "<p>\n  {%- if true -%}\n    {{- x -}}\n  {%- endif -%}\n</p>"
#=> <p>
#     {%- if true -%}
#       {{- x -}}
#     {%- endif -%}
#   </p>"
output = html.gsub(/
  [ \t]*(?<m>\{\%)-              # handle {%-
  |
  -(?<m>\%\})[ \t]*\r?\n?        # handle -%}
  |
  \r?\n?[ \t]*(?<m>\{\{)-        # handle {{-
  |
  -(?<m>\}\})[ \t]*\r?\n?[ \t]*  # handle -}}
/x) { |str|
  Regexp.last_match[:m] # will be "{%", "%}", "{{", or "}}"
}
puts output
#=> <p>
#   {% if true %}{{ x }}{% endif %}</p>

Again, no benchmarking done yet; just showing a single-pass regex solution for discussion.

Also, when I wrote this 2.5 years ago, { and } weren't allowed in Liquid strings (e.g., {{ 'this {would not} work' }}). If memory serves, these characters are allowed now, and since the regex implementation in this comment and in this PR happens before Liquid parses the template, any {%-/-%}/{{-/-}} appearing in a Liquid string would be affected even though it shouldn't be. (You could argue that these sequences will likely never appear, but you could argue more prudently that that doesn't matter, and that a template language should handle whatever input you give it in a predictable way.)

Contributor

nickpearson commented Feb 8, 2016

I haven't checked how much this helps with garbage creation, but the four regexes can be combined into one, causing only one pass to be necessary. (I don't object that this could and perhaps should be done in the tag parsing system; this is just for discussion.) Example:

html = "<p>\n  {%- if true -%}\n    {{- x -}}\n  {%- endif -%}\n</p>"
#=> <p>
#     {%- if true -%}
#       {{- x -}}
#     {%- endif -%}
#   </p>"
output = html.gsub(/
  [ \t]*(?<m>\{\%)-              # handle {%-
  |
  -(?<m>\%\})[ \t]*\r?\n?        # handle -%}
  |
  \r?\n?[ \t]*(?<m>\{\{)-        # handle {{-
  |
  -(?<m>\}\})[ \t]*\r?\n?[ \t]*  # handle -}}
/x) { |str|
  Regexp.last_match[:m] # will be "{%", "%}", "{{", or "}}"
}
puts output
#=> <p>
#   {% if true %}{{ x }}{% endif %}</p>

Again, no benchmarking done yet; just showing a single-pass regex solution for discussion.

Also, when I wrote this 2.5 years ago, { and } weren't allowed in Liquid strings (e.g., {{ 'this {would not} work' }}). If memory serves, these characters are allowed now, and since the regex implementation in this comment and in this PR happens before Liquid parses the template, any {%-/-%}/{{-/-}} appearing in a Liquid string would be affected even though it shouldn't be. (You could argue that these sequences will likely never appear, but you could argue more prudently that that doesn't matter, and that a template language should handle whatever input you give it in a predictable way.)

@j0rd

This comment has been minimized.

Show comment
Hide comment
@j0rd

j0rd Mar 31, 2016

Can we get this pushed to Shopify.

One of my largest gripe with liquid, is this whitespace issue.

For instance, I'm using this massive snippet in my store to transform image urls to not use shopify's CDN

{% capture IMGIX %}
  {% comment %}
    <!--
    Convert a Shopify CDN path into an imgix path, with parameters.

    * Check out imgix's Shopify integration guide to learn more: https://docs.imgix.com/guides/shopify-integration
    * Refer to the imgix documentation to learn about all the available parameters: https://docs.imgix.com/apis/url
    -->
  {% endcomment %}
  {% if settings.enableImgix %}
    {% for i in (1..1) %}
      {% comment %}
        <!-- Check to make sure the src exists, and that imgix url theme settings is not blank. If blank, stop! -->
      {% endcomment %}
      {% unless src or settings.imgixUrl != blank %}
        {{ src }}
        {% break %}
      {% endunless %}

      {% comment %}
        <!-- Check to make sure the src has the Shopify CDN url in it. If it doesn't this does not need to run any further -->
      {% endcomment %}
      {% assign cdnUrl = settings.imgixShopifyUrl | strip %}
      {% unless src contains cdnUrl %}
        {{ src }}
        {% break %}
      {% endunless %}

      {% assign imgixUrl = settings.imgixUrl | strip %}

      {% comment %}
        <!-- Create a list of all the imgix filters we want to use -->
      {% endcomment %}
      {% assign filters = 'bri,con,cs,exp,gam,high,hue,invert,sat,shad,sharp,usm,usmrad,vib,auto,bg,blend,bm,ba,balph,bp,bw,bh,bf,bc,bs,bx,by,border,pad,faceindex,facepad,faces,ch,chromasub,colorquant,dl,dpi,fm,lossless,q,mask,nr,nrs,colors,palette,prefix,dpr,flip,or,rot,crop,fit,h,rect,w,blur,htn,mono,px,sepia,txt,txtalign,txtclip,txtclr,txtfit,txtfont,txtline,txtlineclr,txtpad,txtshad,txtsize,txtwidth,trim,trimcolor,trimmd,trimsd,trimtol,mark,markalign,markalpha,markbase,markfit,markh,markpad,markscale,markw,markx,marky' | split:',' %}

      {% comment %}
        <!-- Build up the list of filters to add to the url -->
      {% endcomment %}
      {% assign imgWithQuerystring = "?" %}

      {% if src contains '?' %}
        {% assign imgWithQuerystring = '&' %}
      {% endif %}

      {% for _filter in filters %}
        {% if [_filter] %}
          {% assign imgWithQuerystring = imgWithQuerystring | append:_filter | append:'=' | append:[_filter] | append:'&' %}
        {% endif %}
      {% endfor %}

      {% assign newSrc = src | strip | replace:cdnUrl,imgixUrl | append:imgWithQuerystring %}
    {% endfor %}

    {{ newSrc | default:src }}
  {% else %}
    {{ src }}
  {% endif %}
{% endcapture %}{{ IMGIX | strip | replace:'  ' | strip_newlines }}

Then using my image tags like this:
{% assign img = collection.image.src | collection_img_url: 'master' %}{% include 'imgix' src:img auto:'format' %}{{ IMGIX | img_tag: collection.title }}

Which gives me image tags like this

<img src="

























    https://foobar.imgix.net/s/files/1/0433/9741/collections/womentee.jpg?v=1448080259&auto=format&

" alt="Womens T-Shirts" />

I've struggled with this a lot developing with liquid on shopify for a couple years now, and I usually roll up all my code into a single line to avoid the silly whitespace issues, but makes my code hard to read.

Any idea when/if this will ever get rolled out?

j0rd commented Mar 31, 2016

Can we get this pushed to Shopify.

One of my largest gripe with liquid, is this whitespace issue.

For instance, I'm using this massive snippet in my store to transform image urls to not use shopify's CDN

{% capture IMGIX %}
  {% comment %}
    <!--
    Convert a Shopify CDN path into an imgix path, with parameters.

    * Check out imgix's Shopify integration guide to learn more: https://docs.imgix.com/guides/shopify-integration
    * Refer to the imgix documentation to learn about all the available parameters: https://docs.imgix.com/apis/url
    -->
  {% endcomment %}
  {% if settings.enableImgix %}
    {% for i in (1..1) %}
      {% comment %}
        <!-- Check to make sure the src exists, and that imgix url theme settings is not blank. If blank, stop! -->
      {% endcomment %}
      {% unless src or settings.imgixUrl != blank %}
        {{ src }}
        {% break %}
      {% endunless %}

      {% comment %}
        <!-- Check to make sure the src has the Shopify CDN url in it. If it doesn't this does not need to run any further -->
      {% endcomment %}
      {% assign cdnUrl = settings.imgixShopifyUrl | strip %}
      {% unless src contains cdnUrl %}
        {{ src }}
        {% break %}
      {% endunless %}

      {% assign imgixUrl = settings.imgixUrl | strip %}

      {% comment %}
        <!-- Create a list of all the imgix filters we want to use -->
      {% endcomment %}
      {% assign filters = 'bri,con,cs,exp,gam,high,hue,invert,sat,shad,sharp,usm,usmrad,vib,auto,bg,blend,bm,ba,balph,bp,bw,bh,bf,bc,bs,bx,by,border,pad,faceindex,facepad,faces,ch,chromasub,colorquant,dl,dpi,fm,lossless,q,mask,nr,nrs,colors,palette,prefix,dpr,flip,or,rot,crop,fit,h,rect,w,blur,htn,mono,px,sepia,txt,txtalign,txtclip,txtclr,txtfit,txtfont,txtline,txtlineclr,txtpad,txtshad,txtsize,txtwidth,trim,trimcolor,trimmd,trimsd,trimtol,mark,markalign,markalpha,markbase,markfit,markh,markpad,markscale,markw,markx,marky' | split:',' %}

      {% comment %}
        <!-- Build up the list of filters to add to the url -->
      {% endcomment %}
      {% assign imgWithQuerystring = "?" %}

      {% if src contains '?' %}
        {% assign imgWithQuerystring = '&' %}
      {% endif %}

      {% for _filter in filters %}
        {% if [_filter] %}
          {% assign imgWithQuerystring = imgWithQuerystring | append:_filter | append:'=' | append:[_filter] | append:'&' %}
        {% endif %}
      {% endfor %}

      {% assign newSrc = src | strip | replace:cdnUrl,imgixUrl | append:imgWithQuerystring %}
    {% endfor %}

    {{ newSrc | default:src }}
  {% else %}
    {{ src }}
  {% endif %}
{% endcapture %}{{ IMGIX | strip | replace:'  ' | strip_newlines }}

Then using my image tags like this:
{% assign img = collection.image.src | collection_img_url: 'master' %}{% include 'imgix' src:img auto:'format' %}{{ IMGIX | img_tag: collection.title }}

Which gives me image tags like this

<img src="

























    https://foobar.imgix.net/s/files/1/0433/9741/collections/womentee.jpg?v=1448080259&auto=format&

" alt="Womens T-Shirts" />

I've struggled with this a lot developing with liquid on shopify for a couple years now, and I usually roll up all my code into a single line to avoid the silly whitespace issues, but makes my code hard to read.

Any idea when/if this will ever get rolled out?

@tobi

This comment has been minimized.

Show comment
Hide comment
@tobi

tobi Mar 31, 2016

Member

Why on earth would you do that?
On Thu, Mar 31, 2016 at 8:14 AM Jordan L notifications@github.com wrote:

Can we get this pushed to Shopify.

One of my largest griped with liquid, is this whitespace issue.

For instance, I'm using this massive snippet in my store to transform
image urls to not use shopify's CDN

{% capture IMGIX %}
{% comment %}
<!--
Convert a Shopify CDN path into an imgix path, with parameters.

* Check out imgix's Shopify integration guide to learn more: https://docs.imgix.com/guides/shopify-integration
* Refer to the imgix documentation to learn about all the available parameters: https://docs.imgix.com/apis/url
-->

{% endcomment %}
{% if settings.enableImgix %}
{% for i in (1..1) %}
{% comment %}

{% endcomment %}
{% unless src or settings.imgixUrl != blank %}
{{ src }}
{% break %}
{% endunless %}

  {% comment %}
    <!-- Check to make sure the src has the Shopify CDN url in it. If it doesn't this does not need to run any further -->
  {% endcomment %}
  {% assign cdnUrl = settings.imgixShopifyUrl | strip %}
  {% unless src contains cdnUrl %}
    {{ src }}
    {% break %}
  {% endunless %}

  {% assign imgixUrl = settings.imgixUrl | strip %}

  {% comment %}
    <!-- Create a list of all the imgix filters we want to use -->
  {% endcomment %}
  {% assign filters = 'bri,con,cs,exp,gam,high,hue,invert,sat,shad,sharp,usm,usmrad,vib,auto,bg,blend,bm,ba,balph,bp,bw,bh,bf,bc,bs,bx,by,border,pad,faceindex,facepad,faces,ch,chromasub,colorquant,dl,dpi,fm,lossless,q,mask,nr,nrs,colors,palette,prefix,dpr,flip,or,rot,crop,fit,h,rect,w,blur,htn,mono,px,sepia,txt,txtalign,txtclip,txtclr,txtfit,txtfont,txtline,txtlineclr,txtpad,txtshad,txtsize,txtwidth,trim,trimcolor,trimmd,trimsd,trimtol,mark,markalign,markalpha,markbase,markfit,markh,markpad,markscale,markw,markx,marky' | split:',' %}

  {% comment %}
    <!-- Build up the list of filters to add to the url -->
  {% endcomment %}
  {% assign imgWithQuerystring = "?" %}

  {% if src contains '?' %}
    {% assign imgWithQuerystring = '&' %}
  {% endif %}

  {% for _filter in filters %}
    {% if [_filter] %}
      {% assign imgWithQuerystring = imgWithQuerystring | append:_filter | append:'=' | append:[_filter] | append:'&' %}
    {% endif %}
  {% endfor %}

  {% assign newSrc = src | strip | replace:cdnUrl,imgixUrl | append:imgWithQuerystring %}
{% endfor %}

{{ newSrc | default:src }}

{% else %}
{{ src }}
{% endif %}
{% endcapture %}{{ IMGIX | strip | replace:' ' | strip_newlines }}

Then using my image tags like this:
{% assign img = collection.image.src | collection_img_url: 'master' %}{%
include 'imgix' src:img auto:'format' %}{{ IMGIX | img_tag:
collection.title }}

Which gives me image tags like this

<img src="

https://foobar.imgix.net/s/files/1/0433/9741/collections/womentee.jpg?v=1448080259&auto=format&

" alt="Womens T-Shirts" />

I've struggled with this a lot developing with liquid on shopify for a
couple years now, and I usually roll up all my code into a single line to
avoid the silly whitespace issues, but makes my code hard to read.

Any idea when/if this will ever get rolled out?


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#215 (comment)

  • tobi
    CEO Shopify
Member

tobi commented Mar 31, 2016

Why on earth would you do that?
On Thu, Mar 31, 2016 at 8:14 AM Jordan L notifications@github.com wrote:

Can we get this pushed to Shopify.

One of my largest griped with liquid, is this whitespace issue.

For instance, I'm using this massive snippet in my store to transform
image urls to not use shopify's CDN

{% capture IMGIX %}
{% comment %}
<!--
Convert a Shopify CDN path into an imgix path, with parameters.

* Check out imgix's Shopify integration guide to learn more: https://docs.imgix.com/guides/shopify-integration
* Refer to the imgix documentation to learn about all the available parameters: https://docs.imgix.com/apis/url
-->

{% endcomment %}
{% if settings.enableImgix %}
{% for i in (1..1) %}
{% comment %}

{% endcomment %}
{% unless src or settings.imgixUrl != blank %}
{{ src }}
{% break %}
{% endunless %}

  {% comment %}
    <!-- Check to make sure the src has the Shopify CDN url in it. If it doesn't this does not need to run any further -->
  {% endcomment %}
  {% assign cdnUrl = settings.imgixShopifyUrl | strip %}
  {% unless src contains cdnUrl %}
    {{ src }}
    {% break %}
  {% endunless %}

  {% assign imgixUrl = settings.imgixUrl | strip %}

  {% comment %}
    <!-- Create a list of all the imgix filters we want to use -->
  {% endcomment %}
  {% assign filters = 'bri,con,cs,exp,gam,high,hue,invert,sat,shad,sharp,usm,usmrad,vib,auto,bg,blend,bm,ba,balph,bp,bw,bh,bf,bc,bs,bx,by,border,pad,faceindex,facepad,faces,ch,chromasub,colorquant,dl,dpi,fm,lossless,q,mask,nr,nrs,colors,palette,prefix,dpr,flip,or,rot,crop,fit,h,rect,w,blur,htn,mono,px,sepia,txt,txtalign,txtclip,txtclr,txtfit,txtfont,txtline,txtlineclr,txtpad,txtshad,txtsize,txtwidth,trim,trimcolor,trimmd,trimsd,trimtol,mark,markalign,markalpha,markbase,markfit,markh,markpad,markscale,markw,markx,marky' | split:',' %}

  {% comment %}
    <!-- Build up the list of filters to add to the url -->
  {% endcomment %}
  {% assign imgWithQuerystring = "?" %}

  {% if src contains '?' %}
    {% assign imgWithQuerystring = '&' %}
  {% endif %}

  {% for _filter in filters %}
    {% if [_filter] %}
      {% assign imgWithQuerystring = imgWithQuerystring | append:_filter | append:'=' | append:[_filter] | append:'&' %}
    {% endif %}
  {% endfor %}

  {% assign newSrc = src | strip | replace:cdnUrl,imgixUrl | append:imgWithQuerystring %}
{% endfor %}

{{ newSrc | default:src }}

{% else %}
{{ src }}
{% endif %}
{% endcapture %}{{ IMGIX | strip | replace:' ' | strip_newlines }}

Then using my image tags like this:
{% assign img = collection.image.src | collection_img_url: 'master' %}{%
include 'imgix' src:img auto:'format' %}{{ IMGIX | img_tag:
collection.title }}

Which gives me image tags like this

<img src="

https://foobar.imgix.net/s/files/1/0433/9741/collections/womentee.jpg?v=1448080259&auto=format&

" alt="Womens T-Shirts" />

I've struggled with this a lot developing with liquid on shopify for a
couple years now, and I usually roll up all my code into a single line to
avoid the silly whitespace issues, but makes my code hard to read.

Any idea when/if this will ever get rolled out?


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#215 (comment)

  • tobi
    CEO Shopify
@j0rd

This comment has been minimized.

Show comment
Hide comment
@j0rd

j0rd Mar 31, 2016

Because it increases my revenue. The shopify CDN has a lot of issues (which wouldn't take long to fix)

Read here:
https://medium.com/@j0rd/an-open-letter-to-shopify-a-feature-worth-supporting-7f7334345188

j0rd commented Mar 31, 2016

Because it increases my revenue. The shopify CDN has a lot of issues (which wouldn't take long to fix)

Read here:
https://medium.com/@j0rd/an-open-letter-to-shopify-a-feature-worth-supporting-7f7334345188

@fw42

This comment has been minimized.

Show comment
Hide comment
@fw42

fw42 Mar 31, 2016

Member

Thanks for sharing that blog post

Member

fw42 commented Mar 31, 2016

Thanks for sharing that blog post

@j0rd

This comment has been minimized.

Show comment
Hide comment
@j0rd

j0rd Mar 31, 2016

@fw42 no problem. Being trying to get this fixed at Shopify for two years. So would be nice if you guys actually seriously look into this. It effects all of your stores. And would seriously help everyone's bottom line. I'll give you a mailbox, you can send cheques to once you fix this and every stores conversion rate go up.

With that said, aside from the cdn issue at Shopify, the liquid whitespace issue is also a problem. I do rather complex things in liquid all the time, and the more logic that is required to work around the limitations, the more white-space I get.

The {%- -%} fix would work just fine for me I think.

j0rd commented Mar 31, 2016

@fw42 no problem. Being trying to get this fixed at Shopify for two years. So would be nice if you guys actually seriously look into this. It effects all of your stores. And would seriously help everyone's bottom line. I'll give you a mailbox, you can send cheques to once you fix this and every stores conversion rate go up.

With that said, aside from the cdn issue at Shopify, the liquid whitespace issue is also a problem. I do rather complex things in liquid all the time, and the more logic that is required to work around the limitations, the more white-space I get.

The {%- -%} fix would work just fine for me I think.

@fw42

This comment has been minimized.

Show comment
Hide comment
@fw42

fw42 Mar 31, 2016

Member

@j0rd: We will definitely take some time to at least verify your claims. From a quick look, it seems like the 90% JPEG thing is true and it might not be too hard to fix. Will keep you updated. Thanks again for bringing it up!

Member

fw42 commented Mar 31, 2016

@j0rd: We will definitely take some time to at least verify your claims. From a quick look, it seems like the 90% JPEG thing is true and it might not be too hard to fix. Will keep you updated. Thanks again for bringing it up!

@j0rd

This comment has been minimized.

Show comment
Hide comment
@j0rd

j0rd Mar 31, 2016

@fw42 main thing which should happen, is if user uploads an optimized image, and if shopify CDN will make the image larger, than use the default settings which are embeded in the image.

I've made scripts to download all my product images, optimize them the way I want, then shopify mangles all my hard work.

j0rd commented Mar 31, 2016

@fw42 main thing which should happen, is if user uploads an optimized image, and if shopify CDN will make the image larger, than use the default settings which are embeded in the image.

I've made scripts to download all my product images, optimize them the way I want, then shopify mangles all my hard work.

@evulse

This comment has been minimized.

Show comment
Hide comment
@evulse

evulse Apr 13, 2016

Contributor

@nickpearson Results for the combined version are slower, I was trying a similar solution when I noticed your comment. Both had the same result.

Rehearsal ------------------------------------------------
parse:         4.320000   0.030000   4.350000 (  4.378114)
parse & run:   9.440000   0.100000   9.540000 (  9.593777)
-------------------------------------- total: 13.890000sec

                   user     system      total        real
parse:         4.460000   0.050000   4.510000 (  4.527056)
parse & run:   9.380000   0.080000   9.460000 (  9.492994)

Compared to the original 4 regex solution

Rehearsal ------------------------------------------------
parse:         4.140000   0.030000   4.170000 (  4.178772)
parse & run:   9.190000   0.050000   9.240000 (  9.270956)
-------------------------------------- total: 13.410000sec

                   user     system      total        real
parse:         4.290000   0.040000   4.330000 (  4.359304)
parse & run:   9.260000   0.070000   9.330000 (  9.385387)
Contributor

evulse commented Apr 13, 2016

@nickpearson Results for the combined version are slower, I was trying a similar solution when I noticed your comment. Both had the same result.

Rehearsal ------------------------------------------------
parse:         4.320000   0.030000   4.350000 (  4.378114)
parse & run:   9.440000   0.100000   9.540000 (  9.593777)
-------------------------------------- total: 13.890000sec

                   user     system      total        real
parse:         4.460000   0.050000   4.510000 (  4.527056)
parse & run:   9.380000   0.080000   9.460000 (  9.492994)

Compared to the original 4 regex solution

Rehearsal ------------------------------------------------
parse:         4.140000   0.030000   4.170000 (  4.178772)
parse & run:   9.190000   0.050000   9.240000 (  9.270956)
-------------------------------------- total: 13.410000sec

                   user     system      total        real
parse:         4.290000   0.040000   4.330000 (  4.359304)
parse & run:   9.260000   0.070000   9.330000 (  9.385387)
@erlgry

This comment has been minimized.

Show comment
Hide comment
@erlgry

erlgry May 1, 2016

@fw42 @j0rd is there an existing issue for the image optimization issue? I have recently been working with Shopify and needless to say completely baffled with the image sizes issue. I am glad to have come across this issue (due to the interest in the whitespace issue). I have noticed exactly what you wrote on the blog and have even chatted with the support about it, only to be told that the issue may be with the theme. After investigating I have concluded, the issue is not with the theme and instead is with Shopify's handling of images. I would like to add my STRONG voice to fixing the issue of image de-optimization. If there is an existing issue please let me know. Also if there is work around, short of moving to another CDN, I would certainly appreciate it.

erlgry commented May 1, 2016

@fw42 @j0rd is there an existing issue for the image optimization issue? I have recently been working with Shopify and needless to say completely baffled with the image sizes issue. I am glad to have come across this issue (due to the interest in the whitespace issue). I have noticed exactly what you wrote on the blog and have even chatted with the support about it, only to be told that the issue may be with the theme. After investigating I have concluded, the issue is not with the theme and instead is with Shopify's handling of images. I would like to add my STRONG voice to fixing the issue of image de-optimization. If there is an existing issue please let me know. Also if there is work around, short of moving to another CDN, I would certainly appreciate it.

@erlgry

This comment has been minimized.

Show comment
Hide comment
@erlgry

erlgry May 1, 2016

Also this PR is closed. Is the proposed solution for whitespace issue no longer being pursued?

erlgry commented May 1, 2016

Also this PR is closed. Is the proposed solution for whitespace issue no longer being pursued?

@fw42

This comment has been minimized.

Show comment
Hide comment
@fw42

fw42 May 2, 2016

Member

@erlgry: We have someone looking into it. @Thibaut might be able to give you more details.

Member

fw42 commented May 2, 2016

@erlgry: We have someone looking into it. @Thibaut might be able to give you more details.

@erlgry

This comment has been minimized.

Show comment
Hide comment
@erlgry

erlgry May 2, 2016

@fw42 Thanks. Overall have been quite happy with Shopify but the issues of image de-optimization has completely stumped me. I am very very interested in more details/insights/workarounds this as well as the whitespace issue.

erlgry commented May 2, 2016

@fw42 Thanks. Overall have been quite happy with Shopify but the issues of image de-optimization has completely stumped me. I am very very interested in more details/insights/workarounds this as well as the whitespace issue.

@Thibaut

This comment has been minimized.

Show comment
Hide comment
@Thibaut

Thibaut May 2, 2016

Member

My team is looking into improving our image optimization pipeline. Expect improvements later this month (reducing JPEG quality to 75%). The issue of image de-optimization is difficult to fix but also on our radar.

Member

Thibaut commented May 2, 2016

My team is looking into improving our image optimization pipeline. Expect improvements later this month (reducing JPEG quality to 75%). The issue of image de-optimization is difficult to fix but also on our radar.

@erlgry

This comment has been minimized.

Show comment
Hide comment
@erlgry

erlgry May 2, 2016

@Thibaut Thanks. That's great news. Look forward to the improvements. What's the best way to know that improvements have been implemented so I can try those out? Is there a Shopify mailing list/blog etc. that I should keep an eye on? Sorry if this is a newb question.

erlgry commented May 2, 2016

@Thibaut Thanks. That's great news. Look forward to the improvements. What's the best way to know that improvements have been implemented so I can try those out? Is there a Shopify mailing list/blog etc. that I should keep an eye on? Sorry if this is a newb question.

@j0rd

This comment has been minimized.

Show comment
Hide comment
@j0rd

j0rd May 3, 2016

@Thibaut don't forget to make the images progressive please. Makes a large difference to "perceived speed" to end users.

PS. Since making this change on a second site of mine and letting is run for 30+ days.

Facebook Ads: 46% conversion improvement
Organic Search: 10.5% conversion improvement
Direct: 30.5% conversion improvement
Social: 19% conversion improvement

j0rd commented May 3, 2016

@Thibaut don't forget to make the images progressive please. Makes a large difference to "perceived speed" to end users.

PS. Since making this change on a second site of mine and letting is run for 30+ days.

Facebook Ads: 46% conversion improvement
Organic Search: 10.5% conversion improvement
Direct: 30.5% conversion improvement
Social: 19% conversion improvement

@Thibaut

This comment has been minimized.

Show comment
Hide comment
@Thibaut

Thibaut May 3, 2016

Member

@erlgry You can subscribe to our partner and developer newsletters here and here, but I'm not sure if we'll announce those changes there. I'll post here just in case.

@j0rd those are interesting numbers. Deploying progressive JPEG across all images is a risky change which we probably won't do (it increases the file size and some research suggests it doesn't make users "happier" compared to sequential JPEGs), but we may add an option to request progressive JPEGs from our CDN.

Member

Thibaut commented May 3, 2016

@erlgry You can subscribe to our partner and developer newsletters here and here, but I'm not sure if we'll announce those changes there. I'll post here just in case.

@j0rd those are interesting numbers. Deploying progressive JPEG across all images is a risky change which we probably won't do (it increases the file size and some research suggests it doesn't make users "happier" compared to sequential JPEGs), but we may add an option to request progressive JPEGs from our CDN.

@erlgry

This comment has been minimized.

Show comment
Hide comment
@erlgry

erlgry May 4, 2016

@Thibaut thanks for the links.

erlgry commented May 4, 2016

@Thibaut thanks for the links.

@erlgry

This comment has been minimized.

Show comment
Hide comment
@erlgry

erlgry Jul 16, 2016

@Thibaut is there any update on this front? If the improvement is in, should I try re-uploading my product images?

erlgry commented Jul 16, 2016

@Thibaut is there any update on this front? If the improvement is in, should I try re-uploading my product images?

@Thibaut

This comment has been minimized.

Show comment
Hide comment
@Thibaut

Thibaut Jul 16, 2016

Member

@erlgry we improved the JPG compression of images that are being requested in a certain size (with _small, _large, etc. suffixes in the URL). The JPG compression of original images is unchanged. You may need to re-upload your images for this to take effect.

We also just released a much improved img_url liquid filter (I was going to update this thread next week): https://help.shopify.com/themes/liquid/filters/url-filters#custom-image-sizes You can now generate image URLs with custom sizes, cropping, and even progressive JPGs.

If you have feedback or questions, please use our forum. This thread/repo is about Liquid itself, not Shopify.

cc @j0rd

Member

Thibaut commented Jul 16, 2016

@erlgry we improved the JPG compression of images that are being requested in a certain size (with _small, _large, etc. suffixes in the URL). The JPG compression of original images is unchanged. You may need to re-upload your images for this to take effect.

We also just released a much improved img_url liquid filter (I was going to update this thread next week): https://help.shopify.com/themes/liquid/filters/url-filters#custom-image-sizes You can now generate image URLs with custom sizes, cropping, and even progressive JPGs.

If you have feedback or questions, please use our forum. This thread/repo is about Liquid itself, not Shopify.

cc @j0rd

@erlgry

This comment has been minimized.

Show comment
Hide comment
@erlgry

erlgry Jul 16, 2016

Wow, thanks. That's awesome!!!. I will give this a try.

Any info on what kind of improvement can we expect with the JPG compression on the _large images?

erlgry commented Jul 16, 2016

Wow, thanks. That's awesome!!!. I will give this a try.

Any info on what kind of improvement can we expect with the JPG compression on the _large images?

@Thibaut

This comment has been minimized.

Show comment
Hide comment
@Thibaut

Thibaut Jul 16, 2016

Member

Sizes smaller than 1024 pixels (_large is 480x480) should see a 30-60% reduction in file size compared to a couple months ago. Larger images use a higher quality compression ratio, but should still see a 20-40% improvement.

Member

Thibaut commented Jul 16, 2016

Sizes smaller than 1024 pixels (_large is 480x480) should see a 30-60% reduction in file size compared to a couple months ago. Larger images use a higher quality compression ratio, but should still see a 20-40% improvement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment