Skip to content

fix: partial context with literal parameters#695

Merged
sunng87 merged 3 commits into
masterfrom
fix/partial-context-constants
Feb 25, 2025
Merged

fix: partial context with literal parameters#695
sunng87 merged 3 commits into
masterfrom
fix/partial-context-constants

Conversation

@sunng87

@sunng87 sunng87 commented Feb 25, 2025

Copy link
Copy Markdown
Owner

This patch add support for literal parameters on partial.

Fixes #643

@coveralls

coveralls commented Feb 25, 2025

Copy link
Copy Markdown

Coverage Status

coverage: 81.972% (+0.05%) from 81.924%
when pulling 0ca5012 on fix/partial-context-constants
into 6055df6 on master.

@mkantor

mkantor commented Feb 25, 2025

Copy link
Copy Markdown
Contributor

I'm not sure if this is within the scope of this pull request, but it seems that even with this fix, hash params in this are not iterated by #each. Here's how I tested that:

--- a/src/partial.rs
+++ b/src/partial.rs
@@ -754,5 +754,14 @@ outer third line",
         hbs.register_template_string("t2", t2).unwrap();
 
         assert_eq!("1", hbs.render("t2", &()).unwrap());
+
+        let t1 = "{{#each this}}{{@key}}:{{this}},{{/each}}";
+        let t2 = "{{> t1 a=1}}";
+
+        let mut hbs = Registry::new();
+        hbs.register_template_string("t1", t1).unwrap();
+        hbs.register_template_string("t2", t2).unwrap();
+
+        assert_eq!("a:1,", hbs.render("t2", &()).unwrap());
     }
 }

(Same problem occurs when using block param syntax, e.g. {{#each this as |value key|}}{{key}}:{{value}},{{/each}}).

It turns out this is a recent regression—it works in handlebars-rust v6.3.0 but not v6.3.1 (it also works in handlebars.js). Perhaps it was a side effect of #694?

Please let me know if you'd like me to file a separate issue about this.

@sunng87

sunng87 commented Feb 25, 2025

Copy link
Copy Markdown
Owner Author

@mkantor Thank you for reporting! In #694 I changed hash implementation from merging into context to block param. It seems we still need to merge the hash into the partial context for this.

But this is another issue can be address in separated PR. I will merge this and work on the issue when I have ideas.

@sunng87 sunng87 merged commit e9c4fe7 into master Feb 25, 2025
@sunng87 sunng87 deleted the fix/partial-context-constants branch February 25, 2025 20:04
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.

Not work partial context

3 participants