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

Replace DotLiquid with Scriban templating engine #188

Closed
tatarincev opened this issue Jan 10, 2019 · 14 comments
Closed

Replace DotLiquid with Scriban templating engine #188

tatarincev opened this issue Jan 10, 2019 · 14 comments
Assignees

Comments

@tatarincev
Copy link
Contributor

tatarincev commented Jan 10, 2019

The working branches

https://github.com/VirtoCommerce/vc-storefront-core/tree/features/scriban-migration
https://github.com/VirtoCommerce/vc-theme-default/tree/features/scriban-migration

The comparison performance benchmark

200 concurrent users. 3 minutes test duration.
image

The known theme breaking changes:

  • The specialized form tag has been removed {% form 'login' %}{% endform %} -> <form accept-charset="UTF-8" action="{{ '~/account/login' | absolute_url }}" method="post" id="customer_login" name="customer_login"></form>

  • The pagination tag has been replaced to pipe function {% paginate collection.products by 5 %}{% endpaginate %} -> {{ paginator = collection.products | paginate 20 }} {% if paginator.pages > 1 %} {% include 'pagination-custom' %}

  • The special variable blank has a new behavior {{ variable != blank }} before equal to variable == null OR (IEnumerable)variable.Any(). Must be replaced to {{ variable.empty? == false }} for collections.
    The 'blank' alias of 'empty' special variable scriban/scriban#107

  • Include statement doesn't support {% include 'snippet' param1: aaa %} -> {% assign param1 = aaa %}{% include 'snippet' %}
    {% Include %} - is it possible to create variables on the same line scriban/scriban#106

  • current_tags contains tag should be replaced to current_tags[tag.value].empty? == false

  • named arguments don't support for functions are called as pipe and by direct call 'localization_key' | t: param1 : 'a', param2: 'b' -> 'localization_key' | t: 'a', 'b'
    https://github.com/lunet-io/scriban/blob/master/doc/liquid-support.md#known-issues

  • Remove '-' from liquid operator brackets {%- comment -%} -> {% comment %}

  • replaced interpolation in localization resources from Liqud expression to string format
    From
    en.default.json

...
 "less": "Under {{ less }}"
...

To

...
  "less": "Under {0}",
...
@ilyawzrd
Copy link
Contributor

One more change:

  • Scriban doesn't have support for else tag inside for loop. I think we should check object with .empty? before declaring for loop.
    Example:
    {% if collection.products.empty? %} <div class="grid-item"> <p>{{ 'collections.general.no_matches' | t }}</p> </div> {% else %} {% for product in collection.products %} {% include 'product-list-item' %} {% endfor %} {% endif %}

@trueboroda
Copy link
Contributor

trueboroda commented Jun 5, 2019

forloop object doesn't exist into context of the scriban for loop. We can use sciban special loop variables and custom variables with math functions. Below is a list of matches.
liquid -> scriban

  • forloop.first -> for.first
  • forloop.index -> none (for.index | plus: 1)
  • forloop.index0 -> for.index
  • forloop.last -> for.last
  • forloop.length -> for.length or \<collection\>.size
  • forloop.rindex -> none (for.rindex | plus: 1)
  • forloop.rindex0 -> for.rindex

@trueboroda
Copy link
Contributor

trueboroda commented Jun 17, 2019

For static content such us pages and blogs root routing does not work. For example we have to use '/pages/about_us' instead of '~/pages/about-us'. In that case page rendering is ok.
To avoid this use absolute_url filter.

{% for link in linklists[settings.footer_legallinks_linklist].links %}
      <li><a href="{{ link.url | absolute_url }}">{{ link.title }}</a></li>
{% endfor %}

@trueboroda
Copy link
Contributor

An realizaion of liquid "date" filter don't support strftime format. It is necessary to use a standard format for the .net framework. https://docs.microsoft.com/en-us/dotnet/standard/base-types/custom-date-and-time-format-strings

@trueboroda
Copy link
Contributor

trueboroda commented Jun 18, 2019

The WorkContext propertycurrent_page has a completely different meaning than before in ranges of shopify work context . Previously, this meant the current page number in pagination. The property now stores the object of the current static page.

{% if current_page != 1 %}
      {{ 'general.meta.page' | t: current_page }}
{%endif%}

It need to be replaced with

{% if page_mumber != 1 %}
      {{ 'general.meta.page' | t: page_mumber }}
{%endif%}

@trueboroda
Copy link
Contributor

trueboroda commented Jun 19, 2019

This is a special case of this case.
Liquid string operator contains works differently than befor. Sometimes its usage occour NullReferenceException. To avoid there is two ways:

  • check string object for null befor use contains
  • use Scriban function string.contains.

{%if article.excerpt contains "<iframe" %} it is throw exception if article.excerpt is null.

Ways to repair it.

  1. More prefered. Clear liquid syntax
    {%if article.excerpt %}
    {%if article.excerpt contains "<iframe" %}
    ...
  2. Scriban syntax usage
    {%if article.excerpt | string.contains "<iframe" %}

@trueboroda
Copy link
Contributor

trueboroda commented Jun 28, 2019

Scriban do not understand HTML comment as text that don't need to be parsed. To avoid parsing such comments we need to wrap them by liquid comment tag.
<!--<p><some_wrong_liqued_expressoin></p>--> - potentially may occur error. For example
Wrapped by liquid comment tag is Ok.

<!--
{%comment%}
<p><some_wrong_liqued_expressoin></p>
{%endcomment%}
-->

@trueboroda
Copy link
Contributor

trueboroda commented Jun 28, 2019

To obtain the necessary links from linklists object of data context, we must use an indexer with the name of the list, than just to use the named property of the linklists context object as befor.
{% for link in linklists.main-menu %} -> {% for link in linklists['main-menu'].links %}

@trueboroda
Copy link
Contributor

trueboroda commented Jun 28, 2019

We must to avoid a situation when a part of the liquid exspression can be null. In that case we should use a previusly checking for null value, after that we can perfom any expression.
See the following examples.
1) {%if article.excerpt contains "<iframe" %} it is throw exception if article.excerpt is null.
To repaire it need to add null checking.
{%if article.excerpt %}
{%if article.excerpt contains "<iframe" %}
What is important! In that case also we can not unite this conditions and make check in single if statement like {%if article.excerpt and article.excerpt contains "<iframe" %}. It is also occur the error.
2) {% unless linklists['main-menu'].links.first.url == '/' %} If linklists don't have such list as 'main-menu' or linklists['main-menu'].links is empty array NullRefferenceEception whould be thrown.

@trueboroda
Copy link
Contributor

trueboroda commented Jun 28, 2019

For checking the var to null value we reccomend to use direct usage the var as a condition operand.
For example {%if article.excerpt %}. null == false; not null == true. An internal function empty? (article.excerpt.empty? == true) also will be work for such cases, as well as comparing with an internal empty var (article.excerpt == empty). But for some reasons sometimes empty var and empty? func works wrongly.
Recomended way. Also this is the more short sintax way.

{% if src.empty? == false %}
        <div class="excerpt responsive-video">
          <iframe width="100%" scrolling="no" frameborder="no" src="{{ src }}"></iframe>
        </div>
{% endif %}

Not recommended
{% if src.empty? == false %}
{% if src != empty %}

@trueboroda
Copy link
Contributor

trueboroda commented Jun 28, 2019

Significantly changed the names of objects responsible for working with the faceted search (sidebar filters).
collection.all_tags.groups => product_search_result.aggregations.
current_tags => current_product_search_criteria.terms.
For more understanding see code of sidebar filters snippet befor, on dotLiquid, and after migration, on Scriban.
dotLiquid markup.
Scriban markup.

@trueboroda
Copy link
Contributor

trueboroda commented Jun 28, 2019

Now the Json liquid filter use camalCase naming strategy instead of snake_case strategy.

@ilyawzrd
Copy link
Contributor

ilyawzrd commented Jul 1, 2019

We have to write conditional expressions in one line,otherwise we will get an error "Expecting EOL"
{% elsif page.url contains 'glossary/magento-alternatives' or page.url contains 'glossary/best-ecommerce-platforms' or page.url contains 'glossary/shopify-alternatives' or page.url contains 'glossary/nopcommerce-alternative' or page.url contains 'glossary/elastic-path-alternatives' or page.url contains 'glossary/znode-alternatives' or page.url contains 'glossary/3dcart-alternatives' or page.url contains 'glossary/demandware-alternatives' or page.url contains 'glossary/episerver-alternatives' %}

@artem-dudarev artem-dudarev changed the title Replace DotLiquid to Scriban templating engine Replace DotLiquid with Scriban templating engine Jul 1, 2019
@tatarincev
Copy link
Contributor Author

@trueboroda trueboroda unpinned this issue Oct 4, 2019
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