-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Proposal: Add end tag support
#1823
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
Conversation
356312a to
ff13e82
Compare
- Not allowed for `comment` nor `raw`
ff13e82 to
354a0f6
Compare
|
Pinging @Shopify/guardians-of-the-liquid for thoughts and potential blind spots. |
| assert_template_result( | ||
| '', | ||
| '{% comment %} {% if true %}{% end %} {% endcomment %}', | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't think we need this test here since it doesn't have to do with raw tag
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
true that! will move
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM from a technical perspective. My only concern was backwards compatibility since we default to supporting the end tag, but AFAIK the usual pattern for breaking the rules around tag scope is to extend Raw, which is opted out.
|
Devils advocate here :) I do understand this would make writing Liquid code a bit simpler, and would match up with how ruby itself works. However, since Liquid is a completely unstructured language that has no whitespace/newline requirements (unlike ruby), this change could reduce readability. In practice, themes can sometimes turn into the wild west with code from multiple sources added with no sensible indentation, and this could make it harder to visually make sense of what's going on. Just something to consider! |
I can imagine irresponsible use leading to closures in weird places... but I don't think that's enough to say "we shouldn't do this". Lots of languages let lots of developers do lots of bad things (including Liquid). It's up to the theme developer to not do those bad things, in my opinion. |
My thoughts were similar, and this is a message I wrote in our internal Slack: Yeah, it’s a good fit for a programming language but not a great fit for a templating language is my first response to this. It only started to make sense when I thought about it in the context of Ruby, but that’s not really the context we’re talking about. @charlespwd - can you talk more about the need for this and the impetus for the change? We make apps that have users write Liquid code, and so from that perspective, this will add complexity. |
|
*thinks about other Liquid implementations, all of which are for host languages other than Ruby |
|
@tobi 👋 ❓ |
If you mark all the tags as not supporting the end tag you can effectively opt out of this feature entirely, would that make it more acceptable to see this merged? |
Yes, that helps for sure :) but, still, I'd love to take a step back and hear about the why/impetus for this change. It would help my mental model. |
|
@mattsodomsky, I probably used poor wording around the PR and it might have given the impression that this was internally aligned before it actually was. My goal was to start the discussion and prove the feasibility with a PR. Specifically about the support of the
More "feely/aesthetic" arguments:
|
Awesome, thanks for the insight. Really helpful ❤️ It's helpful to know what you are seeing as the benefits. I keep returning to the fact that Liquid is a templating language rather than a programming language (this is a key difference). I think it is designed for readability and accessibility for non-programmers or less technical users. That’s why having matching opening and closing tags—much like in HTML—is important for users (and apps 😄 ). I haven’t found any other templating languages that don’t enforce matching closing tags—that's not reason enough not to do it, but it might be a sign. I am willing to admit I could be wrong here, if there is high value, don't let me stop you 🤟 Here's how we are using Liquid if it helps: https://tasks.mechanic.dev/ |
karreiro
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, @charlespwd!
I personally really like this approach. Most human-friendly languages support the end keyword (such as Ruby and Lua).
I believe this helps in making the template a bit cleaner and less verbose. Keeping in mind that we may have nested if/endif statements, having the end keyword may encourage writers even more to rely on indentation to organize their code.
Still, before merging, as you mentioned, I believe it's worth involving a broader audience, since Liquid has been written differently for years, and we want to make sure that most Liquid users are happy with this change.
Thanks again for this proposal, and for demonstrating its viability! 🔥🚀
|
I agree with what others have said regarding the potential for reduced readability, if universal end tags are misused. The opposite is not necessarily true, in my opinion. “Good” use of universal end tags is not likely to improve readability, just save a few characters. A bigger issue for me would be that of poorer error reporting. I can imagine a scenario (perhaps an uncommon one) where it’s impossible to identify (or misidentify) the location of an error due to accidental, superfluous nested universal end tags, leading to a prematurely closed parent block.
I’d rather solve this with a logical
I'd be interested to hear if this is a goal shared by everyone, as I'd always considered Liquid to be independent of the language it's implemented in, although influenced by Ruby.
If concise syntax (shorter, without sacrificing readability) is a goal, then the often requested “inline conditions” (or “ternary expressions” if you prefer) feature would have more of an impact. Again, just my opinion, and I realise these things are not necessarily trivial to implement in a non-breaking manner. 😀 P.S. For what it's worth, running this PR against the Golden Liquid test suite shows no breaking changes. |
This PR is not mutually exclusive to this other proposal ^^ this one is far easier to implement though. |
|
👏🏻 thank you @charlespwd |
|
lfg |
|
this could be one of those we’ll all regret somewhere down the line |
|
This is merged but I still wanted to add my opinions / thoughts to this discussion, and I hope I'm not too late. This has already been said multiple times, but Liquid is a templating language and not a programming language. I want to add on to this by saying Liquid is not Ruby. What worked with Ruby will not necessarily work with Liquid. In the real world and outside of our IDEs, the merchants work with multiple developers and the good ol' blame game of the other developer messing up the theme code is going to have yet another point, more safety rails are going to be put in, merchants who try to be developers are more prone to breaking their stores and more importantly, this removes a safety rail that was put in by pure coincidence. This takes a major hit on readability and maintainability, which, again, has been discussed already. The whole argument of "Bad developers do bad things it's not the language's problem" is a bad argument since this isn't about adding a new safety rail, it's about removing an existing one. There are bigger problems to solve instead of renaming a tag. |
|
I agree with @kinngh. Sometimes verbosity is a good thing, especially in a template language Like Liquid. There is no added benefit here. Bad PR overall. |
|
I also agree with @kinngh here. Liquid as a templating engine did not need this. The only place I think this might make a devs life easier are in {% liquid %} tags. Like @mattsodomsky we also have apps that lets users write their own liquid. Look at what Shopify's own email notification editor does inside capture tags, no highlighting. It would be a nightmare to match the end tags |
|
Note that this PR was based on and merged into the old |
|
Introducing an There is a misalignment here. This new syntactic does not sufficiently account or consider the exhaustive nesting and structural depths inherent with Liquid or how it's implemented by developers. If anything, it completely neglects the appropriated syntactical structures apparent in 90% of Shopify themes. The only outcome here beyond the sugar extrapolation is likely to be the incurred readability issues. The argument for "linting" tools is also assumptive because only a small percent of developers are actually using them, but in any sense they should not be dictating implementation into a language. Optionality between the explicit It's important to note that Liquid employs keyword control grammar for its execution, rather than relying on character termination (outside of delimiter formation). Therefore, it's crucial to balance this approach, and its imperative to respect consistency as maintainability can be compromised in applied usage. While I can (personally) reason with this when considering it from the perspective of someone with a more well versed understanding of programming (and to a degree I'd champion something like this in most cases) but I am unconvinced of its merits in practicality for Liquid. I am unable to formulate compelling counter arguments in its favor. ~ Original Post on X |
|
Is there an RFC process for this kind of thing, or could one be introduced? Also, are there any non-Shopify maintainers involved? (Genuinely curious—I’m not sure.) I’m interested in how we can incorporate community feedback, particularly from those actively using the templating language. Users often bring perspectives that developers or product managers might not foresee without speaking to them (I encounter this frequently in my own work). |
|
Just want to say that I appreciate all of the commentary here. This feedback has been incredibly valuable, and it's apparent we need to find a better way to close the communication gap with devs on Liquid.
As of now, there has been no official RFC process for changes to Liquid but it's absolutely something we need to consider moving forward if we want to evolve the language. In general, given the vast amounts of Liquid code in the wild, making changes to the language is tricky to get right, so I think having an open process to discuss changes makes a lot of sense. I'm heading OOO for a week, and am going to revert the PR for now. |
|
@ianks really appreciate you acting on this quickly! Streamlining the RFC process would be really great so we can add feedback before it's merged^ |
|
Let's go with more RFC 😍 |
Thank you so much for all your work, thoughtfulness, and approach to this. Enjoy the time off! I wonder if part of the RFC process for high-impact changes could involve an advisory board or something similar to represent consumers/users of the project. GitHub issues are great and will always have their place, but sometimes they feel more like debates than structured feedback. |

This PR demonstrates support for the generic and shorter
{% end %}tag.Behaviour
When used, it will close the closest tag that wasn't closed.
{% # still valid %} {% if true %} {% unless false %} some stuff {% endunless %} {% endif %} {% # new! ✨✨ %} {% if true %} {% unless false %} yay! {% end %} {% end %}Because of the exceptional behaviour of the
rawandcommenttags around nesting, those still require a completeendrawandendcommenttag to close.{% # still works %} {% comment %} {% end %} {% endcomment %} {% # still works %} {% raw %} {% comment %} {% end %} {% endcomment %} {% endraw %} {% # won't works %} {% raw %} {% comment %} {% end %} {% end %} -- this one would close the raw tag. {% end %} {% comment %} In other words, end doesn't close endraw or endcomment {% endcomment %}Why make
commentandrawexceptional?Because you should be able to put a comment anywhere in the code and close it anywhere else. If
endcloses the comment, then that's no longer possible.Also, it would be impossible to make this work: