Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make #each handle non-iterable values like the original JavaScript version #380

Merged

Conversation

mkantor
Copy link
Contributor

@mkantor mkantor commented Sep 7, 2020

Previously things like {{#each "foo"}} were render errors. Now non-iterable values are treated like empty iterables. This brings handlebars-rust's behavior closer in line with the original JavaScript library.

Previously things like `{{#each "foo"}}` were render errors. Now
non-iterable values are treated like empty iterables. This brings
handlebars-rust's behavior closer in line with the original JavaScript
library.

Here are the two new test cases in the handlebars playground:

1. https://handlebarsjs.com/playground.html#format=1&currentExample=%7B%22template%22%3A%22%7B%7B%23each%20this%7D%7Deach%20block%7B%7Belse%7D%7Delse%20block%7B%7B%2Feach%7D%7D%22%2C%22partials%22%3A%5B%5D%2C%22input%22%3A%22%5C%22strings%20aren't%20iterable%5C%22%22%2C%22output%22%3A%22else%20block%22%2C%22preparationScript%22%3A%22%22%2C%22handlebarsVersion%22%3A%224.7.6%22%7D

2. https://handlebarsjs.com/playground.html#format=1&currentExample=%7B%22template%22%3A%22%7B%7B%23*inline%20%5C%22walk%5C%22%7D%7D(%7B%7B%23each%20this%7D%7D%7B%7B%23if%20%40key%7D%7D%7B%7B%40key%7D%7D%7B%7Belse%7D%7D%7B%7B%40index%7D%7D%7B%7B%2Fif%7D%7D%3A%20%7B%7Bthis%7D%7D%20%7B%7B%3E%20walk%20this%7D%7D%2C%20%7B%7B%2Feach%7D%7D)%7B%7B%2Finline%7D%7D%5Cn%7B%7B%3E%20walk%7D%7D%22%2C%22partials%22%3A%5B%5D%2C%22input%22%3A%22%7B%5Cn%20%20%20%20%5C%22array%5C%22%3A%20%5B42%2C%20%7B%5C%22wow%5C%22%3A%20%5C%22cool%5C%22%7D%2C%20%5B%5B%5D%5D%5D%2C%5Cn%20%20%20%20%5C%22object%5C%22%3A%20%7B%20%5C%22a%5C%22%3A%20%7B%20%5C%22b%5C%22%3A%20%5C%22c%5C%22%2C%20%5C%22d%5C%22%3A%20%5B%5C%22e%5C%22%5D%20%7D%20%7D%2C%5Cn%20%20%20%20%5C%22string%5C%22%3A%20%5C%22hi%5C%22%5Cn%7D%22%2C%22output%22%3A%22%5Cn(array%3A%2042%2C%5Bobject%20Object%5D%2C%20(0%3A%2042%20()%2C%201%3A%20%5Bobject%20Object%5D%20(wow%3A%20cool%20()%2C%20)%2C%202%3A%20%20(0%3A%20%20()%2C%20)%2C%20)%2C%20object%3A%20%5Bobject%20Object%5D%20(a%3A%20%5Bobject%20Object%5D%20(b%3A%20c%20()%2C%20d%3A%20e%20(0%3A%20e%20()%2C%20)%2C%20)%2C%20)%2C%20string%3A%20hi%20()%2C%20)%22%2C%22preparationScript%22%3A%22%22%2C%22handlebarsVersion%22%3A%224.7.6%22%7D
let input = json!("strings aren't iterable");
let rendered = reg.render_template(template, &input).unwrap();
assert_eq!("else block", rendered);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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


let rendered = reg.render("walk", &input).unwrap();
assert_eq!(expected_output, rendered);
}
Copy link
Contributor Author

@mkantor mkantor Sep 7, 2020

Choose a reason for hiding this comment

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

Compatibility check.

The output of this one differs slightly, but only because handlebars-rust stringifies objects and arrays in its own way (FWIW I like the handlebars-rust stringifications better).

Copy link
Contributor Author

@mkantor mkantor Sep 7, 2020

Choose a reason for hiding this comment

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

Recursion was my main motivation behind this pull request, by the way. I couldn't recursively iterate through data structures because handlebars-rust would error on the first non-iterable value it encountered.

@mkantor
Copy link
Contributor Author

mkantor commented Sep 7, 2020

It doesn't change the behavior, but I think this additional tweak improves readability a bit:

diff --git a/src/helpers/helper_each.rs b/src/helpers/helper_each.rs
index 0814360..8137f2d 100644
--- a/src/helpers/helper_each.rs
+++ b/src/helpers/helper_each.rs
@@ -5,7 +5,7 @@ use crate::block::{BlockContext, BlockParams};
 use crate::context::Context;
 use crate::error::RenderError;
 use crate::helpers::{HelperDef, HelperResult};
-use crate::json::value::{to_json, JsonTruthy};
+use crate::json::value::to_json;
 use crate::output::Output;
 use crate::registry::Registry;
 use crate::render::{Helper, RenderContext, Renderable};
@@ -79,8 +79,8 @@ impl HelperDef for EachHelper {
         let template = h.template();
 
         match template {
-            Some(t) => match (value.value().is_truthy(false), value.value()) {
-                (true, &Json::Array(ref list)) => {
+            Some(t) => match value.value() {
+                &Json::Array(ref list) if !list.is_empty() => {
                     let block_context = create_block(&value)?;
                     rc.push_block(block_context);
 
@@ -108,7 +108,7 @@ impl HelperDef for EachHelper {
                     rc.pop_block();
                     Ok(())
                 }
-                (true, &Json::Object(ref obj)) => {
+                &Json::Object(ref obj) if !obj.is_empty() => {
                     let block_context = create_block(&value)?;
                     rc.push_block(block_context);

Let me know if you like it and I'll push another commit.

@sunng87
Copy link
Owner

sunng87 commented Sep 8, 2020

Thank you very much. This is great addition!

Copy link
Owner

@sunng87 sunng87 left a comment

Choose a reason for hiding this comment

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

It seems we can remove the truthy check in https://github.com/sunng87/handlebars-rust/pull/380/files#diff-6f7f1fa0fa8cb634e6e7d062be921ac8L82 because else will be rendered for any case that non-iterable

Update: #380 (comment) is welcomed

@sunng87
Copy link
Owner

sunng87 commented Sep 8, 2020

It doesn't change the behavior, but I think this additional tweak improves readability a bit:

diff --git a/src/helpers/helper_each.rs b/src/helpers/helper_each.rs
index 0814360..8137f2d 100644
--- a/src/helpers/helper_each.rs
+++ b/src/helpers/helper_each.rs
@@ -5,7 +5,7 @@ use crate::block::{BlockContext, BlockParams};
 use crate::context::Context;
 use crate::error::RenderError;
 use crate::helpers::{HelperDef, HelperResult};
-use crate::json::value::{to_json, JsonTruthy};
+use crate::json::value::to_json;
 use crate::output::Output;
 use crate::registry::Registry;
 use crate::render::{Helper, RenderContext, Renderable};
@@ -79,8 +79,8 @@ impl HelperDef for EachHelper {
         let template = h.template();
 
         match template {
-            Some(t) => match (value.value().is_truthy(false), value.value()) {
-                (true, &Json::Array(ref list)) => {
+            Some(t) => match value.value() {
+                &Json::Array(ref list) if !list.is_empty() => {
                     let block_context = create_block(&value)?;
                     rc.push_block(block_context);
 
@@ -108,7 +108,7 @@ impl HelperDef for EachHelper {
                     rc.pop_block();
                     Ok(())
                 }
-                (true, &Json::Object(ref obj)) => {
+                &Json::Object(ref obj) if !obj.is_empty() => {
                     let block_context = create_block(&value)?;
                     rc.push_block(block_context);

Let me know if you like it and I'll push another commit.

Just saw this. I'm +1 for it. Also the if check in pattern matching is new to me.

@mkantor
Copy link
Contributor Author

mkantor commented Sep 8, 2020

Thanks for taking a look! I pushed 578590a 5930fdd (clippy had a suggestion so I amended the commit) with the readability improvement from #380 (comment).


the if check in pattern matching is new to me

FYI they're called match guards and can be pretty handy, although a downside is the compiler doesn't check them for exhaustiveness like it does with unguarded matches (not a problem here since we have a _ case).

@mkantor mkantor force-pushed the compat-each-should-allow-non-iterable-values branch from 578590a to 5930fdd Compare September 8, 2020 17:35
@sunng87 sunng87 merged commit 6b11126 into sunng87:master Sep 9, 2020
sunng87 added a commit that referenced this pull request Sep 9, 2020
…rable-values

Make #each handle non-iterable values like the original JavaScript version
@mkantor mkantor deleted the compat-each-should-allow-non-iterable-values branch September 18, 2020 18:31
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.

None yet

2 participants