-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
[Request] The need for a "unique_id" generation filter in Liquid #2483
Comments
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. Cache > Developer ExperienceEven 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. Then there's also when do these unique ids change, should it when a template is edited, when the line position changes..etc 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.. Common collision scenarios and fixes
In this case, if a single section instance, can't the collection.id's be added added for better context. reaaaly bad spitball and worse names:
Isn't this case just a context namespacing problem. Solved by adding a context to the modularized code. 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. 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 . |
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
Then, the facets just render itself:
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. |
Increments should be contextual within all snippets in a file, in a parent template it just needs to be called again.
With {% render %} being scoped message passing is a behavior we have to live with and the fragility that creates(parameter configuration "coupling").
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 For a template lang I just consider snippet params configuration I don't really consider it coupling for what's just a gibberish But one less thing to configure or uncouple in fixing themes would be nice. |
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 |
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:
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:
Everything would be so much easier with a |
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
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. If this is for a set of radio buttons most of that information is completely redundant for an ID selector.
If options use the position property for more uniqueness, or any current forloops index. Would want to double check the html spec for id & class selectors allowed characters but also see: |
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.
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
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:
But to guarantee this unicity, we now have to couple this component to ton of things this component should not be aware of:
So now, whenever you want to render an input, instead of just:
You have to remember to pass all the parameters. Forget one? You're in trouble...
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 For JS based approach you can also simply use the But in Liquid we can't rely on JS to generate ID (otherwise the theme won't work when JS is disabled). |
A component managing it's own internals does not mean stuffing it like a turkey.
But that's what your doing and may not have to be doing.
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.
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 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. 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. |
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:
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.
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.
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.
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:
|
thank you for talking us through this. We will cook something up. |
A primary goal of liquid is for merchants ability to understand and make even minor edits to their themes. 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. One complexity was blackboxing snippets with {% render %}.
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 🤢. Bad DX symptoms , the examples read as reaching a complexity point with bad tooling, or lack of build process. Aside for non-theme-makers: |
@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. |
It is your architecture. There are limits to consolidation under a regime of "no duplication". 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. |
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. |
If you need a unique ID in DAWN somewhere: assets/unique-id-generator.js
layout/theme.liquid
To use it, for example, to give every line item a unique ID (UUID) snippets/buy-buttons.liquid
Console logs:
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
|
Use line_item.key property 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. |
[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:
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):
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:
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:
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:
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.
The text was updated successfully, but these errors were encountered: