Skip to content
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

Use VariableLookup instead of dot notation in standard array filters #1899

Closed
wants to merge 1 commit into from

Conversation

karreiro
Copy link
Contributor

@karreiro karreiro commented Jan 17, 2025

Following @ianks' suggestion (https://github.com/Shopify/liquid/pull/1869/files#r1919321969), this PR updates standard array filters to use Liquid::VariableLookup instead of the dot notation convention.

I initially considered something similar to the usage of the Liquid::VariableLookup, but I noticed it would open the door to broader use cases like accessing arrays, which might not be ideal as mentioned here (#1869 (comment)).

However, in the interest of consistency, we see value in evaluating this approach, which is the goal of this PR :)

@jg-rp
Copy link
Contributor

jg-rp commented Jan 17, 2025

Parsing filter arguments as variables at render time, with or without VariableLookup.parse, has two potential downsides for me.

The first is one of efficiency. property arguments will be parsed at least once for every item in the input array. In the case of the sort filter, it could be that they will be parsed twice for each comparison (I don't actually know how Ruby's Array.sort works internally).

The second downside is regarding template static analysis. If we were walking a template's parse tree with ParseTreeVisitor, property arguments will always appear as strings and never instances of VariableLookup. Even if we were to implement some special case handling, knowing that some filters accept variables as strings, we'd have no way of knowing if those strings were intended to be a name or a path to a variable.

I understand that you might have already considered these things and decided that the current approach is still the best option. I'm just sharing my thoughts here in the absence of an RFC 😃.

Copy link

@aswamy aswamy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did some tophatting where i added variants array to the list of products in our sandbox, and it looks good 👍

image

@karreiro
Copy link
Contributor Author

Thank you for bringing that context, @jp-rp.

I followed up on these thoughts at Shopify, performed additional risk assessment on the nested filters, and noticed it’s not ideal to introduce nested properties support at this time.

I left some context about this here: #1907

I’m closing this PR in favor of #1906

Thanks again for those thoughts 🙇

@karreiro karreiro closed this Jan 31, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
#gsd:43130 Simplify Liquid DX
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants