Skip to content

Conversation

@karreiro
Copy link
Contributor

@karreiro karreiro commented Jan 16, 2025

This PR implements RFC#1865 and introduces support for the new {% doc %} tag, which will be used to document elements of Liquid templates, primarily snippets.

Snippets are the primary method for reusing logic in Liquid. However, their weak interfaces make it easy to overlook dependencies when rendering them.

By adopting the emerging pattern of using headers on these snippets, the {% doc %} tag will document those interfaces. This way, tools such as the Liquid language server can support features around these definitions, such as code completion, better linting rules, and inline documentation. Here's an example of {% doc %} in action:

{% doc %}
  Renders loading-spinner.

  @param {string} foo - some foo
  @param {string} [bar] - optional bar

  @example
  {% render 'loading-spinner', foo: 'foo' %}
  {% render 'loading-spinner', foo: 'foo', bar: 'bar' %}
{% enddoc %}

<div class="{{ bar }} loading__spinner hidden">
  {{ foo | append: '.svg' | inline_asset_content }}
</div>

This new tag should behave just like the {% comment %} tag and have no effect on rendering. Therefore, to implement this, the Doc tag inherits from Comment, similar to how Unless inherits from If.

@bakura10
Copy link

bakura10 commented Jan 16, 2025

{{ 'loading-spinner-{{ foo }}.svg' |

Is this a typo? Since when such an awesome syntax is possible ? 😍

@karreiro
Copy link
Contributor Author

{{ 'loading-spinner-{{ foo }}.svg' |
Is this a typo? Since when such an awesome syntax is possible ? 😍

Oops! I just fixed that. That syntax would be interesting indeed, but it's not part of this proposal 😅

Thank you for catching that, @bakura10!

@karreiro karreiro force-pushed the liquid-doc branch 4 times, most recently from e175367 to 18d584d Compare January 21, 2025 11:36
@karreiro karreiro added the #gsd:44152 Simplify common Liquid patterns label Jan 31, 2025
Copy link
Contributor

@gmalette gmalette left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When we change the syntax of a programming language, we have to carefully consider which previously invalid programs we're making valid, and conversely which valid programs we're making invalid (aka breaking the sytnax). Making valid programs invalid is undesirable, and thus we should always strive to always only allow the smallest set of programs as valid; we always have the options to make them valid in the future.

I think we should take the opportunity to refine the spec of the doc tag more, and only allow what we actually want. This will make adding features to it much easier. Making sure that we can't parse invalid templates will prevent them from being saved, and means we won't accrue unexpected usages that become a nightmare to support in the future (hello {% comment %}).

Right now, the syntax allows for almost anything within the block body.

Valid Programs

Otherwise invalid tags

{% doc %}
  {% if %}
  {% comment %}
  {% %}
{% enddoc %}

Any attributes

{% doc %}
  @params foo
  @paramssss foo
  @potato
  @param
  @example
{% enddoc %}

Repeated attributes

{% doc %}
  @param bar
  @param bar
  @param {Product} product
  @param {ProductVariant} product
{% enddoc %}

Unspecified sytnax attribute

{% doc %}
  @param [string} this is the bar attribute | `bar`
{% enddoc %}

Attribute ordering

{% doc %}
  Just a comment

  @params foo - this is foo

  Is this just a comment or part of params?

  @example 
  {% render 'snippet', foo: true %}

  Also this, what is this?

  @param bar - this is bar

  @example 
  {% render 'snippet', bar: true %}
{% enddoc %}

Invalid Programs

AFAICT from the doc, these are the only invalid brands of doc programs:

Unclosed moustache

This invalid by virtue of forcing unclosed doc

{% doc %}
  {{
{% enddoc %}

Unclosed tag

This invalid by virtue of forcing unclosed doc

{% doc %}
  {% 
{% enddoc %}

Nested doc

Intentionally disallowed

{% doc %}
  {% doc %}
{% enddoc %}

@gmalette
Copy link
Contributor

Note, I'm not formulating an opinion on whether any of the currently valid programs should remain or should change. What I want is for the spec (and implementation) to make this determination

Copy link
Contributor

@ggmichaelgo ggmichaelgo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@GuillermoShopify I believe the doc tag should parse the tag body like the raw tag and allow incomplete Liquid codes like this:

{% doc %}
  {% assign x = ...
{% enddoc %}

With the comment tag we can't support this syntax due to backward compatibility, but we shouldn't make that a case for the new doc tag.

@gmalette gmalette self-requested a review February 12, 2025 15:40
@gmalette
Copy link
Contributor

I still disagree with the addition of this tagged as proposed, but in Slack it was discussed more and it was mentioned that @tobi already approved the tag, so I will disagree and commit.

I have dismissed my blocking review.

@PaulNewton
Copy link

{% comment #doc %}

@ggmichaelgo ggmichaelgo self-requested a review February 13, 2025 15:21
@karreiro karreiro dismissed gmalette’s stale review February 17, 2025 14:03

As Guillaume mentioned, we revisited those thoughts on Slack and have decided to dismiss that review. Valuable thoughts tho 🙇

Copy link
Contributor

@ggmichaelgo ggmichaelgo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@karreiro karreiro merged commit 17d3279 into main Feb 20, 2025
13 checks passed
@stijns96
Copy link

I think this closes Shopify/theme-tools#475 as well?

@IZ26
Copy link

IZ26 commented Apr 4, 2025

When will the new {% doc %} tag be merged? I tried using it today, but I encountered an error when attempting to upload.

@stijns96
Copy link

stijns96 commented Apr 4, 2025

Well...

Should have been released the 19th of March.

See Shopify/theme-tools#853 (comment)

They already mentioned that it could be later as well. So I actually hope any time soon.

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

Labels

#gsd:44152 Simplify common Liquid patterns

Projects

None yet

Development

Successfully merging this pull request may close these issues.