refactor: architectural improvements for grouped expressions (demo for PR #863)#876
Draft
rosomri wants to merge 14 commits intoharttle:masterfrom
Draft
refactor: architectural improvements for grouped expressions (demo for PR #863)#876rosomri wants to merge 14 commits intoharttle:masterfrom
rosomri wants to merge 14 commits intoharttle:masterfrom
Conversation
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.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This is a demonstration PR showing proposed architectural improvements for PR #863 in response to review comments 4, 6, 8, 9.
Changes
Architectural refactoring (comments 4, 8, 9):
resolvedValue: ValuewithresolvedFilters: Filter[]inGroupedExpressionTokenresolveGroupedExpressions()toresolveGroupedExpressionFilters()evalGroupedExpressionToken()Code organization (comment 7):
extractGroupedExpressionTokenVariables()helper functionType safety (comment 5):
ValueToken | GroupedExpressionTokenunion types for for/tablerow collection propertiesBug fix:
tablerowtag (was missing)Code quality:
getFilter()method in Value classRegarding Comment 6
Tags that store raw
ValueTokenand evaluate directly withevalToken()still need explicitresolveGroupedExpressionFilters()calls:for.ts- collection propertycase.ts- when valuestablerow.ts- collection propertyTags that wrap with
new Value()get automatic recursive resolution via Value constructor.Test Results
All 1537 tests pass ✅
Note: This PR is for demonstration purposes to discuss the architectural approach. The actual changes should be applied to PR #863 by @skynetigor.
cc @harttle @skynetigor - feedback welcome!