feat: Add support of inner expressions enclosed by parentheses#863
Conversation
src/parser/tokenizer.ts
Outdated
| 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.
src/parser/tokenizer.ts
Outdated
| this.p = begin | ||
| return | ||
| } | ||
| const savedN = this.N |
There was a problem hiding this comment.
Though we know JS is single threaded, we need avoid this hack for simplicity. For example when readFilteredValue throw an error, we need this.N be correct in error handler.
src/parser/tokenizer.ts
Outdated
| if (this.groupedExpressions && !this.looksLikeRange()) { | ||
| variable = this.readGroupedExpression() | ||
| } else { | ||
| variable = this.readRange() |
There was a problem hiding this comment.
Let's avoid lookslike, the language should be deterministic and non ambiguous.
|
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 🤞 |
|
|
||
| function * evalGroupedExpressionToken (token: GroupedExpressionToken, ctx: Context, lenient: boolean): IterableIterator<unknown> { | ||
| assert(token.resolvedValue, 'grouped expression not resolved') | ||
| return yield token.resolvedValue!.value(ctx, lenient) |
There was a problem hiding this comment.
Try to create something like evalGroupedExpressionToken(), instead of pre populate resolvedValue, which can be scattered in multiple places.
|
|
||
| export default class extends Tag { | ||
| variable: string | ||
| collection: ValueToken |
There was a problem hiding this comment.
| collection: ValueToken | GroupedExpressionToken |
|
|
||
| this.variable = variable.content | ||
| this.collection = collection | ||
| resolveGroupedExpressions(this.collection, liquid) |
| 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
| import type { Liquid } from '../liquid' | ||
| import type { Context } from '../context' | ||
|
|
||
| export function resolveGroupedExpressions (token: Token, liquid: Liquid): void { |
There was a problem hiding this comment.
I guess this should be moved to evalGroupedExpressionToken(), and if some structures can be created in parse time, need to be done in GroupedExpressionToken#constructor and consumed during evalGroupedExpressionToken()
| import { Expression } from '../render' | ||
|
|
||
| export class GroupedExpressionToken extends Token { | ||
| public resolvedValue?: { value (ctx: any, lenient?: boolean): Generator<unknown, unknown, unknown> } |
There was a problem hiding this comment.
templates (include Value) depends on tokens, tokens don't depends on templates
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.
|
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. |
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