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

Jump to bottom

Support Map for iteration and nested paths #1679

Open
wants to merge 3 commits into
base: 4.x
Choose a base branch
from
Open

Support Map for iteration and nested paths #1679

wants to merge 3 commits into from

Conversation

karlvr
Copy link

@karlvr karlvr commented Apr 16, 2020 •
edited

Re #1418 and #1557 this PR adds support for accessing properties in Maps, and iterating over the values in Maps using #each.

e.g.

var foo = new Map();
foo.set('expression', 'beautiful');
...

+

Goodbye {{foo/expression}} world!
{{#each foo}}
The {{@key}} is {{.}}.
{{/each}}

=

Goodbye beautiful world!
The expression is beautiful.
  • Please don't start pull requests for security issues. Instead, file a report at https://www.npmjs.com/advisories/report?package=handlebars
  • Maintain the existing code style
  • Are focused on a single change (i.e. avoid large refactoring or style adjustments in untouched code if not the primary goal of the pull request)
  • Have good commit messages
  • Have tests
  • Have the typings (lib/handlebars.d.ts) updated on every API change. If you need help, updating those, please mention that in the PR description.
  • Don't significantly decrease the current code coverage (see coverage/lcov-report/index.html)
  • Currently, the 4.x-branch contains the latest version. Please target that branch in the PR.

@karlvr karlvr mentioned this pull request Apr 16, 2020
Closed
@karlvr
Copy link
Author

karlvr commented Apr 16, 2020

I was trying to fix the change in code formatting around the blockParams, but it looks like there's some automatic reformatting on commit? Something's doing something :-D

@karlvr
Copy link
Author

karlvr commented Apr 16, 2020 •
edited

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 webpack babel.

@karlvr
Copy link
Author

karlvr commented Apr 16, 2020

That's the ticket. I'm keen to discuss; Handlebars is central to my project and I'm trying to make the decision between Map and POJOs.

karlvr added a commit to karlvr/openapi-generator-plus that referenced this pull request Apr 16, 2020
@karlvr
core: change from arrays to indexed objects
5f9f4d8 
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.
@nknapp
Copy link
Collaborator

nknapp commented Apr 26, 2020

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 4.x-branch. In #1557, you said that Maps could be iterated at the moment, they would be iterate like an array of key-value pairs, rather than like an object (values with @key containing the key).

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 {{#each}} to iterate maps and expect key-value pairs in the iteration. Support for iterable objects was added only a couple of months ago in 4.4.0, so there is only a limited changes that this is the case

Tests

There is a new way of writing tests (see #1683 for details). You IDE probably didn't show the shouldCompileTo-function as deprecated, but the new way is to use expectTemplate(...).toCompileTo(...) instead. It makes the test-code much more readable.

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.

@karlvr
Copy link
Author

karlvr commented Apr 27, 2020

@nknapp thanks for your message. No, I didn't see that! I've converted those tests. I'm also happy to rebase and make any adjustments when #1683 lands.

@nickshreck
Copy link

Did this ever get merged? It seems the perfect solution to handling Maps in Handlebars. I haven't found a cleaner way yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@karlvr @nknapp @nickshreck @jaylinski