Shopify / dawn

Shopify's first source available reference theme, with Online Store 2.0 features and performance built-in.
Other
2.55k stars 3.45k forks source link

[Request] The need for a "unique_id" generation filter in Liquid #2483

Open bakura10 opened 1 year ago

bakura10 commented 1 year ago

[Disclaimer] This issue is not directly related to Dawn, but as always I don't have any better place to do platform suggestion, and this one would benefit to Dawn as well.

Hi,

Michael from Maestrooo.

As you may have heard, our theme Impact suddenly became unusable after Shopify rolled on April 1st an internal change to the platform. I would like to use this issue to give some details and suggest an improvement that should make it easier for Dawn as well to have a more robust code, and avoid the same trouble that we had to face.

The problem of ID generation

In a lot of place in a Shopify theme, we have to deal with ID generation. Such examples include form input ID, popover or drawer ID...

To do that, one typical approach in Liquid is to generate an ID like this and hoping to make it unique:

{% capture id %}input-{{ section.id }}-{{ product.id }}{% endcapture %}

However, this uniqueness can be hard to achieve under some context. For instance, a merchant may be using a featured collections section featuring two collections, with the two collection having the same product. In such cases, we would end up with the same ID.

Another issue is for generating ID in the filters. Let's take for instance the use case of collection filters in Dawn: https://github.com/Shopify/dawn/blob/main/snippets/facets.liquid#L173

Dawn uses the following pattern: Filter-{{ filter.param_name | escape }}-{{ forloop.index }}

In our theme for instance, we have a more complicated design where the facets are outputted twice: one in a sidebar, and once in a drawer. We therefore have isolated the code inside a "facets.liquid" snippet that gets re-used. So here, the Dawn approach for instance would result in having duplicate ID, which would cause problem in the case of checkboxes, for instance.

Of course, we could add a more complex ID generation rule, but it can gets very cumbersome to achieve uniqueness. Even more if, as in our theme, we abstract things in more granular snippet (for instance, Dawn encourages copy-pasting by always having to re-type the whole input with all its classes, while our theme abstract this into a snippet for cleaner consumption):

{%- render 'input', name: filter_param.name, value: filter_param.value -%}

You can't also rely on things like value, as value may contained non ASCII character, space... or whatever that are not usable in ID. Handleizing them is not a possibility neither as Japanese kanji for instance cannot be converted to handle.

When using this granularity, providing a unique ID becomes extremely hard. Dawn encourage copy-pasting over composition, which I do not feel is the best approach, and Dawn would face the same problems that we are facing if they change their architecture in the future.

The approach I found

To try to goes around this and generate unique ID, I found a "clever" approach involving getting a nanosecond timestamp from a date:

{% capture id %}checkbox-{{ 'now' | date: '%N' }}{% endcapture %}
{% capture id %}checkbox-{{ 'now' | date: '%N' }}{% endcapture %}

If you would call this twice in a row, due to the nanosecond precision, you would receive a different timestamp, and this was providing an (albeit) hacky way to generate ID, that could be used to generate aria-controls ID, input ID... without having to generate long ID and potentially having conflicts. Shopify doc stated that it supported all strftime attributes, and I relied on it:

image

Some tutorials also referred to the usage of nanoseconds (https://studiozerance.fr/blogs/shopify-conversion/generate-random-numbers-using-liquid-shopify) so it is sure that other people have used it for other use cases.

Big mistake. This worked well, until 1st of April, where Shopify rolled a change in Liquid infrastructure that broke this. Starting from 1st of April, Shopify started to return the same date for every call of {{ 'now' }}. As a consequence, the same nanosecond timestamp would be return for every call. The effect on our theme is that each checkbox, radio (which were used for variant picker)... ended up with the exact same ID, which, of course, caused a lot of problems.

Admitedly, this approach was a bit hacky and a smart workaround for a platform limitation, but it worked, and we relied on it. Shopify broke compatibility in a way where thousands of stores, including Plus merchants, would be completely broken. We had to deal with hundreds of manual fixes over a lot of files to fix merchants store as soon as possible.

Potential solution

This problem alone that impacted thousands of stores, should be a sufficient reason to try to have a built-in solution at the platform level that allows us to generate unique, predictable ID to be used for aria-controls, HTML ID attribute, without having to rely on fragile and confusing string concatenation, or on hacky solution.

React for instance has a useId hook (https://react.dev/reference/react/useId), but Shopify could add a new tag that would generate a HTML ID compliant (for instance a UUID v4). With this, generating unique ID would become easy:

{% capture checkbox_id %}checkbox-{% unique_id %}{% endcapture %}

Each call would return a new, unique ID across the whole Liquid execution. No more dealing with concatenation, conversion to handle, escaping, ID conflicts...

This would not cause any issue in regards to caching, because what we want here is just creating unique ID to generate a relationship between a button and a control (popover), a label and its associated input... so as long as it is unique, we don't care. It therefore does not suffer from the same problem of a "rand" function.

I really hope that, in regards of what happened to us, and that a feature exposed to the platform (such as the date hack I used) will always be used in unintended way, let's at least back this into the platform with a clean, proper solution.

Thanks.

PaulNewton commented 1 year ago

@bakura10 said: Starting from 1st of April, Shopify started to return the same date for every call of {{ 'now' }}.

Afaik the date now pattern has always had the possibility of returning cached data but the documentation for liquid caching behavior dangers has always been lacking. And I've never seen a confirmation of a reliable way to cache bust[1] from the clientside, so probably has nothing to do with April 1 in totality.. ( had to check this posts date to make sure it itself wasn't a meta April-1st thing 😉 ) .

Root cause?

Side effect of: unofficial workarounds are always unsupported but it's high effort to even know when something supported breaks or a if first class feature will ever exist. Platform behaviors can change with no changelog entry, little or no warning. And there is no first party test suite that can be easily run to identify breaking changes that get no changelog entry. So merchant complaint is still the default platform problem notification system in the ecosystem.

Cache > Developer Experience

Even if stored after the first call it still seems like it would still overlap on the random numbers problem, and random numbers are bad for caching, and bad caching is bad for scale. A reductive take but that's how I view whether or not a platform like shopify would ever add such a feature for it's theme DX.

Then there's also when do these unique ids change, should it when a template is edited, when the line position changes..etc Is there a situation in which the unique_id NOT properly changing causes an issue. I don't know about you but the cache has cost me countless hours because it's giving me the wrong info after I've purposefully made changes to ID's.

Would/should a unique_id generation at all under the developers control or is it yet another black box like the cache developers will end up stubbing toes on again and again.

An aside because something is nagging me that it's somehow related.. Many shopify merchants on the community forums get given very specific CSS that has the liquid generated ID of of a section. But this is incredibly fragile if a merchant ever deletes&remakes a section. Though the new section css feature should help with this fragility as time goes on

Common collision scenarios and fixes

To do that, one typical approach in Liquid is to generate an ID like this and hoping to make it unique: {% capture id %}input-{{ section.id }}-{{ product.id }}{% endcapture %} However, this uniqueness can be hard to achieve under some context. For instance, a merchant may be using a featured collections section featuring two collections, with the two collection having the same product. In such cases, we would end up with the same ID.

In this case, if a single section instance, can't the collection.id's be added added for better context. If two sections, the ID should unique for those two sections instances because each gets a unique section.id. So the prefix is unique at least so doesn't matter if the rest of string repeats a pattern. I think in this case the problem is the object hierarchy is not being leverage to support itself: {% capture id %}input-{{ section.id }}-{{collection.id}}-{{ product.id }}{% endcapture %}

_reaaaly bad spitball and worse names: context_id or object_path_id. So {{ product.id | context_id }} spits out the ids of the current objects context/hierarchy. which is some mix of store.id ➕ theme.id ➕ templatetype.id ➕ settingobject.id(if any) ➕ object.id-loop.index(if any) ➕ {%increment%}. {{ product.id | object_pathid}} would give things a bit more locally collection.id+product.id 🤷

Dawn uses the following pattern: Filter-{{ filter.param_name | escape }}-{{ forloop.index }}

In our theme for instance, we have a more complicated design where the facets are outputted twice: one in a sidebar, and once in a drawer. We therefore have isolated the code inside a "facets.liquid" snippet that gets re-used. So here, the Dawn approach for instance would result in having duplicate ID, which would cause problem in the case of checkboxes, for instance.

Isn't this case just a context namespacing problem. Solved by adding a context to the modularized code. So for example in facets.liquid the output for the drawer is Filter-color-0-drawer and sidebar is Filter-color-0-sidebar Or use the increment tag https://shopify.dev/docs/api/liquid/tags/increment

I see this dupe problem alot in menu/sidebars and also way too many times for things like quick-views, product-recommendations, modals, etc where the current pages product is present in it's own recommendations,upsells,etc. So the product form or PDP section gets dupe IDs because the context isn't being namespaced into the ID's being rendered. Sometimes hilariously it's using the section rendering api to pull in a big chunk so there are multiple IDs that are no longer unique.

Experimental cache busting or client side id generation?

[1] Hypothetically if a store reaaally needs to figure this I've considered two methods I've just never had need to explore. A) a client-side javascript generated timestamp pushed into a cart attribute and that cart attribute is used to make ids past the first page load. B) Backend app kludge maybe with shopify-flow to update a metafield , or use the http action, to keep it dynamic. Though that doesn't address the problem at the theme level and would have setup overhead every theme .. silly .

bakura10 commented 1 year ago

Hi @PaulNewton ,

For information, Shopify has reverted the change on "date" so it is working again. However, I still feel we should have a clean, long-term solution for that.

Regarding your comments,, using {% increment %} for instance does not really work as the increment is contextual to a given file. Of course, you can build a complex strategy for generating ID by concatenatin stuff, but imagine the following use case:

It would look like this:

collection.liquid

{% render 'facets' %} // Render in one context
....
{% render 'facets' %} // Render the facets but in a different context, the drawer for instance

Then, the facets just render itself:

{% for filter in collection.filters %}
  {% for filter_value in filter.values %}
     {% render 'checkbox', name: filter_value.param_name %}
  {% endfor %}
{% endfor %}

And finally the checkbox snippet, that needs to generate a unique ID.

As you can see, of course we can pass a "context_id" or something like this, but you can easily see from this example that you have to generate the context_id in the higher file, and pass it down file by file. Ultimately, the checkbox snippet will receive a strange "context_id" attribute that somehow tie it to its parent, while it should not (each component should be independant).

So here, the checkbox should be the one responsible to generate a unique ID to connect the checkbox and the label that this snippet manages, and not with an external dependency.

This is where a unique_id tag would help, by completely decoupling this.

PaulNewton commented 1 year ago

Increments should be contextual within all snippets in a file, in a parent template it just needs to be called again. But then to your point that is getting as clunky but more confusing than just using a forloop isn't it. e.g. {% for index in (1..2) -%}{{ index }}

As you can see, of course we can pass a "context_id" or something like this, but you can easily see from this example that you have to generate the context_id in the higher file, and pass it down file by file.

With {% render %} being scoped message passing is a behavior we have to live with and the fragility that creates(parameter configuration "coupling"). The alternative is to generate the context within the snippet itself or yet another separate snippet. Which smells of being similar to the painful problem of trying to try to do object type checking if your not lucky enough to be working on link.object.

Ultimately, the checkbox snippet will receive a strange "context_id" attribute that somehow tie it to its parent, while it should not (each component should be independent).

An example of this tying problem is consider the case of moving which template a snippet is used in, from a collection to an article. Then you have to ensure the snippet parameter context_id is updated properly(i.e. go from context_id:collection.id to context_id:article.id). Otherwise things can silently break because the context is now empty, or is an ambiguous reference to a collect object in that article template. Since there is also no generic object.id object for that problem

For a template lang I just consider snippet params configuration I don't really consider it coupling for what's just a gibberish string used for uniqueness and any refactor just requires parameter configuration. Mainly I view it that way since it's not like we can easily render snippets outside of templates anyway so they are coupled to their parent template by nature.

But one less thing to configure or uncouple in fixing themes would be nice.

bakura10 commented 1 year ago

I run again today into a use case that would have benefitted a lot from such a filter. We have a section "Shop the look" in our theme where the product has to be duplicated twice (one for mobile and one for desktop) due to completely different layout. Because those products are part of the same block we cannot use the block.id as a differentiator.

The product quick buy include itself severla snippets recursively (product includes quick-buy.liquid which includes product-info.liquid which includes variant-selector.liquid which include "dropdown.liquid"

Due to the lack of unique_id filter, the only way is to pass down through the whole hierarchy a "context" or "id_prefix" or whatever. This makes the code way more fragile. Having a {% unique_id %} filter that we could use at the deepest snippet would help a lot to clean this...

bakura10 commented 1 year ago

Hi,

Another issue that arises today due to those ID generations, that shows the importance of this filter. For generating unique ID for radio buttons, my code has evolved into this monster of concatenating strings:

{%- capture id -%}radio-{{ id_prefix }}-{{ section.id }}-{{ block.id }}-{{ product.id }}--{{ form }}-{{ name | handle }}-{{ value | handle }}{%- endcapture -%}

beside the fact that this couple this component to a lot of other Liquid objects and force the consumer of the snippet to ensure they pass ton of parameters, I believed it was good enough... I was wrong!

We happened to have a customer who had two option values for their products, named "Fuel-5" and "Fuel-5+". The issue is that when handleized, they both return as "fuel-5". Boom, duplicate ID, selectors not working. So now, our id generator has become even more stupid:

{%- capture id -%}radio-{{ id_prefix }}-{{ section.id }}-{{ block.id }}-{{ product.id }}--{{ form }}-{{ name | handle }}-{{ value | replace: '+', '-plus' | handle }}{%- endcapture -%}

Everything would be so much easier with a {% capture id %}{% unique_id %}{% endcapture %}...

PaulNewton commented 1 year ago

{% capture id %}{% unique_id %}{% endcapture %}

Would be neat, though a dangerous catch with any unique_id in a theme system could be an inability to backtrack it to it's source when debugging.

This is why for an ID dawn would pass something like a singular product_form_id as the main context into snippets, WHEN the inputs are not contained with the principal <form> element. If in a containing form there should already be enough context for any javascript or CSS.

{%- capture id -%}radio-{{ id_prefix }}-{{ section.id }}-{{ block.id }}-{{ product.id }}--{{ form }}-{{ name | handle }}-{{ value | handle }}{%- endcapture -%}

What's going on here, is this being used to try and individually style every radio button to be unique?

Unless your doing javascript stuff that needs unique #ID's selectors from some reason, it seems more like over/under engineering of an unresolved architecture problem and not a liquid or dawn problem. The section id, block.id, even the product id generally should all be hoisted to higher parent elements such as the containing form.

If this is for a set of radio buttons most of that information is completely redundant for an ID selector. {%- capture id -%}radio-{{ id_prefix }}--{{ name | handle }}-{{ value | handle }}{%- endcapture -%}

Fuel-5" and "Fuel-5+". The issue is that when handleized, they both return as "fuel-5".

If options use the position property for more uniqueness, or any current forloops index. https://shopify.dev/docs/api/liquid/objects/product_option#product_option-position

Would want to double check the html spec for id & class selectors allowed characters but also see: url_encode or url_escape ? {%- capture id -%}radio-{{ id_prefix }}--{{ name | handle }}-{{ value | url_encode }}{%- endcapture -%}

bakura10 commented 1 year ago

Yes, we need the ID to be unique. There can be a lot of complex situation that are more complex that "one section" = "one product". Imagine a complex section with a list of blocks, each blocks with a list of products, each product with their own selector. You have to guarantee a unicity of the ID to avoid everything to fail. So we have to come with those complex concatenations, but once again, the main issue is that it create ton of issues as we need to pass down a lot of context specific information back to a super generic snippet.

The section id, block.id, even the product id generally should all be hoisted to higher parent elements such as the containing form.

I have to disagree with that. In component based system (which we are using) it is the responsibility of the component to manage its own internals. A input/radio component should NOT have any relationship with their parent to generate an ID. It is the responsibility of the radio component to generate a unique ID and link it to its label.

If options use the position property for more uniqueness, or any current forloops index.

Yeah, so you're suggesting adding even more coupling between a generic component and its parent. A radio button should only have a minimal set of parameters, the name, the value and the label. That's all.

Here is the ideal case:

{% capture id %}{% unique_id %}{% endcapture %}
<input type="radio" name="{{ name }}" value="{{ value }}" id="{{ id }}">
<label for="{{ id }}">{{ label }}</label>

But to guarantee this unicity, we now have to couple this component to ton of things this component should not be aware of:

{% comment %}
Generate an input. Make sure to pass the following parameter to ensure it works:

- section
- block
- product
- context
- forloop
{% endcomment %}

{% capture id %}radio-{{ section.id }}-{{ block.id }}-{{ product.id }}-{{ forloop.index }}-{{ context }}-{{ name }}-{{ value }}{% endcapture %}
<input type="radio" name="{{ name }}" value="{{ value }}" id="{{ id }}">
<label for="{{ id }}">{{ label }}</label>

So now, whenever you want to render an input, instead of just:

{% render 'radio', name: 'foo', label: 'My radio', value: 'bar' %}

You have to remember to pass all the parameters. Forget one? You're in trouble...

{% render 'radio', name: 'foo', label: 'My radio', value: 'bar', section: section, block: block, forloop: forloop, product: product %}

Do you see the non-sense ? Why a radio snippet should be aware of the product, the forloop or a block?

Want to re-use this snippet for creating blog post input? Oh, now you will need to add a dependency to article as well.

Of course you could move this ID generation to the parent, and pass down the ID, but this is an even worse solution, because now you will need to remember your complex ID generation logic, and duplicate it over and over and over instead of just giving this responsibility to the component.

I mean, if many template languages have a unique id generator for this exact purpose, it is for a reason!

React: https://react.dev/reference/react/useId EmberJS: https://rfcs.emberjs.com/id/0659-unique-id-helper/

For JS based approach you can also simply use the crypto.randomUUID() to generate a UUID that can be used for that.

But in Liquid we can't rely on JS to generate ID (otherwise the theme won't work when JS is disabled).

PaulNewton commented 1 year ago

A component managing it's own internals does not mean stuffing it like a turkey.

A input/radio component should NOT have any relationship with their parent to generate an ID

But that's what your doing and may not have to be doing.

Yeah, so you're suggesting adding even more coupling between a generic component a

Yes, when a radio option is representing a product option the productOption position is part of the components identity in liquid this isn't react.js. It's not some thing alone out in the ether.

You have to remember to pass all the parameters. Forget one? You're in trouble... {% render 'radio', name: 'foo', label: 'My radio', value: 'bar', section: section, block: block, forloop: forloop, product: product %}

No, just pass a single var , that's a badly contrived example that shows a design problem not a system problem

Dawn has already established this as a pattern and practice with product_form_id. https://github.com/Shopify/dawn/blob/main/sections/main-product.liquid#L76

Construct your poisons

{% capture render_context %}{{ section.id }}-{{ block.id }}{% endcapture %}
{% capture product_context %{{ product.id }}-{{variant.id}}-{{product_option.position}}{% endcapture %}
{% capture processing_context %}{{ forloop.parent.index}}-{{ forloop.index }}{% endcapture %}

{% capture id_context %}{{ render_context }}--{{ product_context  }}--{{ processing_context }}{% endcapture %}

{% render 'thing', id_prefix: render_context %}
or 
{% render 'thing', id_prefix: id_context %}
etc
or
<input type="radio" name="{{ name }}" value="{{ value }}" id="{{id_prefix}}--{{ id }}">

Trying to avoid dependencies injection or inversion has a limit when working in a merchant facing templating language.

Using react.js and ember.js as reasons for {% unique_id %} why isn't a valid point. Shops needing that level of complexity for comparison should be using the storefront-apis.

Shopify liquid is much simpler in scope, even if {% unique_id %} may be a needed thing it needs rock solid examples that are more than flawed theme design and can't be poked through easily.

bakura10 commented 1 year ago

I strongly disagree with that. Dawn is far from optimal from a code quality and I would not take Dawn as the reference for doing things. Using component based approach works like a charm for Liquid. The way Dawn creates all the input fields is a mess to manage once you want to do some global changes. Dawn is repeating over and over the input, label, the class... Dawn also have a LOT of code duplication that we managed to abstract away using snippet.

Also, Dawn is not as complex in terms of features. When you start adding quick view and things like that, Dawn approach starts to collapse completely.

Just to show you an example of how abstracting that as component is way better. Look at Dawn code here: https://github.com/Shopify/dawn/blob/main/sections/main-addresses.liquid#L75

Here is basically our code:

<div class="fieldset-row">
          {%- render 'input', name: 'address[first_name]', label: first_name_label, value: form.first_name, autocomplete: 'given-name', id_prefix: address.id -%}
          {%- render 'input', name: 'address[last_name]', label: last_name_label, value: form.last_name, autocomplete: 'family-name', id_prefix: address.id -%}
        </div>

        {%- render 'input', name: 'address[company]', label: company_label, value: form.company, autocomplete: 'organization', id_prefix: address.id -%}
        {%- render 'input', type: 'tel', name: 'address[phone]', label: phone_label, value: form.phone, autocomplete: 'tel', id_prefix: address.id -%}
        {%- render 'input', name: 'address[address1]', label: address1_label, value: form.address1, autocomplete: 'address-line1', id_prefix: address.id -%}
        {%- render 'input', name: 'address[address2]', label: address2_label, value: form.address2, autocomplete: 'address-line2', id_prefix: address.id -%}

        <div class="fieldset-row">
          {%- render 'input', name: 'address[city]', label: city_label, value: form.city, autocomplete: 'address-level2', id_prefix: address.id -%}
          {%- render 'input', name: 'address[zip]', label: zip_label, value: form.zip, autocomplete: 'postal-code', autocapitalize: 'characters', id_prefix: address.id -%}
        </div>

Much simper. Now, we have several advantages:

But even here, as you can see, to ensure this unicity we had to create a dependency between the input and the parent (with the address.id to ensure unicity). This should not be needed. It is the responsibility of the input to create the relationship of between the input and the label.

A component managing it's own internals does not mean stuffing it like a turkey.

But, in this case, this is the only way to solve this problem. So yes, we have to fill it like a turkey. There are no other ways.

Using react.js and ember.js as reasons for {% unique_id %} why isn't a valid point. Shops needing that level of complexity for comparison should be using the storefront-apis.

No, this is incorrect. We are doing themes for the theme store, and we HAVE this complexity, because this is what merchants are expecting from paid themes. Once again, Dawn is a really bad example to look at. The code is really, really far from good, and Dawn is too simple in its scope to serve as a base of what people are doing with Liquid.

Shopify liquid is much simpler in scope, even if {% unique_id %} may be a needed thing it needs rock solid examples that are more than flawed theme design and can't be poked through easily.

Mmhhh... what do you mean? Do not need to be rock-solid example, the primary use case is generating ID. Can be as simple as:

`unique_id` is a Liquid filter to generate a unique ID that can be passed to accessibility attributes, or to create reference between elements (for instance, an input and its associated label).
tobi commented 1 year ago

thank you for talking us through this. We will cook something up.

PaulNewton commented 1 year ago

Dawn is far from optimal from a code quality

A primary goal of liquid is for merchants ability to understand and make even minor edits to their themes. Dawn is an approachable example of what a primary base interacts with for OS2.0 themes. It's not mean to be the dizzying height of cleverness because wearesoveryverysmart. It's not an example of how unclever it is simply because it's not treating "no duplication" as a law. It's a DX excuse to use this view to turn themes even more opaque against merchants and stakeholders.

Simply because I as a shopify-theme developer want some niche feature to support my personal architecture isn't a great approach to feature bloat in the template language itself. Liquids complexity has exploded in recent years, waaay more complexity in shopify-liquid-themes, and this can just end up being more development creep hiding behind DX.

One complexity was blackboxing snippets with {% render %}. Bringing us here because the core issue of there being no globals, no reference states in a simple templating language. Contortions 🤸‍♂️ and shoehorns 👞📯.

we can have a skeleton .. abstracted as snippet. ..change once .. hundreds of input all over the themes adapt.

I get the point and agree in ways as I primarily customize/fix themes and constantly having to edit price displays in 3~7 files is obnoxious 🤢. But this is what a build process is for when managing a theme long-term either as a theme-maker or store developer.

Bad DX symptoms , the examples read as reaching a complexity point with bad tooling, or lack of build process. Then trying to backfill build process concepts into liquid itself from unwillingness to build that tooling and drastically improve the DX at the root.; or for non theme-makers migrate a store to the more complex systems that would allow the wanted features It's one thing to snippettize a form itself, when understandable such as extracting it out of a bigger file. Completely another thing, in a general theme, to turn every input in a form into a {% render %}. This is how do-everything-inflexible monolith snippets get created and perf testing becomes a recurring necessity which hurts merchants who can not have such a process. Architectural overkill and design inflexibility; there's a reason so many stores start out fine then start to look blocky after a while.

Aside for non-theme-makers: It's not an easy list to build of bulletproof reasons to use the storefront api but if a merchants theme has reached such a complex level that developer expenses has to super optimize every manually hand edited liquid theme file that's a pretty good reason to take a serious look.

bakura10 commented 1 year ago

@tobi thanks a lot for considering adding this to Liquid. I’m very grateful for this.

@PaulNewton keep in mind that it is not also just for MY personal architecture. We have countless of agencies using our themes as a base (instead of Dawn). Part of my architecture is also part of a big effort I initiated internally years ago to make our themes more dev-friendly. And based on the output I receive from many agencies, I can confirm you that those architecture decisions are for the good.

PaulNewton commented 1 year ago

It is your architecture. Regardless of how many users we end up being the primary theme-dev for. we me, you , maestroo, don't represent every single theme-maker, or the end users. The ecosystem matters.

There are limits to consolidation under a regime of "no duplication". Look at maestroos own focal theme having separate <quantity-selector> and <line-item-quantity>. Doing something as basic as changing the themes triputs to have step behavior , because it uses a "text" input instead of a number input, requires editing many files including separate javascript behaviors. Because despite the elements UI aesthetics meant to be the "same", the functional approach taken for both HTML components are wildly different for some legitimate reasons requiring separate html and separate js. Turning such a thing into a single-source of truth omni-component could be moot and self-defeating.

It's a smell to use "If we developers have X in liquid then merchants benefit indirectly because then we can add complexity". Good read on this type of culture problem https://grugbrain.dev/ making the rounds.

May as well push for being able to create our own global-objects in liquid as at least that would be a categorical solution solving this and many many many other theme development limitations with liquid.

bakura10 commented 1 year ago

I'm stopping arguing here, I think it will go nowhere. Thanks for your opinions and ideas so far, and I am happy to hear Tobi is considering adding a feature that will help improving accessibility, code quality for us, other theme dev who expressed the same need, and the whole ecosystem.

sottovocedsp commented 2 weeks ago

If you need a unique ID in DAWN somewhere:

assets/unique-id-generator.js

// Function to generate and assign unique IDs to elements matching a selector
function generateUniqueIDs(selector) {
  const elements = document.querySelectorAll(selector);
  elements.forEach((element, index) => {
    console.log("Element before:", element, "ID:", element.id, "Value:", element.value);

    // Declare the unique ID we will generate
    let uniqueID;

    // Use crypto.randomUUID if available 
    if (typeof crypto !== 'undefined' && crypto.randomUUID) {
      uniqueID = crypto.randomUUID();
    } 
    // Use crypto.getRandomValues as a fallback
    else if (typeof crypto !== 'undefined' && crypto.getRandomValues) {
      // Fallback using crypto.getRandomValues for UUID generation
      uniqueID = 'xxxxxxx-xxxx-4xxx-yxxx-xxxxxxxxxxxx'.replace(/[xy]/g, function(c) {
        const r = (crypto.getRandomValues(new Uint8Array(1))[0] % 16) | 0;
        return (c === 'x' ? r : (r & 0x3) | 0x8).toString(16);
      });
    } 
    // Just generate a random number on old browsers with the date
    else {
      // Final fallback using timestamp and random number
      uniqueID = `${Date.now()}-${Math.floor(Math.random() * 1000)}`;
    }

    element.id = uniqueID;

    if (element.tagName.toLowerCase() === 'input') {
      element.value = uniqueID;
    }

    console.log("Element after:", element, "ID:", element.id, "Value:", element.value);
  });
}

// Automatically assign unique IDs on DOM load
document.addEventListener('DOMContentLoaded', function () {
  console.log("DOM fully loaded. Assigning unique IDs...");
  generateUniqueIDs('.needs-unique-id');
});

// Add click event listener to all elements with .click-causes-regeneration to regenerate unique IDs
document.querySelectorAll('.click-causes-regeneration').forEach(element => {
  element.addEventListener('click', function () {
    console.log("Regeneration triggered by click on:", element);
    generateUniqueIDs('.needs-unique-id');
  });
});

// Export function for accessibility elsewhere
window.generateUniqueIDs = generateUniqueIDs;

layout/theme.liquid

<script src="{{ 'unique-id-generator.js' | asset_url }}" defer="defer"></script>

To use it, for example, to give every line item a unique ID (UUID)

snippets/buy-buttons.liquid

{%- form 'product',
        product,
        id: product_form_id,
        ...
      -%}
        <input
          type="hidden"
          name="id"
          ...
        >

        {% comment %} ADD UNIQUE ID TO SEPARATE LINE ITEMS {% endcomment %}
        <input type="hidden" name="properties[lineitem_unique_id]" value="" class="needs-unique-id">

Console logs:

DOM fully loaded. Assigning unique IDs...
unique-id-generator.js:5 Element before: input.needs-unique-id ID:  Value: 
unique-id-generator.js:22 Element after: input#1c6e1063-137f-4fee-9c0d-75a1cb6121f5.needs-unique-id ID: 1c6e1063-137f-4fee-9c0d-75a1cb6121f5 Value: 1c6e1063-137f-4fee-9c0d-75a1cb6121f5

As you can see I also added a regenerator, which is useful when someone clicks add to cart so the next add to cart causes the line item to have a different ID.

snippets/buy-buttons.liquid

<button
            id="ProductSubmitButton-{{ section_id }}"
            type="submit"
            name="add"
            class="click-causes-regeneration .....
PaulNewton commented 2 weeks ago

To use it, for example, to give every line item a unique ID (UUID)

Use line_item.key property https://shopify.dev/docs/api/liquid/objects/line_item#line_item-key

What's the case for having to generate random keys for something submited to the ajax api.

For js and css If ID's are being randomly generated on the frontend now there's an extra hoop to get ID based behaviors/styles to work; when an attribute selector using prefix of the ID isn't usable that is.