Shopify / liquid

Liquid markup language. Safe, customer facing template language for flexible web apps.
https://shopify.github.io/liquid/
MIT License
11.13k stars 1.39k forks source link

Proposal: Add `end` tag support #1823

Closed charlespwd closed 2 months ago

charlespwd commented 2 months ago

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 raw and comment tags around nesting, those still require a complete endraw and endcomment tag 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 comment and raw exceptional?

Because you should be able to put a comment anywhere in the code and close it anywhere else. If end closes the comment, then that's no longer possible.

Also, it would be impossible to make this work:

assert_template_result(
  '{% comment %} test {% end %}',
  '{% raw %}{% comment %} test {% end %}{% end %}',
)
charlespwd commented 2 months ago

Pinging @Shopify/guardians-of-the-liquid for thoughts and potential blind spots.

jstorie commented 2 months ago

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!

tyleralsbury commented 2 months ago

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.

mattsodomsky commented 2 months ago

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!

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.

isaacbowen commented 2 months ago

*thinks about other Liquid implementations, all of which are for host languages other than Ruby

isaacbowen commented 2 months ago

@tobi 👋 ❓

Maaarcocr commented 2 months ago

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.

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?

mattsodomsky commented 2 months ago

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.

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.

charlespwd commented 2 months ago

@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 end tag, there's a bunch of minor wins devs would get:

More "feely/aesthetic" arguments:

mattsodomsky commented 2 months ago

@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 end tag, there's a bunch of minor wins devs would get:

  • You can swap an if for an unless for easy debugging without having to find the matching endif/endunless
  • Syntax resembles ruby. Less visual noise.
  • We could have a prettier-plugin-liquid rule that would prefer one over the other (default to false for a while, tbd if we change in the next major version of the plugin)

More "feely/aesthetic" arguments:

  • Makes it feel a bit more modern. You usually find syntax like this in older languages like Fortran.

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/

jg-rp commented 2 months ago

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.

You can swap an if for an unless for easy debugging without having to find the matching endif/endunless

I’d rather solve this with a logical not operator and parentheses for grouping in if/unless expressions.

Syntax resembles ruby

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.

Less visual noise

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.

charlespwd commented 2 months ago

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.

This PR is not mutually exclusive to this other proposal ^^ this one is far easier to implement though.

ianks commented 2 months ago

👏🏻 thank you @charlespwd

benjaminsehl commented 2 months ago

lfg

MurmeltierS commented 2 months ago

this could be one of those we’ll all regret somewhere down the line

kinngh commented 2 months ago

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.

panoply commented 2 months ago

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.

geongeorge commented 2 months ago

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. The problem is liquid is more often written intertwined with html and even JS in some cases. I do not mind this as an optional method but I'm sure people starting to use this would just create more confusion

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

image
jg-rp commented 2 months ago

Note that this PR was based on and merged into the old master branch, not the active main branch.

panoply commented 2 months ago

Introducing an {% end %} syntax is akin to suggesting an </> as a closing tag in markup. It's about as practical as the subset of HTML implicitly closed tags. Sure, nice to have but necessity is mostly void (no pun intended).

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 {% endtag %} and the implicit {% end %} introduces a significant amount of ambiguity about what constitutes correctness. This ambiguity undermines the natural intuitiveness of the language. The rationale behind the decision might make sense when considering it from the viewpoint of serving a community of seasoned developers. However, this approach doesn't align with the reality, as we're discussing a template language primarily used by developers who are often novices, merchants and similar.

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

mattsodomsky commented 2 months ago

@charlespwd @ianks

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).

ianks commented 2 months ago

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.

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).

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.

kinngh commented 2 months ago

@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^

TeamDijon commented 2 months ago

Let's go with more RFC 😍

mattsodomsky commented 2 months ago

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.

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).

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

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.