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

Don't parse nodes inside a comment tag #1755

Merged
merged 8 commits into from
Nov 29, 2023
Merged

Conversation

ggmichaelgo
Copy link
Contributor

@ggmichaelgo ggmichaelgo commented Nov 7, 2023

depends on Shopify/liquid-c#209

What are you trying to solve?

Liquid Tags inside a comment tag are getting parsed, and any Liquid tags are required to be valid.

require 'liquid'

template = Liquid::Template.parse(<<~LIQUID)
  {% comment %}
    {% assign foo = "123" %}
  {% endcomment %}
  {{ foo}}
LIQUID

puts template.root.nodelist[0] # => Liquid::Comment
puts template.root.nodelist[0].nodelist[1].class # => Liquid::Assign

Because of this parsing behaviour, the Liquid codes inside the comment tag has to be valid.
A known workaround of this issue is using a raw tag inside the comment tag:

{% comment %}{% raw %}
  {% if WIP ... %}
{% endraw %}{% endcomment %}

How are you solving this?

This PR updates the comment tag to only consume the tokens without creating child nodes.

require 'liquid'

template = Liquid::Template.parse(<<~LIQUID)
  {% comment %}
    {% assign foo = "123" %}
  {% endcomment %}
  {{ foo}}
LIQUID

puts template.root.nodelist[0] # => Liquid::Comment
puts template.root.nodelist[0].nodelist.empty? # => true

This change will improve the comment tag's Template parsing speed, and allow developers to have incomplete code inside the comment tag.

{% comment %}
  {% if 1 > 0 %}
    incomplete code...
{% endcomment %}

Legacy Support

Inside a comment tag, children comment tag and raw tags will have be valid and closed.

This comment tag inside a comment will have to be closed because {% endcomment %} tag delimiters needs to be balanced. (Closing the first comment tag in line 4 will result a syntax error)

{% comment %}
  {% comment %}
    child comment
  {% endcomment %}
{% endcomment %}

This raw tag inside a comment will have to be closed because {% endcomment %} inside a raw tag has been ignored:

{% comment %}
  {% raw %}
    {% endcomment %}
  {% endraw %}
{% endcomment %}

@ggmichaelgo ggmichaelgo force-pushed the non-parsing-comment branch 2 times, most recently from cf773ee to 8d65f2b Compare November 7, 2023 22:34
Copy link
Member

@peterzhu2118 peterzhu2118 left a comment

Choose a reason for hiding this comment

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

I'm not sure if this is within the scope of this PR, but a broken tag inside of the comment still fails.

require "liquid"

template = Liquid::Template.parse(<<~LIQUID)
  {% comment %}
    {% assign foo = "123"
  {% endcomment %}
  {{ foo}}
LIQUID

@ggmichaelgo
Copy link
Contributor Author

I'm not sure if this is within the scope of this PR, but a broken tag inside of the comment still fails.

require "liquid"

template = Liquid::Template.parse(<<~LIQUID)
  {% comment %}
    {% assign foo = "123"
  {% endcomment %}
  {{ foo}}
LIQUID

@peterzhu2118 Ah... great catch!
That should be a valid template, and I will update the PR to allow templates like that :)

@ggmichaelgo ggmichaelgo marked this pull request as ready for review November 8, 2023 20:51
@ggmichaelgo ggmichaelgo merged commit cf27877 into main Nov 29, 2023
9 checks passed
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

Successfully merging this pull request may close these issues.

4 participants