Repository navigation
Conversation
fix: resolve block params named like @-variables by name
Unrelated to the JsonNode changes in this PR. The new JSON array iteration tests use `as |item index|`, which exposed this bug as an intermittent CI failure.
A block param that shares a name with a context variable (e.g. `{{#each items as |item index|}}`) was looked up in BlockParamsObject using the entry index of the matching @-variable in ContextDataObject. The two dictionaries usually place the key in the same slot, so this mostly worked, but when a hash collision moved the @-variable's slot, the lookup returned another block param's value or nothing. String hashes are randomized per process, so the failure was intermittent on every OS.
Look block params up by segment, as TryGetContextVariable already does. Adds a regression test covering index, key, first, last, root and parent. Because the bug depends on the process hash seed, the test cannot fail deterministically without the fix.
|
Thanks for this @jsneedles! The truthiness fix for JsonNode and the new JsonElement array access are both useful, and having the same tests run against both types is great. Below are some question and suggestions: 1. Should the block param fix be its own PR? 2. Can a wrapped JsonValue avoid GetValueKind()? 3. Should wrapped JsonValue support be in this PR at all? 4. Is the JsonArray special case needed? 5. Should JsonObject use alias providers? 6. Can the JsonNodeObjectDescriptorProvider constructor take alias providers instead of EnumerableObjectDescriptor? 7. Should JsonArray hide Parent, Root and Options? 8. Your question on the STJ version 9. Your question on output formatting (true vs True) For the release notes, these change what existing users see, so we'll call them out:
Thanks again! |
…les/Handlebars.Net into feature/json-node-support
This reverts commit eee5c6c.
|
Thanks for the thorough review @rexm I’ve narrowed the changes accordingly:
The shared JsonElement/JsonNode tests remain, with additional alias and non-serialization regressions. All tests pass, and all four library targets build. |
Hello @rexm, wanted to say how much I love this lib. We use it extensively in HTML/CSS to Image.
As a follow-up to #657, this adds support for passing
JsonNode(JsonObject/JsonArray/JsonValue) directly into templates, with the same behavior asJsonElement: member access,#eachwith@index/@key/@first/@last, truthiness,includeZero, and partials.It also expands
JsonElement:{{items.[0]}},{{lookup items 1}}){{items.Count}}on arrays, plus{{items.length}}whenUseCollectionMemberAliasProvider()is enabled (same as other collections)AddAlias<JsonElement>(...))A
JsonValuebuilt in code that wraps a .NET collection or object (e.g.JsonValue.Create(new List<string>())) behaves like the wrapped value: it can be iterated, its members accessed, and it can be passed to partials. Members resolve by .NET property name, so[JsonPropertyName]and naming policies don't apply. The README documents this.I moved the existing
JsonElementtests out ofDynamicTestsinto a newJsonTestsfile, where a shared base class runs the same cases against bothJsonElementandJsonNode.Public API additions are non-breaking:
JsonNodeObjectDescriptorProvider,JsonNodeMemberAccessor,JsonNodeIterator, and new constructor overloads onJsonElementObjectDescriptorProvider/JsonElementMemberAccessorthat take alias providers (the parameterless ones remain).A few questions:
System.Text.Json version. The netstandard targets pin STJ 8.0.5 (net8.0+ use the in-box version). .NET 8 / STJ 8.x go out of support on Nov 10, 2026, and 9+ also caches
JsonNode.GetValueKind(), which the wrapped-value path uses. Bumping the netstandard reference to 10.x would mean .NET Framework consumers take STJ 10 and its dependencies. Worth doing, here or in a separate PR?Output formatting.
{{value}}output differs slightly between the two types: aJsonNodebool renderstrue(its JSON form), while aJsonElementbool rendersTruelike any other .NET bool. Likewise, aJsonValuebuilt in code that holds aDateTimerenders quoted, because it stringifies to JSON. I could add a formatter that unwrapsJsonValueto its underlying .NET value so both types match normal Handlebars.Net output (True,DateTimeFormatterdates). That's also what Handlebars.Net.Extension.Json does, since it converts JSON values to .NET values. Or we could keep JSON-style output. Preference?