liquid icon indicating copy to clipboard operation
liquid copied to clipboard

render vs include

Open mfs-mindsize opened this issue 3 years ago • 13 comments

Pardon me for being a bit late on this issue. While I understand the need for render and the benefits, I don't understand why include is being deprecated. Render is obviously a subset of include and therefore not exactly a replacement for include.

Specifically, I've used include as a way to mimic global values. That is, define a single file for globals and include that file as necessary. Include can kinda sorta function as a function - which I suppose is why there's render now (which again, makes sense for its intent) - but that "two way" communication of values (from child to parent) is gone.

What's the roadmap look like? How is include going to be fully replaced, because render is a significantly crippled version of include?

mfs-mindsize avatar Sep 01 '20 19:09 mfs-mindsize

https://github.com/Shopify/liquid/pull/1122#issuecomment-517758787 provides some context on why the render tag was introduced

Some context about why we're introducing this for those outside Shopify: over the years we've found many problems and limitations with include:

  • include introduces dependencies between files, which makes solving certain problems such as automated upgrades hard or impossible, and is the cause of much complexity in our themes;
  • dynamic includes ({% include variable %}) prevent certain performance optimizations;
  • there are number of annoying issues with include (#911 #821 #1002 #293) which we're unable to fix due to our promise for backward compatibility.

As part of the new online store design experience that was announced at Unite, we're introducing three new type of section files in our themes, where we're planning to remove support for include. We intend to deprecate include in favor of render going forward.

As I mentioned in that issue, the include tag did seem to be used in a similar way as a function. However, I think the include tag actually behaves more like a macro expansion, which results in a lot of implicit coupling between the snippet and where it is included. That means in order to understand how it is safe to use the snippet, it requires understanding what the snippet does and understanding how it is safe to modify the snippet requires understanding the contexts in which it is used.

What's the roadmap look like?

As mentioned in the above comment, the include tag is being prevented for these new section files. Sections have configuration data that can be used for immutable shared global state, so perhaps that helps with your use case.

If you are concerned about using the include tag outside of section files where it is already supported, then you will also notice in the above quoted comment that we promise backwards compatibility, so the deprecation doesn't mean it will be removed in the future where it is already supported.

dylanahsmith avatar Sep 12 '20 17:09 dylanahsmith

I understand some of the issues. I understand why. However, render is not a replacement. It's a very limited subset of "features" when compared to include.

What is planned to replace those "features"?

"That means in order to understand how it is safe to use the snippet, it requires understanding what the snippet does and understanding how it is safe to modify the snippet requires understanding the contexts in which it is used."

Not to sound bitter / snarky but isn't this what developers / engineers do? Taking away responsibility from everyone because it's beyond the efforts of some feels short-sighted. It's our job to understand how our applications work, how the pieces fit together and so on.

I understand this is not most non-techincal store owners. But what about the rest of us? What about those who are going to push Shopify forward? To higher hights? How does tying our hands help? Shopify or us?

This is really disappointing.

mfs-mindsize avatar Sep 14 '20 13:09 mfs-mindsize

Put another way, there are things that include can do that render can not. What other tags are planned to backfill the needs that include fulfills? Ignoring those needs does not make them go away.

Is the message here that we should abandon liquid and adopt a pure frontend framework architecture? It certainly feels that way. Fair enough. But if that's the case then that would be good for the rest of us to know, eh?

mfs-mindsize avatar Sep 14 '20 13:09 mfs-mindsize

From your response, I gather that you are facing this limitation within Shopify theme sections which has this restriction and that you don't feel like configurable theme settings satisfy your use case. However, I'm still not clear on your use case.

Note that I'm just trying to provide context and clarify the issue, I was not trying to send a message beyond that.

dylanahsmith avatar Sep 14 '20 13:09 dylanahsmith

Long to short, we use an env.liquid. That keeps critical values out of the repo and out of the DB. This is used in conjunction with a couple other patterns we've developed - which also reply on include

All in all, despite the flaws in include from your POV, include enables us to use modern best practices in the content of Shopify. As I see it, at this point, our only alternative is to come up with an app that uses the Asset API and then we'll "injecting" the args we need, as we need them. But gawd, that seems like a long way to go simply to follow industry best practices.

mfs-mindsize avatar Sep 14 '20 14:09 mfs-mindsize

To draw inspiration from the unix world, perhaps a good solution for this use case could be:

  • an export tag that pushes the specified variable up to the calling context (similar to export in bash)
  • an option to render that allows variables to be exported (similar to source in bash)

Can you explain how you're using env.liquid in a bit more detail? Maybe another solution could involve improvements to the theme setting system.

On the topic of "tying hands", I find variable scoping and modularity restrictions like this to be even more useful when you have a huge codebase with a lot of developers. The change here is intended to be a forward evolution, like going from C macros and functions in one giant namespace, to encapsulated Rust modules (for example). Though it seems there are some unresolved use cases to consider.

pushrax avatar Sep 14 '20 21:09 pushrax

@pushrax - Yes. That's exactly the problem with render, it only work in one direction.

I understand why render is. It makes sense. My rub is that it is not a replacement for include. Not even close. It's that include isn't being discouraged, it's being killed off. We have no choice. The Shopify Nanny State is saving us from ourselves (even if we're aware of include's implications).

Our env.liquid is a case statement. We could have given each value it's own var name but - to render's point - we wanted a light footprint, wanted avoid variable "pollution", mitigate conflicts, etc. It's easier to use something like api_key as a variable name, and then set that based on the when case.

Example - When we need something from the env, we specify which case / when in the include's with. This way the env is only setting the vars for that particular need. Those variable name + value pairs are effectively passed back to the parent (where the include is).

The parent then does whatever it is it needs to do. For example, load the Google Tag Manager snippet using the account_id that's in the env. That account_id isn't in the DB (read: theme settings, which get exported), nor is it in the code that's committed to the repo.

Without include's current scope, we're being put into a very bad position. Something that is completely contradictory to a modern dev workflow.

mfs-mindsize avatar Sep 21 '20 15:09 mfs-mindsize

@dylanahsmith said "n Shopify theme sections which has this restriction and that you don't feel like configurable theme settings satisfy your use case. "

The issue is, theme settings are part of the export. We, on the other hand, what something closer to a .env where critical values are unique to the environment stay protected (?) within that environment. As they should be ;)

mfs-mindsize avatar Sep 21 '20 15:09 mfs-mindsize

I believe env.liquid will be exported with the theme. It will also be stored in the DB along with other Liquid files used to render the page. There is no way to make data accessible to Liquid in Shopify without storing it in the DB. I would certainly not recommend putting your banking secrets in here, but for things like GTM account IDs, which are accessible by anyone who reads the HTML source of a storefront page, it is appropriate.

When managing a theme with a repository, it definitely makes sense to not want to check in data like this. Presumably you are either manually uploading the theme to Shopify after making changes, or using an automated process that adds env.liquid from private storage. In either case, modifying the theme settings JSON when deploying the theme should be fairly straightforward.

Some sort of well-namespaced shop settings that are accessible by themes but not coupled to themes seems directly useful for what you are describing, if theme exports are the main concern.

pushrax avatar Sep 25 '20 18:09 pushrax

"I believe env.liquid will be exported with the theme." Yes, unfortunately, that's true. But at least it's not in our repo. To your point, it would be wise of Shopify to add an env folder something that is not exported.

" It will also be stored in the DB along with other Liquid files used to" But that's something we can avoid due to the nature of the platform. But in a file - as is typical of a .env - is better than part of the UI. Clients are less likely to find the env. Again, we're doing the best we can given the platform. Our coming up short is more about the platform than our desire to want to follow industry best practices.

Presumably you are either manually uploading the theme to Shopify Nope. We use Buddy.works. It takes a branch from the repo and sends it to Shopify.

if theme exports are the main concern. Our concerns are:

  1. Keep credential out of the repo
  2. Abstract and decouple as much as possible. Because this is better than a monolithic code base.

Long to short, render is not a 1-for-1 replacement of include.

mfs-mindsize avatar Sep 25 '20 18:09 mfs-mindsize

Wouldn't the better way to set environment variables be through metafields using an app like Advanced Custom Fields? You can then access them anywhere using a syntax like:

shop.metafields.env.myvar

This keeps the environment completely outside code (which is where environment variables should ideally reside anyway), and it allows you to use render tag.

See the documentation here.

ADTC avatar Mar 09 '21 01:03 ADTC

I know this is late in the game and all but...

Aside from the .env-style usage @mfs-mindsize discusses, there are other critical uses for include that will be lost with render. For example, we use a snippet product-status that gets included anywhere a product is used on the site. It goes through all the product's properties, tags, metafields, etc, and sets a series of variables, arrays, classlists, and booleans that can be used for the unique placement of that product. This is CRITICAL to how our site works, and without the parent template having access to the snippet's variables, the only other way to handle this is to manually rewrite this complex block of code with EVERY instance of a product on the site. In a world where DRY is king, this is an insane option.

In a better CMS, we'd be able to query exactly what we need at the render point in the template. But Shopify doesn't allow custom queries, so we're stuck having to process the firehose of info from liquid tags within the templates. If we can't reuse that code with include, our templates are going to be exponentially harder to maintain, having to find/replace code blocks for updates, and hoping all the devs are formatting the code blocks exactly the same way.

While render definitely can offer some benefits, and in most cases be a good alternative to include, it's NOT a viable replacement for include. It's very disappointing that the Shopify team can't see that.

bootsified avatar May 28 '21 17:05 bootsified

We have the exact same issue that @bootsified is facing, except it's a bit smaller since we "only" need to copy ~10 lines of code. That being said, this still sucks a lot.

This only affects snippets though, using include inside non-snippets works fine.

Liquid error (snippets/... line 1): include usage is not allowed in this context

ciriousjoker avatar Dec 09 '22 13:12 ciriousjoker