Shopify / prettier-plugin-liquid

Prettier Liquid/HTML plugin
https://npm.im/@shopify/prettier-plugin-liquid
MIT License
93 stars 15 forks source link

Liquid if HTML tag name #141

Closed charlespwd closed 10 months ago

charlespwd commented 1 year ago

Unformatted source

<{%if swipe%}product-slider-component{% else %}div{% endif %}>
</{%if swipe%}product-slider-component{% else %}div{% endif %}>

Expected output

<{% if swipe %}product-slider-component{% else %}div{% endif %}>
</{% if swipe %}product-slider-component{% else %}div{% endif %}>

Actual output

LiquidHTMLParsingError
charlespwd commented 10 months ago

Won't fix. No intention to do this since folks can also put attributes in that if statement.

saiichihashimoto commented 10 months ago

What's a way to accomplish this that is supported? (besides copy/pasting the entire element to put in both the if and else)

charlespwd commented 10 months ago

Option 1.

{% if swipe %}
  <product-slider-component with props that only this gets>
{% else %}
  <div>
{% endif %}

{% # content... %}

{% if swipe %}
  </product-slider-component>
{% else %}
  </div>
{% endif %}

Option 2.

{% liquid
  if swipe
    assign wrapper = "product-slider-component"
    assign props = "..."
  else 
    assign wrapper = "div"
    assign props = ""
  endif
%}
<{{ wrapper }} {{ props }}>
  {% # content... %}
</{{ wrapper }}>
saiichihashimoto commented 10 months ago

For Option 1 I still receive a LiquidHTMLParsingError.

I'll try out Option 2

charlespwd commented 10 months ago

For Option 1 I still receive a LiquidHTMLParsingError.

What version are you using? What's the exact error? Example code? Shouldn't be happening. If it is, that's a bug!

saiichihashimoto commented 10 months ago
  {%- if media.media_type == 'model' -%}
    <product-model class="deferred-media media media--transparent gradient global-media-settings no-js-hidden" style="padding-top: 100%" data-media-id="{{ media.id }}">
  {%- else -%}
    <deferred-media class="deferred-media gradient global-media-settings media no-js-hidden" style="padding-top: {{ 1 | divided_by: media.aspect_ratio | times: 100 }}%" data-media-id="{{ media.id }}">
  {%- endif -%}
  <button id="Deferred-Poster-Modal-{{ media.id }}" class="deferred-media__poster" type="button">
    <span class="deferred-media__poster-button motion-reduce">
      {%- if media.media_type == 'model' -%}
        <span class="visually-hidden">{{ 'products.product.media.play_model' | t }}</span>
        {%- render 'icon-3d-model' -%}
      {%- else -%}
        <span class="visually-hidden">{{ 'products.product.media.play_video' | t }}</span>
        {%- render 'icon-play' -%}
      {%- endif -%}
    </span>
    <img
      srcset="{% if media.preview_image.width >= 493 %}{{ media.preview_image | image_url: width: 493 }} 493w,{% endif %}
        {% if media.preview_image.width >= 600 %}{{ media.preview_image | image_url: width: 600 }} 600w,{% endif %}
        {% if media.preview_image.width >= 713 %}{{ media.preview_image | image_url: width: 713 }} 713w,{% endif %}
        {% if media.preview_image.width >= 823 %}{{ media.preview_image | image_url: width: 823 }} 823w,{% endif %}
        {% if media.preview_image.width >= 990 %}{{ media.preview_image | image_url: width: 990 }} 990w,{% endif %}
        {% if media.preview_image.width >= 1100 %}{{ media.preview_image | image_url: width: 1100 }} 1100w,{% endif %}
        {% if media.preview_image.width >= 1206 %}{{ media.preview_image | image_url: width: 1206 }} 1206w,{% endif %}
        {% if media.preview_image.width >= 1346 %}{{ media.preview_image | image_url: width: 1346 }} 1346w,{% endif %}
        {% if media.preview_image.width >= 1426 %}{{ media.preview_image | image_url: width: 1426 }} 1426w,{% endif %}
        {% if media.preview_image.width >= 1646 %}{{ media.preview_image | image_url: width: 1646 }} 1646w,{% endif %}
        {% if media.preview_image.width >= 1946 %}{{ media.preview_image | image_url: width: 1946 }} 1946w,{% endif %}
        {{ media.preview_image | image_url }} {{ media.preview_image.width }}w"
      src="{{ media | image_url: width: 1946 }}"
      sizes="(min-width: {{ settings.page_width }}px) {{ settings.page_width | minus: 100 | times: media_width | round }}px, (min-width: 990px) calc({{ media_width | times: 100 }}vw - 10rem), (min-width: 750px) calc((100vw - 11.5rem) / 2), calc(100vw - 4rem)"
      {% unless lazy_load == false %}loading="lazy"{% endunless %}
      width="973"
      height="{{ 973 | divided_by: media.preview_image.aspect_ratio | ceil }}"
      alt="{{ media.preview_image.alt | escape }}"
    >
  </button>
  <template>
    {%- liquid
      case media.media_type
      when 'external_video'
        assign video_class = 'js-' | append: media.host
        if media.host == 'youtube'
          echo media | external_video_url: autoplay: true, loop: loop, playlist: media.external_id | external_video_tag: class: video_class, loading: "lazy"
        else
          echo media | external_video_url: autoplay: true, loop: loop | external_video_tag: class: video_class, loading: "lazy"
        endif
      when 'video'
        echo media | media_tag: image_size: "2048x", autoplay: true, loop: loop, controls: true, preload: "none"
      when 'model'
        echo media | media_tag: image_size: "2048x", toggleable: true
      endcase
    -%}
  </template>

  {%- if media.media_type == 'model' -%}
    </product-model>
    {%- if xr_button -%}
      <button
        class="button button--full-width product__xr-button"
        type="button"
        aria-label="{{ 'products.product.xr_button_label' | t }}"
        data-shopify-xr
        data-shopify-model3d-id="{{ media.id }}"
        data-shopify-title="title"
        data-shopify-xr-hidden
        >
        {% render 'icon-3d-model' %}
        {{ 'products.product.xr_button' | t }}
      </button>
    {%- endif -%}
  {%- else -%}
    </deferred-media>
  {%- endif -%}

Error:

✖ prettier --ignore-unknown --write --cache .:
[error] snippets/product-thumbnail.liquid: LiquidHTMLParsingError: Attempting to close HtmlElement 'product-model' before LiquidTag 'if' was closed
[error]   173 |   </template>
[error]   174 |
[error] > 175 |   {%- if media.media_type == 'model' -%}
[error]       |   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[error] > 176 |     </product-model>
[error]       | ^^^^^^^^^^^^^^^^^^^^^
[error]   177 |     {%- if xr_button -%}
[error]   178 |       <button
[error]   179 |         class="button button--full-width product__xr-button"

Version 1.3.1

charlespwd commented 10 months ago

Ah OK so I see what's up. This is hitting the limits of the heuristic I chose. You see, we have to strike a balance between being helpful and telling folks there's a problem with their code and allowing for problems like this. It's hard for us to do statically, so the logic I went with was the following:

“dangling” open or close HTML tags are OK if and only if:

In the snippet you provided, you have a nested if statement and a dangling close in the first branch and a dangling close in the other. So you get the default “holup you are closing something that isn't open” error.

I might relax this rule, but I'm struggling to find the balance. Especially if your case was the opposite (an open without a close), then it would be really hard to tell if that was intentional or not.

e.g. this would be difficult to tell if it was intentional or not. I'd say probably not.

{% if cond %}
  <product-model>
    {% if xr_button %}
       ...
    {% endif %}
{% endif %}