feat: Add support of inner expressions enclosed by parentheses - #863
Conversation
| return -1 | ||
| } | ||
|
|
||
| private looksLikeRange (): boolean { |
There was a problem hiding this comment.
The parsing is no longer near O(N) complexity with findMatchingParen and findMatchingParen introduced.
|
Thanks for your effort on this long waited feature. It's expected to break something thus I put it in the v11 version planning. But if it's totally compatible, l'd like to merge it in current major version with a feature bump. As @jg-rp mentioned Shopify/liquid is also working on this, I think we can wait until then to keep aligned. |
|
Many thanks for the review, I'll address all the comments 👍 |
|
You could replace readGroupOrRange (): RangeToken | GroupedExpressionToken | undefined {
this.skipBlank()
const begin = this.p
if (this.peek() !== '(') return
++this.p
const lhs = this.readValueOrThrow()
this.skipBlank()
if (this.peek() === '.' && this.peek(1) === '.') {
this.p += 2
const rhs = this.readValueOrThrow()
this.skipBlank()
this.assert(this.read() === ')', 'invalid range syntax')
return new RangeToken(this.input, begin, this.p, lhs, rhs, this.file)
}
if (this.groupedExpressions) {
const expression = new Expression(toIterableIterator(lhs))
this.skipBlank()
const filters = this.readFilters()
this.skipBlank()
this.assert(this.read() === ')', 'unbalanced parentheses')
this.skipBlank()
return new GroupedExpressionToken(expression, filters, this.input, begin, this.p, this.file)
}
throw this.error('invalid range syntax')
}Along with some changes to But it still all feels like a bit of a work around. As soon as filters become context free expressions rather than a special post processing step, tokenizing/parsing really needs access to the current In an ideal world I'd like to drop the concept of That being said - if you're keen to get this feature released without a wider refactoring - the approach with (Developers with custom tags might still need to make changes when enabling the |
Made-with: Cursor
…l-and-Loop-Tags' of https://github.com/skynetigor/liquidjs into 833_Support-Value-Expressions-as-Operands-in-Conditional-and-Loop-Tags Made-with: Cursor # Conflicts: # src/parser/tokenizer.ts
…in-Conditional-and-Loop-Tags
…l-and-Loop-Tags' of https://github.com/skynetigor/liquidjs into 833_Support-Value-Expressions-as-Operands-in-Conditional-and-Loop-Tags
…cenarios for enabled and disabled grouped expressions in case, for, if, unless tags, ensuring proper handling of expressions and error throwing for invalid syntax.
|
Hi guys! @harttle I think it's safe change for the current major version. The flag That would be a nice feature improving our customers experience 🤞 |
| yield * extractValueTokenVariables(token.lhs) | ||
| yield * extractValueTokenVariables(token.rhs) | ||
| } else if (isGroupedExpressionToken(token)) { | ||
| for (const t of token.initial.postfix) { |
There was a problem hiding this comment.
create a extractGroupedExpressionTokenVariables
The readGroupedExpression() test suite was duplicated twice in the spec file. Removed the duplicate block to avoid redundant test execution.
Extract inline grouped expression variable extraction logic into a dedicated function for consistency with other extractors (extractFilteredValueVariables, extractPropertyAccessVariable). This addresses PR harttle#863 comment 7 - improves code organization and maintainability.
collection: ValueToken | GroupedExpressionToken Addresses PR harttle#863 comment 5.
…lters Addresses PR review comments 4, 6, 8, 9 - moves grouped expression evaluation from parse-time resolution to render-time lazy evaluation following the generator-based async/sync duality pattern used throughout liquidjs. Key changes: - Replace resolvedValue (Value instance) with resolvedFilters (Filter[]) - Rename resolveGroupedExpressions() to resolveGroupedExpressionFilters() - Move evaluation logic to evalGroupedExpressionToken() at render time - Build Filter instances at parse time (carry liquid reference for render) - Evaluate expression and apply filters lazily via generators - Add support for tablerow tag with grouped expressions - Remove duplicate getFilter() method in Value class Maintains proper layering (tokens → render → templates) and consistency with Value.value() pattern. Filter resolution still happens at parse time since it requires liquid.filters access, but actual evaluation is deferred to render time. Tags that store raw ValueToken (for, case when-values, tablerow) still need explicit resolveGroupedExpressionFilters() calls. Tags that wrap with new Value() get automatic recursive resolution via Value constructor.
|
Hi @harttle, Omri from @skynetigor's team - thanks for the detailed review! I'd like to confirm the approach before implementing: Proposed Changes1. Update token structure (fixes layering violation): export class GroupedExpressionToken extends Token {
public resolvedFilters?: Filter[] // Change from resolvedValue to resolvedFilters
}Tokens will store 2. Evaluate at render time (fixes eager resolution): function * evalGroupedExpressionToken(token, ctx, lenient) {
assert(token.resolvedFilters, 'grouped expression filters not resolved')
let val = yield token.initial.evaluate(ctx, lenient) // Lazy evaluation
for (const filter of token.resolvedFilters!) {
val = yield filter.render(val, ctx) // Apply filters lazily
}
return val
}This follows the generator pattern used throughout liquidjs for async/sync duality. 3. Build Filter instances during Value construction: export function resolveGroupedExpressionFilters(token: Token, liquid: Liquid) {
if (isGroupedExpressionToken(token)) {
for (const t of token.initial.postfix) {
resolveGroupedExpressionFilters(t, liquid) // Recursive
}
token.resolvedFilters = token.filters.map(filterToken =>
new Filter(filterToken, getFilter(liquid, filterToken.name), liquid)
)
}
// ... handle Range/PropertyAccess recursively
}Called from 4. Resolution calls in tag constructors (regarding this): Tags that store raw
Tags that wrap values with Why This Works
Attaching a draft PR with the proposed implementation - https://github.com/harttle/liquid |
|
I was imagining that we could change the type TokenizerCtor = new (
input: string,
liquid: Liquid, // Get options directly from the Liquid object.
file?: string,
range?: [number, number]
) => Tokenizer;And add type ValueToken =
RangeToken
| LiteralToken
| QuotedToken
| PropertyAccessToken
| NumberToken
| FilteredValueToken // This is new.We'd then need to change (Notice that the existing definition of To continue to support runtime tokenization in some filters, we'd also need to change This approach moves responsibility for |
|
Can we reuse I admit |
|
@rosomri thank you for the effort. Comments to your proposals:
3.,4. Same, let's see whether we can reuse. |
Sounds good 👍 . We could even add an export type ValueToken =
RangeToken
| LiteralToken
| QuotedToken
| PropertyAccessToken
| NumberToken
export type ExpressionToken = ValueToken | FilteredValueToken;There are quite a few places where |
|
@harttle and @jg-rp, thank you for the detailed architectural feedback again! I've implemented the suggested improvements in a draft PR to demonstrate the approach: #878 Changes in #8781. Reused
2. Fixed layering violation (as noted here)
3. Lazy filter resolution at render time (as noted here)
Results✅ All 1537 tests pass QuestionThe implementation adds I'd appreciate your feedback on #878 to ensure the architectural direction is correct before finalizing this feature. Thanks! |
👍 Render time filter resolution is a valid approach. This is what Shopify/liquid does. I've drafted #879 as a proof of concept for maintaining parse-time filter resolution. I'm not yet convinced it is the best approach, just an option to consider. |
|
It’ll be good to avoid passing liquid instance to lower layers, if we can. |
Allow arbitrary expressions inside parentheses (e.g. ((a | upcase) > 3) and (1 < 3)) when groupedExpressions is enabled, reusing readFilteredValue for the general case while keeping range and filter-chain fast paths.
Added `readExpressionTokensFromHere` and `readGroupedExpressionTokens` methods to improve the handling of expression tokens. This refactor simplifies the token reading process and maintains compatibility with existing grouped expressions, ensuring proper evaluation and filtering.
…in-Conditional-and-Loop-Tags
|
Hi @harttle I did the changes, please give it another shot 🙏 In order to close all gaps, it seems we need major architectural change in LiquidJS to support this nice feature. |
|
@harttle did you have a chance to take another look? |
|
Thank you @skynetigor . The architecture is already good enough. I made some minor comments on some impl details. |
…in-Conditional-and-Loop-Tags
|
Hey guys! |
|
@all-contributors please add @skynetigor for code |
|
@skynetigor already contributed before to code |
|
🎉 This PR is included in version 10.28.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
…sions (#282257) ## Summary - Upgrades `liquidjs` from `10.27.2` to `10.28.0`. - Enables the `groupedExpressions` option in the shared workflow Liquid engine, allowing parenthesised sub-expressions and filter chains as operands in `if`/`unless`/`for`/`case` conditions. ## Details ### liquidjs v10.28.0 [harttle/liquidjs#863](harttle/liquidjs#863) adds native support for parenthesised expressions as operands in tags, gated behind a new `groupedExpressions` option (default `false`). This is a **non-standard, opt-in extension** to Liquid syntax — Ruby Liquid does not support this. All existing templates continue to work identically; the change is purely additive (syntax that previously threw a parse error is now valid when the flag is on). With the flag enabled, parentheses that don't contain range syntax (`..`) are parsed as grouped sub-expressions, enabling filter chains and logical grouping directly as operands: ```liquid {% if a and (b or c) %}...{% endif %} {% if (username | size) >= 3 %}...{% endif %} {% if (name | size) > 0 and (email | downcase) contains "@" %}...{% endif %} {% unless (content | strip_html | size) == 0 %}...{% endunless %} {% case (status | downcase) %}{% when "active" %}...{% endcase %} {% for i in (1..(items | size)) %}...{% endfor %} ``` Existing range syntax `(1..5)` is unaffected. ### Engine configuration `groupedExpressions: true` is added to `createWorkflowLiquidEngine` so the feature is available across all contexts that use the shared engine — server-side rendering, the YAML editor's syntax validation, and template autocomplete.
…sions (elastic#282257) ## Summary - Upgrades `liquidjs` from `10.27.2` to `10.28.0`. - Enables the `groupedExpressions` option in the shared workflow Liquid engine, allowing parenthesised sub-expressions and filter chains as operands in `if`/`unless`/`for`/`case` conditions. ## Details ### liquidjs v10.28.0 [harttle/liquidjs#863](harttle/liquidjs#863) adds native support for parenthesised expressions as operands in tags, gated behind a new `groupedExpressions` option (default `false`). This is a **non-standard, opt-in extension** to Liquid syntax — Ruby Liquid does not support this. All existing templates continue to work identically; the change is purely additive (syntax that previously threw a parse error is now valid when the flag is on). With the flag enabled, parentheses that don't contain range syntax (`..`) are parsed as grouped sub-expressions, enabling filter chains and logical grouping directly as operands: ```liquid {% if a and (b or c) %}...{% endif %} {% if (username | size) >= 3 %}...{% endif %} {% if (name | size) > 0 and (email | downcase) contains "@" %}...{% endif %} {% unless (content | strip_html | size) == 0 %}...{% endunless %} {% case (status | downcase) %}{% when "active" %}...{% endcase %} {% for i in (1..(items | size)) %}...{% endfor %} ``` Existing range syntax `(1..5)` is unaffected. ### Engine configuration `groupedExpressions: true` is added to `createWorkflowLiquidEngine` so the feature is available across all contexts that use the shared engine — server-side rendering, the YAML editor's syntax validation, and template autocomplete.
Adds support for parenthesized expressions as operands in tags, gated behind a new
groupedExpressionsoption (defaultfalse). Closes #833.This is a non-standard extension to Liquid syntax — Ruby Liquid does not support this. The feature is opt-in to preserve strict compatibility by default and can be enabled by default in a future major version.
Backward compatibility: Even with
groupedExpressions: true, all existing Liquid templates continue to work identically. The change is purely additive — it only gives meaning to syntax that previously threw a parse error ((expr | filter)in operand position). No valid Liquid template changes behavior. Range syntax(1..5)is unaffected. Templates written for standard Liquid are fully compatible regardless of the flag setting.When enabled, parentheses that don't contain range syntax (
..) are parsed as grouped sub-expressions, allowing filter chains and logical grouping to be used directly as operands:{% if (username | size) >= 3 %}...{% endif %} {% if (a | upcase) == (b | upcase) %}...{% endif %} {% if (name | size) > 0 and (email | downcase) contains "@" %}...{% endif %} {% unless (content | strip_html | size) == 0 %}...{% endunless %} {% case (status | downcase) %}{% when "active" %}...{% endcase %} {% for i in (1..(items | size)) %}...{% endfor %}Existing range syntax
(1..5)is unaffected — a lookahead scan checks for..at depth 1 before deciding which parser to invoke.Approach
looksLikeRange()lookahead inreadValue()routes(to eitherreadRange()or the newreadGroupedExpression()readGroupedExpression()temporarily limits the tokenizer boundary to the matching)and delegates to the existingreadFilteredValue()parserGroupedExpressionTokenstores the parsed expression and filters; aresolveGroupedExpressions()utility recursively resolves them intoValueinstances when theLiquidinstance is availableValue(for,case/when) call the resolver in their constructorsTest plan
if,unless,for,case/whenwithgroupedExpressions: trueevalValueSyncAPI tests for direct expression evaluation