Skip to content

feat(template): rewrite Liquid engine with Notifuse/liquidgo#4

Merged
zeroedin merged 2 commits into
mainfrom
feature/liquid-engine
Apr 12, 2026
Merged

feat(template): rewrite Liquid engine with Notifuse/liquidgo#4
zeroedin merged 2 commits into
mainfrom
feature/liquid-engine

Conversation

@zeroedin

Copy link
Copy Markdown
Owner

Summary

  • Replace osteele/liquid with Notifuse/liquidgo for full Shopify Liquid compatibility
  • Gains native support for {% render %} with isolated scope, forloop.parentloop, {%- -%} whitespace control, and {% tablerow %}
  • Remove osteele/liquid dependency entirely

Test results

Test group Before After
Basic rendering (variables, if, for, assign) 5/5 5/5
Content injection 1/1 1/1
Error handling (parse errors) 1/1 1/1
Error format contracts 1/1 1/1
Includes and partials 0/2 2/2
forloop.parentloop 0/1 1/1
Whitespace control ({%- -%}) 1/1 1/1
{% render %} scoping 0/1 1/1
{% tablerow %} 1/1 1/1
Total 10/14 14/14

Template package overall: 113/120 (remaining 7 are GoEngine stubs — Phase 4B).

Test plan

  • go test ./internal/template/... — 113/120 pass (7 GoEngine stubs expected)
  • go test ./... — no regressions in other packages
  • go vet ./... — clean
  • osteele/liquid removed from go.mod

🤖 Generated with Claude Code

Replace osteele/liquid with Notifuse/liquidgo for full Shopify Liquid
compatibility. Gains native support for {% render %} with isolated
scope, forloop.parentloop, {%- -%} whitespace control, and
{% tablerow %} — all previously missing from osteele/liquid.

14/14 liquid-specific tests now pass (was 10/14 with osteele/liquid).

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

@zeroedin zeroedin left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Code Review: feat(template): rewrite Liquid engine with Notifuse/liquidgo

Good work swapping to the spec-recommended Liquid library. The migration is clean, the dependency hygiene is solid (old deps fully removed), and the test results match the claims. A few issues to address before merge — one is a blocker for downstream phases.

Summary

Area Verdict
Dependency swap ✅ Clean — osteele/liquid, osteele/tuesday, gopkg.in/yaml.v2 all removed
Test results ✅ Verified — 113/120 pass, 7 GoEngine stubs expected
Integration regressions ✅ None — 6/16 pass (was 4/16 on main)
go vet ✅ Clean
AddFilter / AddTag stubs 🚫 Blocker — silently no-ops, breaks the TemplateEngine contract
Error handling in Render ⚠️ Fragile — swallowing errors when output is non-empty
RenderTemplate allocates new env per call ⚠️ Performance concern for hot path
ParseWithIncludes removed ℹ️ Acceptable — not in interface, noted in IMPLEMENTATION.md only

See inline comments for details.

Comment thread internal/template/liquid.go
Comment thread internal/template/liquid.go
Comment thread internal/template/liquid.go
Comment thread internal/template/liquid.go
Address PR #4 review feedback:

1. AddFilter (blocker): Create alloyFilterBridge struct with Go methods
   for all 14 Alloy-specific filters not in liquidgo's StandardFilters.
   Bridge is registered with the environment; AddFilter stores FilterFuncs
   in the map, which methods proxy to at invocation time.

2. AddTag: Create alloyTag adapter that wraps TagFunc and implements
   liquidgo's tag rendering interface (Render + RenderToOutputBuffer).
   Tags registered via AddTag are now parsed and rendered by liquidgo.

3. Error handling: Keep lax-mode conditional propagation (only error when
   output is empty). This matches Liquid's standard behavior where missing
   partials produce inline error messages rather than hard failures.
   Added TODO for strict build-mode error handling per §2.

4. RenderTemplate perf: Acknowledged, deferred — signature is locked by
   existing test contract. Will optimize in pipeline phase.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@zeroedin
zeroedin merged commit d340ff7 into main Apr 12, 2026
zeroedin added a commit that referenced this pull request Apr 25, 2026
Reviewer items:
- Use MarkdownOptions.Hooks field instead of separate function (#1)
- Tests use []byte(input) matching RenderMarkdown signature (#2)
- Heading ID uses slugify: "My Section" → "my-section" (#3)
- Protocol-relative URLs intentionally not external for v1 (#4)
- Render hooks follow engine choice (Liquid vs Go templates) (#5)
- Add blockquote and table tests (#6)

Copilot items:
- Clarify Alloy registers renderers, not goldmark auto-detecting
- Add template tag escaping note for custom hook output structures

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
zeroedin added a commit that referenced this pull request Apr 25, 2026
Reviewer items:
- Third duplicate heading test (overview, overview-1, overview-2) (#3)
- Pipeline test: page.toc accessible in layout templates (#4)
- Config path: content.markdown.toc instead of content.toc

Copilot items:
- Config path consistency with existing content.markdown.goldmark.*

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
zeroedin added a commit that referenced this pull request May 6, 2026
…ack guards

- #1 P1: matchPageGlob handles zero-segment ** match (e.g. /blog/**/index matches /blog/index)
- #4 P2: data/pageFields use !== undefined instead of || null to preserve empty arrays
- #5 P2: fireContentTransformedHooks writeback guarded by scope.WantsField checks
- #6 P2: parseScopeJSON warns on non-string taxonomy term values
- #9 P3: computeUnionScope has explicit PagesScopeNone case with comment

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
zeroedin added a commit that referenced this pull request Jul 23, 2026
- Add nil guard in buildCompletePayload for nil error elements (P2 #2)
- Copy opts in BuildWithContent to avoid aliasing caller's slice (P3 #4)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
zeroedin added a commit that referenced this pull request Jul 24, 2026
P1 #1: Fix duplicate step 19 in PLAN.md pipeline stages — renumber
steps 20-25 after inserting onFormatRendered at step 19. Update hook
table step numbers (onAssetProcess 22+, onBuildComplete 25).

P1 #2: Add content mutation write-back assertion to single non-HTML
page test — verifies page.SetRenderedBody path is wired, not just
FormatBodies.

P2 #3: Fix test count 12 → 14 in IMPLEMENTATION.md (was 13, now 14
with new test).

P2 #4: Add test for HTML in non-first position of outputs array
(outputs: ["json", "html"]) — catches page.Outputs[0] == "html"
implementations.

P2 #5: Add second plugin to read-only fields test that verifies
frontMatter immutability end-to-end — throws if first plugin's
frontMatter mutation was applied back.

P3 #6: Add BuildIncremental parity note for onFormatRendered in both
PLAN.md and IMPLEMENTATION.md.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
zeroedin added a commit that referenced this pull request Jul 24, 2026
- Fix test count 8→9 in IMPLEMENTATION.md (#1)
- Pick single outbound approach: GetOwnPropertyNames + property walk (#2)
- Use extractGoMap helper in first test instead of inline logic (#3)
- Inline toOrderedMap into extractGoMap (#4)
- Extract buildBenchHTML helper to deduplicate benchmark HTML construction (#5)
- Add path assertion to HookTransformPayload test (#11)
- Add toc outbound verification to HookTransformPayload result (#12)
- Add url, path, frontMatter assertions to identity return test (#13)
- Fix 800KB benchmark: reduce to 4300 iterations for ~800KB actual size (#14)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
zeroedin added a commit that referenced this pull request Jul 25, 2026
- P1 #6: Updated summary table (Phase 5: ~129, cumulative downstream)
- P2 #1: Added multi-byte character table entry (CJK, emoji) —
  validates X-Body-Length is byte count not rune count
- P2 #7: Added \r\n suffix to X-Body-Length assertion to prevent
  decimal-prefix false match (50 matching 500)
- P3 #2: Use parsed headers map from parseSplitFrame, assert
  header value matches actual raw body byte count
- P3 #3: Added empty split field test — falls back to single-header
- P3 #4: Added dual-key map priority test — html over content
- P3 #5: Added caller map immutability guard test

12 total tests (was 8). Test counts updated in IMPLEMENTATION.md
(section header, node.go description, impl guidance, summary table).
PLAN.md updated with empty-field fallback, dual-key priority, and
map immutability constraints.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
zeroedin added a commit that referenced this pull request Jul 25, 2026
- P0 #1: Update bridge.js inbound parser to handle split-body frames
  (X-Body-Length/X-Body-Field headers) and outbound sendMessage to
  produce split-body frames for hook results with html/content fields
- P0 #2-3: Add bounds and negative-value validation on Content-Length
  and X-Body-Length in both DecodeMessage and Send before slicing or
  allocating
- P1 #4: Require X-Body-Field header when X-Body-Length is present;
  error on empty field name
- P1 #5: Restore stdout pollution detection — first header line in
  Send must start with "Content-Length:", rejecting "debug: ..." lines
- P1 #6: Switch stripField to JSON round-trip (marshal struct → unmarshal
  to map → delete key) per IMPLEMENTATION.md spec, so new struct fields
  are automatically included
- P1 #7: DecodeMessage split-body injection prefers Result map, falls
  back to Payload map when Result is nil, per spec
- P2 #8: Use len(fieldValue) directly in EncodeMessage instead of
  allocating []byte(fieldValue) copy for byte-length measurement

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
zeroedin added a commit that referenced this pull request Jul 25, 2026
P2 #1: Hoist setupQuickJSWithHook to outer Describe scope, remove
duplicate setupQuickJSWithHookFor1186. Both #1180 and #1186 sections
now share the same helper.

P2 #2: Extract checkGlobalIsFunction closure — three existence tests
are now one-liners calling the shared helper.

P2 #3: Add *qjs.Value lifetime note to IMPLEMENTATION.md — stored
function references must be Free()d in Close() before context teardown.

P2 #4: Add lazy FM test for HookFormatRenderedPayload (onFormatRendered)
to verify setPayloadFrontMatter is wired consistently across all three
payload types.

P2 #9 (addendum): Fix null sentinel collision in __installLazyFM and
__installLazyTOC JS snippets — use unique sentinel object instead of
null so page.frontMatter = null sticks. Add corresponding test.

P3 #5: Add configurable: true test for lazy frontMatter property.

P3 #6: Add lazy TOC setter test for parity with frontMatter setter.

P3 #7: Add no-globals disambiguation test — verifies invokeHookFastPath
uses ctx.Invoke (no __callInput/__callHookName globals) vs ctx.Eval.

Test count: 9 → 14 (7 RED, 7 GREEN).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
zeroedin added a commit that referenced this pull request Jul 25, 2026
- P1 #1: Fix hook chain misrouting onContentTransformed payloads — check
  for toc key alongside html to disambiguate payloadTransform vs
  payloadRendered, preventing silent TOC data loss in multi-hook chains
- P1 #2: Log marshal errors in lazy __resolveFM/__resolveTOC callbacks
  instead of silently returning empty object/undefined
- P2 #3: Use NewFloat64 for int values in callHookMapPayload to avoid
  int32 narrowing truncation on timestamps and large counts
- P2 #4: Wire up SetRenderedHTML at 3 hook apply-back call sites in
  hooks.go (onContentTransformed html/string, onPageRendered html)
- P2 #5: Log warning on marshal failure in callHookMapPayload default
  case instead of silent continue
- P2 #6: Document thread-safety assumption on convertedFrontMatter
  in-place mutation

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant