-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Support Map for iteration and nested paths #1679
Conversation
I was trying to fix the change in code formatting around the |
I'm sorry I didn't notice the failing tests outside of the node environment before. I am trying to work out what's going wrong now... I think perhaps it's a product of |
That's the ticket. I'm keen to discuss; Handlebars is central to my project and I'm trying to make the decision between |
The code supports both Maps and POJOs, as I couldn’t decide on the best solution. Handlebars doesn’t properly support Maps yet, so I guess it’s POJOs for now (see handlebars-lang/handlebars.js#1679) I changed to indexed types over arrays as we can iterate over objects just as well (and in insertion order, see https://www.stefanjudis.com/today-i-learned/property-order-is-predictable-in-javascript-objects-since-es2015/), and with the benefit of being able to look things up by key; including in Handlebars.
Thanks for submitting this PR. I haven't been able to answer earlier, but I think your PR makes sense. There are just one point that I would like to discuss and one change request. I haven't had a very close look at the code yet though. Is this a breaking change?I am not sure if this change should go into the I would like to hear some opinions (@wycats, @ErisDS, @aorinevo) if this should be considered a breaking change. If we interprete semver strictly, it is not breaking, because iterating Maps is not documented anywhere. But one could also argue that some people might already use TestsThere is a new way of writing tests (see #1683 for details). You IDE probably didn't show the I am also not sure if we will get any merge conflicts with #1683, so I think it would be best to wait until that one is merged, and then rebase. |
Did this ever get merged? It seems the perfect solution to handling Maps in Handlebars. I haven't found a cleaner way yet. |
Memo: in order to play it safe, this will be only merged in @karlvr I know it's been 3 years, but if this PR is still relevant for you, please target in on |
@jaylinski no problem! I've left things much longer than that. I actually didn't end up using this. I had a quick look and it looks like it might benefit from a little insight to rebase it (particularly |
did handlebars support map now? |
Re #1418 and #1557 this PR adds support for accessing properties in Maps, and iterating over the values in Maps using
#each
.e.g.
+
=
4.x
-branch contains the latest version. Please target that branch in the PR.