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

Added support for iterable objects in {{#each}} helper #1557

Merged
merged 4 commits into from Sep 29, 2019
Merged

Added support for iterable objects in {{#each}} helper #1557

merged 4 commits into from Sep 29, 2019

Conversation

antelle
Copy link
Contributor

@antelle antelle commented Sep 21, 2019

Currently {{#each}} helper supports either arrays, or objects,
however nowadays you can define custom iterable objects by overriding
a special method called Symbol.iterator, which results in empty result
being rendered.

  • 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.

Currently {{#each}} helper supports either arrays, or objects,
however nowadays you can define custom iterable objects by overriding
a special method called Symbol.iterator, which results in empty result
being rendered.
@antelle antelle changed the title Added support for iterable object in {{#each}} helper Added support for iterable objects in {{#each}} helper Sep 21, 2019
@nknapp nknapp added the feature label Sep 21, 2019
@nknapp
Copy link
Collaborator

nknapp commented Sep 21, 2019

Looks interesting. I was under the impression that generator-functions were only used for asynchronous javascript (which Handlebars does not support).

I would like to wait for @wycats approval to merge this, because he wanted no new features until we have spec.

I don't know if that applies here, because this is just a helper being adapted a little, and other programming languages don't have iterators. But still...

@antelle
Copy link
Contributor Author

antelle commented Sep 21, 2019

Actually iterator doesn't have to be a generator function, it must comply to the iterator protocol, generators are just an easier way to do this. Inside it can be a normal class, ES5 function, plain object, etc...

Here's an example of implementing an iterable object without using generators, and using it in two ways: by creating an array with [...iterable] and iterating manually:

class MyIterator {
    constructor(arr) {
        this.arr = arr;
        this.index = 0;
    }
    next() {
        const value = this.arr[this.index];
        const done = this.index === this.arr.length;
        if (!done) {
          this.index++;
        }
        return { value, done };
    }
}

class MyIterable {
    constructor(arr) {
        this.arr = arr;
    }
    [Symbol.iterator]() {
        return new MyIterator(this.arr);
    }
}

const my = new MyIterable(['a', 'b', 'c']);

// using the iterator with ES6
console.log([...my]);

// using the iterator manually
const it = my[Symbol.iterator]();
for (let current = it.next(); !current.done; current = it.next()) {
    console.log(current.value);
}

@nknapp
Copy link
Collaborator

nknapp commented Sep 27, 2019

I've been thinking about this PR, and it is probably save to merge this. But your tests will fail in saucelabs with IE11, because "Symbol.iterator" is not supported there, and we currently do not transpile the tests.

So either we build babel into the tests as well (see #1512) or you rewrite the test such that "yield" and "Symbole.iterator" is not used.

I would prefer the first solution, and I have thought about refactoring the whole test mechanism (to use jest and babel), but this might take some time. So the second option is probably the only viable one for the near future.

@antelle
Copy link
Contributor Author

antelle commented Sep 27, 2019

Thanks, didn't know tests are not transpiled. But the code is transpiled I believe, right? Or should I also rewrite the code in ES5? It's using [...context] now.
Do you have a way to run tests in IE? I've pushed a fix, but I don't have IE to try it out.
// eslint config in tests should be probably adjusted to point out that they're not transpiled

@antelle
Copy link
Contributor Author

antelle commented Sep 27, 2019

Yes, the code is transpiled, but most likely we don't want to include the polyfill into the runtime just because of this one helper. So, I've rewritten it in ES5 as well 😉

@nknapp
Copy link
Collaborator

nknapp commented Sep 27, 2019

when the build is run in a local branch, it will be run in Saucelabs in a variety of browsers.
I'll have to do that manually later.

Oh, and please remind me again, if I haven't done so by next Monday. I have forgotten about some PRs in the past and I don't want this lying around for months.

@nknapp nknapp merged commit cf7545e into handlebars-lang:4.x Sep 29, 2019
@antelle antelle deleted the each-iterable branch September 29, 2019 14:03
@nknapp
Copy link
Collaborator

nknapp commented Sep 29, 2019

released in 4.4.0

@antelle
Copy link
Contributor Author

antelle commented Sep 29, 2019

Thanks a lot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants