fix: preserve parent data context for primitive optional blocks#91
Conversation
When an optional block guards a primitive value (string, number,
boolean), the runtime incorrectly navigated into the property scope,
causing named variables like {{age}} inside {{#optional age}} to
resolve as $['age']['age'] instead of $['age']. This also affected
the pre-processing step which tried to treat primitive values as
objects.
Changes:
- getJsonPath: skip path navigation for primitive OptionalDefinition
nodes so variables resolve from the parent data context
- generateOptionalBlocks: skip pre-processing for primitive optional
values and let the normal traverse handle them
- Update test templates to use named variables instead of {{this}}
for primitive optionals
- Add conditional-variables test template
Closes accordproject/template-playground#2
Signed-off-by: Yash Goel <162511050+yashhzd@users.noreply.github.com>
|
Hi @yashhzd, I see a very deep root cause analysis done by @Shubh-Raj and this two PRs you raised just implements those changes. I'm closing this and the other PR for now. I'm not sure you'd had an offline conversation with him about this. it'll be better to ask him first if he's okay with this. I'd suggest please start a conversation on the issue and ask @Shubh-Raj for this. Let us give him a week to respond, if he doesn't if we can look into reviewing and merging the PRs. |
Thanks @sanketshevkar for handling this fairly! Happy to review @yashhzd's PRs and collaborate on getting the best fix merged. |
There was a problem hiding this comment.
Pull request overview
Fixes a runtime scoping bug for primitive-typed {{#optional ...}} blocks by preventing getJsonPath from double-navigating into the guarded property and by avoiding optional-block pre-processing for primitive optional values so variables resolve from the intended parent context.
Changes:
- Update
getJsonPathto avoid appending the optional property segment for primitive optional definitions. - Update
generateOptionalBlocksto skip pre-processing when the optional value is primitive. - Refresh/extend template-based snapshot tests (update optional templates to use named variables; add
conditional-variablesfixture).
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/TemplateMarkInterpreter.ts | Adjusts JSON-path scoping and optional-block pre-processing to support primitive optional blocks correctly. |
| test/templates/good/optional_comprehensive/template.md | Updates primitive optional cases to use named variables instead of {{this}}. |
| test/templates/good/optional-nested/template.md | Updates primitive optional usage to named variable. |
| test/templates/good/conditional-variables/template.md | Adds new template fixture for conditional + variable resolution. |
| test/templates/good/conditional-variables/model.cto | Adds model for the new fixture. |
| test/templates/good/conditional-variables/data.json | Adds data for the new fixture. |
| test/snapshots/TemplateMarkInterpreter.test.ts.snap | Updates snapshots to reflect new fixture and optional rendering changes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // property scope — variables inside should resolve from the parent context | ||
| const isOptional = OPTIONAL_DEFINITION_RE.test(obj.$class); | ||
| const isPrimitive = isOptional && obj.elementType && (ModelUtil as any).isPrimitiveType(obj.elementType); | ||
| if (!isPrimitive) { |
| // property scope — variables inside should resolve from the parent context | ||
| const isOptional = OPTIONAL_DEFINITION_RE.test(obj.$class); | ||
| const isPrimitive = isOptional && obj.elementType && (ModelUtil as any).isPrimitiveType(obj.elementType); | ||
| if (!isPrimitive) { |
the first copilot suggestions seems to be a critical issue to fix, else this is ready to merge, according to me |
Description
Fixes a bug where named variables inside
{{#optional}}blocks with primitive types (string, number, boolean) would fail at runtime because the JSON path resolver incorrectly navigated into the property scope.Root cause:
OptionalDefinitionis classified as aNAVIGATION_NODE, sogetJsonPathwould append the property name to the path. For{{age}}inside{{#optional age}}, this produced$['age']['age']instead of the correct$['age']. Additionally,generateOptionalBlockstried to treat primitive values as objects during pre-processing.Closes accordproject/template-playground#2
Changes
TemplateMarkInterpreter.ts
getJsonPath— Skip path navigation forOptionalDefinitionnodes that guard primitive types (detected viaModelUtil.isPrimitiveType). This ensures variables inside primitive optional blocks resolve from the parent data context.generateOptionalBlocks— Skip pre-processing for primitive optional values (typeof value !== 'object'). Primitive values don't need recursive agreement generation — the normal traverse handles them correctly with the full parent data context.Test Updates
optional_comprehensive/template.mdandoptional-nested/template.mdto use named variables (e.g.,{{age}}) instead of{{this}}for primitive optional blocksconditional-variablestest template with model, template, and data filesAll 28 tests pass (27 existing + 1 new). The 3 pre-existing failures in
TemplateArchiveProcessor.test.tsare unrelated (missing--experimental-vm-modulesflag).Related
This fix works in conjunction with a companion PR on markdown-transform that addresses the type-checking (parsing) side of the same issue.
Author Checklist